Correct locking and cancellation cleanup in syslog functions (bug 26100)
Commit Message
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(-)
Comments
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.
@@ -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);
}