Correct locking and cancellation cleanup in syslog functions (bug 26100)

Message ID mvm5zbandul.fsf@suse.de
State New
Headers show
Series
  • Correct locking and cancellation cleanup in syslog functions (bug 26100)
Related show

Commit Message

Andreas Schwab June 29, 2020, 2:30 p.m.
Properly serialize the access to the global state shared between the
syslog functions, to avoid races in multithreaded processes.  Protect a
local allocation in the __vsyslog_internal function from leaking during
cancellation.
---
 misc/syslog.c | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

-- 
2.26.2


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Comments

Ben Woodard via Libc-alpha July 30, 2020, 2:15 p.m. | #1
On 29/06/2020 11:30, Andreas Schwab wrote:
> Properly serialize the access to the global state shared between the

> syslog functions, to avoid races in multithreaded processes.  Protect a

> local allocation in the __vsyslog_internal function from leaking during

> cancellation.


LGTM, thanks.  

As a side note, I think we could simplify this code a bit if we just 
define syslog as non-cancellable (since POSIX states it is a 'may' 
entrypoint) and to remove the internal 'syslog' call on __vsyslog_internal.

Former would allow to just call __pthread_setcancelstate / 
__pthread_setcancelstate on each required symbol and former would
allow to move the lock/unlock out of __vsyslog_internal.  We might still
check the NO_SIGPIPE necessity (as least Linux explicit sets it, not
sure if Hurd requires it).

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>


> ---

>  misc/syslog.c | 44 ++++++++++++++++++++++++++++----------------

>  1 file changed, 28 insertions(+), 16 deletions(-)

> 

> diff --git a/misc/syslog.c b/misc/syslog.c

> index fd6537edf6..2cc63ef287 100644

> --- a/misc/syslog.c

> +++ b/misc/syslog.c

> @@ -91,14 +91,20 @@ struct cleanup_arg

>  static void

>  cancel_handler (void *ptr)

>  {

> -#ifndef NO_SIGPIPE

>    /* Restore the old signal handler.  */

>    struct cleanup_arg *clarg = (struct cleanup_arg *) ptr;

>  

> -  if (clarg != NULL && clarg->oldaction != NULL)

> -    __sigaction (SIGPIPE, clarg->oldaction, NULL);

> +  if (clarg != NULL)

> +    {

> +#ifndef NO_SIGPIPE

> +      if (clarg->oldaction != NULL)

> +	__sigaction (SIGPIPE, clarg->oldaction, NULL);

>  #endif

>  

> +      /* Free the memstream buffer,  */

> +      free (clarg->buf);

> +    }

> +

>    /* Free the lock.  */

>    __libc_lock_unlock (syslog_lock);

>  }


Ok.

> @@ -169,9 +175,17 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,

>  		pri &= LOG_PRIMASK|LOG_FACMASK;

>  	}

>  

> +	/* Prepare for multiple users.  We have to take care: most

> +	   syscalls we are using are cancellation points.  */

> +	struct cleanup_arg clarg;

> +	clarg.buf = NULL;

> +	clarg.oldaction = NULL;

> +	__libc_cleanup_push (cancel_handler, &clarg);

> +	__libc_lock_lock (syslog_lock);

> +

>  	/* Check priority against setlogmask values. */

>  	if ((LOG_MASK (LOG_PRI (pri)) & LogMask) == 0)

> -		return;

> +		goto out;

>  

>  	/* Set default facility if none specified. */

>  	if ((pri & LOG_FACMASK) == 0)


Ok.

> @@ -235,6 +249,9 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,

>  	    /* Close the memory stream; this will finalize the data

>  	       into a malloc'd buffer in BUF.  */

>  	    fclose (f);

> +

> +	    /* Tell the cancellation handler to free this buffer.  */

> +	    clarg.buf = buf;

>  	  }

>  

>  	/* Output to stderr if requested. */


Ok.

> @@ -252,22 +269,10 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,

>  		    v->iov_len = 1;

>  		  }

>  

> -		__libc_cleanup_push (free, buf == failbuf ? NULL : buf);

> -

>  		/* writev is a cancellation point.  */

>  		(void)__writev(STDERR_FILENO, iov, v - iov + 1);

> -

> -		__libc_cleanup_pop (0);

>  	}

>  

> -	/* Prepare for multiple users.  We have to take care: open and

> -	   write are cancellation points.  */

> -	struct cleanup_arg clarg;

> -	clarg.buf = buf;

> -	clarg.oldaction = NULL;

> -	__libc_cleanup_push (cancel_handler, &clarg);

> -	__libc_lock_lock (syslog_lock);

> -

>  #ifndef NO_SIGPIPE

>  	/* Prepare for a broken connection.  */

>   	memset (&action, 0, sizeof (action));


Ok.

> @@ -320,6 +325,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap,

>  		__sigaction (SIGPIPE, &oldaction, (struct sigaction *) NULL);

>  #endif

>  

> + out:

>  	/* End of critical section.  */

>  	__libc_cleanup_pop (0);

>  	__libc_lock_unlock (syslog_lock);

> @@ -430,8 +436,14 @@ setlogmask (int pmask)

>  {

>  	int omask;

>  

> +	/* Protect against multiple users.  */

> +	__libc_lock_lock (syslog_lock);

> +

>  	omask = LogMask;

>  	if (pmask != 0)

>  		LogMask = pmask;

> +

> +	__libc_lock_unlock (syslog_lock);

> +

>  	return (omask);

>  }

> 


Ok.

Patch

diff --git a/misc/syslog.c b/misc/syslog.c
index fd6537edf6..2cc63ef287 100644
--- a/misc/syslog.c
+++ b/misc/syslog.c
@@ -91,14 +91,20 @@  struct cleanup_arg
 static void
 cancel_handler (void *ptr)
 {
-#ifndef NO_SIGPIPE
   /* Restore the old signal handler.  */
   struct cleanup_arg *clarg = (struct cleanup_arg *) ptr;
 
-  if (clarg != NULL && clarg->oldaction != NULL)
-    __sigaction (SIGPIPE, clarg->oldaction, NULL);
+  if (clarg != NULL)
+    {
+#ifndef NO_SIGPIPE
+      if (clarg->oldaction != NULL)
+	__sigaction (SIGPIPE, clarg->oldaction, NULL);
 #endif
 
+      /* Free the memstream buffer,  */
+      free (clarg->buf);
+    }
+
   /* Free the lock.  */
   __libc_lock_unlock (syslog_lock);
 }
@@ -169,9 +175,17 @@  __vsyslog_internal(int pri, const char *fmt, va_list ap,
 		pri &= LOG_PRIMASK|LOG_FACMASK;
 	}
 
+	/* Prepare for multiple users.  We have to take care: most
+	   syscalls we are using are cancellation points.  */
+	struct cleanup_arg clarg;
+	clarg.buf = NULL;
+	clarg.oldaction = NULL;
+	__libc_cleanup_push (cancel_handler, &clarg);
+	__libc_lock_lock (syslog_lock);
+
 	/* Check priority against setlogmask values. */
 	if ((LOG_MASK (LOG_PRI (pri)) & LogMask) == 0)
-		return;
+		goto out;
 
 	/* Set default facility if none specified. */
 	if ((pri & LOG_FACMASK) == 0)
@@ -235,6 +249,9 @@  __vsyslog_internal(int pri, const char *fmt, va_list ap,
 	    /* Close the memory stream; this will finalize the data
 	       into a malloc'd buffer in BUF.  */
 	    fclose (f);
+
+	    /* Tell the cancellation handler to free this buffer.  */
+	    clarg.buf = buf;
 	  }
 
 	/* Output to stderr if requested. */
@@ -252,22 +269,10 @@  __vsyslog_internal(int pri, const char *fmt, va_list ap,
 		    v->iov_len = 1;
 		  }
 
-		__libc_cleanup_push (free, buf == failbuf ? NULL : buf);
-
 		/* writev is a cancellation point.  */
 		(void)__writev(STDERR_FILENO, iov, v - iov + 1);
-
-		__libc_cleanup_pop (0);
 	}
 
-	/* Prepare for multiple users.  We have to take care: open and
-	   write are cancellation points.  */
-	struct cleanup_arg clarg;
-	clarg.buf = buf;
-	clarg.oldaction = NULL;
-	__libc_cleanup_push (cancel_handler, &clarg);
-	__libc_lock_lock (syslog_lock);
-
 #ifndef NO_SIGPIPE
 	/* Prepare for a broken connection.  */
  	memset (&action, 0, sizeof (action));
@@ -320,6 +325,7 @@  __vsyslog_internal(int pri, const char *fmt, va_list ap,
 		__sigaction (SIGPIPE, &oldaction, (struct sigaction *) NULL);
 #endif
 
+ out:
 	/* End of critical section.  */
 	__libc_cleanup_pop (0);
 	__libc_lock_unlock (syslog_lock);
@@ -430,8 +436,14 @@  setlogmask (int pmask)
 {
 	int omask;
 
+	/* Protect against multiple users.  */
+	__libc_lock_lock (syslog_lock);
+
 	omask = LogMask;
 	if (pmask != 0)
 		LogMask = pmask;
+
+	__libc_lock_unlock (syslog_lock);
+
 	return (omask);
 }