[PATCH v3 7/8] libcamera: base: log: Protect log categories with lock

[Laurent Pinchart]

Hi Barnabás,

Thank you for the patch.

On Mon, Feb 17, 2025 at 06:55:05PM +0000, Barnabás Pőcze wrote:
> Log categories may be added from any thread, so it is important to
> synchronize access to the `Logger::categories_` list between its two
> users: category creation (by LogCategory::create(), which calls
> Logger::findCategory() and Logger::registerCategory()); and log level
> setting (by Logger::logSetLevel()).
> 
> The LogCategory::create() function uses a mutex to serialize category
> creation, but Logger::logSetLevel() can access `Logger::categories_`
> concurrently without any protection. To fix the issue, move the mutex to
> the Logger class, and use it to protect all accesses to the categories
> list. This requires moving all the logic of LogCategory::create() to a
> new Logger::findOrCreateCategory() function that combines both
> Logger::findCategory() and Logger::registerCategory() in order to make
> the two operations exacute atomically.
> 
> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
>  include/libcamera/base/log.h |  1 +
>  src/libcamera/base/log.cpp   | 58 +++++++++++++-----------------------
>  2 files changed, 22 insertions(+), 37 deletions(-)
> 
> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
> index 195426c41..8ae1b51d0 100644
> --- a/include/libcamera/base/log.h
> +++ b/include/libcamera/base/log.h
> @@ -39,6 +39,7 @@ public:
>  	static const LogCategory &defaultCategory();
>  
>  private:
> +	friend class Logger;
>  	explicit LogCategory(std::string_view name);
>  
>  	const std::string name_;
> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> index 2c233be36..fd6c11716 100644
> --- a/src/libcamera/base/log.cpp
> +++ b/src/libcamera/base/log.cpp
> @@ -317,12 +317,12 @@ private:
>  	static LogSeverity parseLogLevel(std::string_view level);
>  
>  	friend LogCategory;
> -	void registerCategory(LogCategory *category);
> -	LogCategory *findCategory(std::string_view name) const;
> +	LogCategory *findOrCreateCategory(std::string_view name);
>  
>  	static bool destroyed_;
>  
> -	std::vector<LogCategory *> categories_;
> +	Mutex mutex_;
> +	std::vector<LogCategory *> categories_ LIBCAMERA_TSA_GUARDED_BY(mutex_);
>  	std::list<std::pair<std::string, LogSeverity>> levels_;
>  
>  	std::atomic<std::shared_ptr<LogOutput>> output_;
> @@ -572,6 +572,8 @@ void Logger::logSetLevel(const char *category, const char *level)
>  	if (severity == LogInvalid)
>  		return;
>  
> +	MutexLocker locker(mutex_);
> +
>  	for (LogCategory *c : categories_) {
>  		if (c->name() == category) {
>  			c->setSeverity(severity);
> @@ -708,37 +710,28 @@ LogSeverity Logger::parseLogLevel(std::string_view level)
>  }
>  
>  /**
> - * \brief Register a log category with the logger
> - * \param[in] category The log category
> - *
> - * Log categories must have unique names. It is invalid to call this function
> - * if a log category with the same name already exists.
> + * \brief Find an existing log category with the given name or create one
> + * \param[in] name Name of the log category
> + * \return The pointer to the log category found or created
>   */
> -void Logger::registerCategory(LogCategory *category)
> +LogCategory *Logger::findOrCreateCategory(std::string_view name)
>  {
> -	categories_.push_back(category);
> +	MutexLocker locker(mutex_);
>  
> -	const std::string &name = category->name();
> -	for (const auto &[pattern, severity] : levels_) {
> -		if (fnmatch(pattern.c_str(), name.c_str(), FNM_NOESCAPE) == 0)
> -			category->setSeverity(severity);
> +	for (LogCategory *category : categories_) {
> +		if (category->name() == name)
> +			return category;
>  	}
> -}
>  
> -/**
> - * \brief Find an existing log category with the given name
> - * \param[in] name Name of the log category
> - * \return The pointer to the found log category or nullptr if not found
> - */
> -LogCategory *Logger::findCategory(std::string_view name) const
> -{
> -	if (auto it = std::find_if(categories_.begin(), categories_.end(),
> -				   [name](auto c) { return c->name() == name; });
> -	    it != categories_.end()) {
> -		return *it;
> +	LogCategory *category = categories_.emplace_back(new LogCategory(name));
> +	const char *name_cstr = category->name().c_str();

camelCase please. You could name is categoryName.

Any reason to not keep

	const std::string &categoryName = category->name();

here ? Both should be fine, but if Category::name() were ever changef to
return a std::string by value, the code above would lead to a
use-after-free. Up to you.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
> +	for (const auto &[pattern, severity] : levels_) {
> +		if (fnmatch(pattern.c_str(), name_cstr, FNM_NOESCAPE) == 0)
> +			category->setSeverity(severity);
>  	}
>  
> -	return nullptr;
> +	return category;
>  }
>  
>  /**
> @@ -776,16 +769,7 @@ LogCategory *Logger::findCategory(std::string_view name) const
>   */
>  LogCategory *LogCategory::create(std::string_view name)
>  {
> -	static Mutex mutex_;
> -	MutexLocker locker(mutex_);
> -	LogCategory *category = Logger::instance()->findCategory(name);
> -
> -	if (!category) {
> -		category = new LogCategory(name);
> -		Logger::instance()->registerCategory(category);
> -	}
> -
> -	return category;
> +	return Logger::instance()->findOrCreateCategory(name);
>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart