* [PATCH] Correct locking and cancellation cleanup in syslog functions (bug 26100)
@ 2020-06-29 14:30 Andreas Schwab
2020-07-30 14:15 ` Adhemerval Zanella via Libc-alpha
0 siblings, 1 reply; 2+ messages in thread
From: Andreas Schwab @ 2020-06-29 14:30 UTC (permalink / raw)
To: libc-alpha
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(-)
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);
}
--
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."
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] Correct locking and cancellation cleanup in syslog functions (bug 26100)
2020-06-29 14:30 [PATCH] Correct locking and cancellation cleanup in syslog functions (bug 26100) Andreas Schwab
@ 2020-07-30 14:15 ` Adhemerval Zanella via Libc-alpha
0 siblings, 0 replies; 2+ messages in thread
From: Adhemerval Zanella via Libc-alpha @ 2020-07-30 14:15 UTC (permalink / raw)
To: libc-alpha, Andreas Schwab
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-07-30 14:15 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-29 14:30 [PATCH] Correct locking and cancellation cleanup in syslog functions (bug 26100) Andreas Schwab
2020-07-30 14:15 ` Adhemerval Zanella via Libc-alpha
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).