[dm-devel] [PATCH 2/7] libmultipath: protect logarea with logq_lock

[Benjamin Marzinski]

On Thu, Dec 17, 2020 at 12:00:13PM +0100, mwilck at suse.com wrote:
> From: Martin Wilck <mwilck at suse.com>
> 
> Make sure the global logarea (la) is only allocated and freed
> hile holding logq_lock. This avoids invalid memory access.
> 
> This patch makes free_logarea() static. libmultipath.version
> is unchanged, as free_logarea() wasn't exported anyway.
> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> ---
>  libmultipath/log.c | 32 +++++++++++++++++++++++---------
>  libmultipath/log.h |  1 -
>  2 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/libmultipath/log.c b/libmultipath/log.c
> index 7f33787..95c8f01 100644
> --- a/libmultipath/log.c
> +++ b/libmultipath/log.c
> @@ -77,16 +77,23 @@ static int logarea_init (int size)
>  
>  int log_init(char *program_name, int size)
>  {
> +	int ret = 1;
> +
>  	logdbg(stderr,"enter log_init\n");
> +
> +	pthread_mutex_lock(&logq_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> +
>  	openlog(program_name, 0, LOG_DAEMON);
> +	if (!la)
> +		ret = logarea_init(size);
>  
> -	if (logarea_init(size))
> -		return 1;
> +	pthread_cleanup_pop(1);
>  
> -	return 0;
> +	return ret;
>  }
>  
> -void free_logarea (void)
> +static void free_logarea (void)
>  {
>  	FREE(la->start);
>  	FREE(la->buff);

I realize that the log area can only be freed by log_close(), which is
called when mulitpathd exits, but it would be nice to have la set to
NULL it's freed, just to make it obvious that that there can't be
double-frees there. However, the code is clearly correct, so

Reviewed-by: Benjamin Marzinski <bmarzins at redhat.com>

> @@ -96,9 +103,14 @@ void free_logarea (void)
>  
>  void log_close (void)
>  {
> -	free_logarea();
> +	pthread_mutex_lock(&logq_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> +
> +	if (la)
> +		free_logarea();
>  	closelog();
>  
> +	pthread_cleanup_pop(1);
>  	return;
>  }
>  
> @@ -175,11 +187,12 @@ static int _log_enqueue(int prio, const char * fmt, va_list ap)
>  
>  int log_enqueue(int prio, const char *fmt, va_list ap)
>  {
> -	int ret;
> +	int ret = 1;
>  
>  	pthread_mutex_lock(&logq_lock);
>  	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> -	ret = _log_enqueue(prio, fmt, ap);
> +	if (la)
> +		ret = _log_enqueue(prio, fmt, ap);
>  	pthread_cleanup_pop(1);
>  	return ret;
>  }
> @@ -215,11 +228,12 @@ static int _log_dequeue(void *buff)
>  
>  int log_dequeue(void *buff)
>  {
> -	int ret;
> +	int ret = 1;
>  
>  	pthread_mutex_lock(&logq_lock);
>  	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> -	ret = _log_dequeue(buff);
> +	if (la)
> +		ret = _log_dequeue(buff);
>  	pthread_cleanup_pop(1);
>  	return ret;
>  }
> diff --git a/libmultipath/log.h b/libmultipath/log.h
> index d2448f6..fa224e4 100644
> --- a/libmultipath/log.h
> +++ b/libmultipath/log.h
> @@ -39,6 +39,5 @@ int log_enqueue (int prio, const char * fmt, va_list ap)
>  int log_dequeue (void *);
>  void log_syslog (void *);
>  void dump_logmsg (void *);
> -void free_logarea (void);
>  
>  #endif /* LOG_H */
> -- 
> 2.29.0