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

22807 diff mbox series

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@protonmail.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 include/libcamera/base/log.h |  1 +
 src/libcamera/base/log.cpp   | 58 +++++++++++++-----------------------
 2 files changed, 22 insertions(+), 37 deletions(-)

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@protonmail.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@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@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);
>  }
>  
>  /**