[PATCH] Mark syslog as MT-unsafe (bug 26100)

[PATCH] Mark syslog as MT-unsafe (bug 26100)

[Andreas Schwab]

The syslog functions have been marked MT-safe by mistake.  They use global
variables in unsafe way.
---
 manual/syslog.texi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/manual/syslog.texi b/manual/syslog.texi
index 02f84d6e6f..3097cc93fd 100644
--- a/manual/syslog.texi
+++ b/manual/syslog.texi
@@ -146,7 +146,7 @@ The symbols referred to in this section are declared in the file
 
 @deftypefun void openlog (const char *@var{ident}, int @var{option}, int @var{facility})
 @standards{BSD, syslog.h}
-@safety{@prelim{}@mtsafe{}@asunsafe{@asulock{}}@acunsafe{@aculock{} @acsfd{}}}
+@safety{@prelim{}@mtunsafe{@mtasurace{:LogTag/LogMask}}@asunsafe{@asulock{}}@acunsafe{@aculock{} @acsfd{}}}
 @c openlog @asulock @aculock @acsfd
 @c  libc_lock_lock @asulock @aculock
 @c  openlog_internal @acsfd [always guarded by syslog_lock, so no race]
@@ -285,7 +285,7 @@ The symbols referred to in this section are declared in the file
 @c syslog() is implemented as a call to vsyslog().
 @deftypefun void syslog (int @var{facility_priority}, const char *@var{format}, @dots{})
 @standards{BSD, syslog.h}
-@safety{@prelim{}@mtsafe{@mtsenv{} @mtslocale{}}@asunsafe{@asucorrupt{} @ascuheap{} @asulock{} @ascudlopen{}}@acunsafe{@acucorrupt{} @aculock{} @acsmem{} @acsfd{}}}
+@safety{@prelim{}@mtunsafe{@mtasurace{:LogTag/LogMask}}@asunsafe{@asucorrupt{} @ascuheap{} @asulock{} @ascudlopen{}}@acunsafe{@acucorrupt{} @aculock{} @acsmem{} @acsfd{}}}
 @c syslog @mtsenv @mtslocale @asucorrupt @ascuheap @asulock @ascudlopen @acucorrupt @aculock @acsmem @acsfd
 @c  va_start dup ok
 @c  vsyslog_chk @mtsenv @mtslocale @asucorrupt @ascuheap @asulock @ascudlopen @acucorrupt @aculock @acsmem @acsfd
-- 
2.27.0


-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] Mark syslog as MT-unsafe (bug 26100)

[Florian Weimer]

* Andreas Schwab:

> The syslog functions have been marked MT-safe by mistake.  They use global
> variables in unsafe way.

Should we make syslog MT-safe instead?

Thanks,
Florian

Re: [PATCH] Mark syslog as MT-unsafe (bug 26100)

[Andreas Schwab]

On Jun 15 2020, Florian Weimer wrote:

> * Andreas Schwab:
>
>> The syslog functions have been marked MT-safe by mistake.  They use global
>> variables in unsafe way.
>
> Should we make syslog MT-safe instead?

Should we?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] Mark syslog as MT-unsafe (bug 26100)

[Florian Weimer]

* Andreas Schwab:

> On Jun 15 2020, Florian Weimer wrote:
>
>> * Andreas Schwab:
>>
>>> The syslog functions have been marked MT-safe by mistake.  They use global
>>> variables in unsafe way.
>>
>> Should we make syslog MT-safe instead?
>
> Should we?

If it's not too hard, I think it would be a reasonable enhancement.
Lack of thread safety is surprising here, and I expect that a lot of
code assumes it.

Thanks,
Florian

Re: [PATCH] Mark syslog as MT-unsafe (bug 26100)

[Andreas Schwab]

On Jun 15 2020, Florian Weimer wrote:

> If it's not too hard, I think it would be a reasonable enhancement.
> Lack of thread safety is surprising here, and I expect that a lot of
> code assumes it.

Its use of shared global state makes it inherently non-safe.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] Mark syslog as MT-unsafe (bug 26100)

[Zack Weinberg]

On Mon, Jun 15, 2020 at 8:31 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Jun 15 2020, Florian Weimer wrote:
>
> > If it's not too hard, I think it would be a reasonable enhancement.
> > Lack of thread safety is surprising here, and I expect that a lot of
> > code assumes it.
>
> Its use of shared global state makes it inherently non-safe.

Messages to syslog need to be serialized over the whole process
_anyway_, so I don't see a problem with having
syslog/openlog/closelog/setlogmask take an internal mutex around the
shared global state.  However, for further bulletproofing we would
need to promise that these functions never acquire any _other_ locks,
in particular that they do not call malloc, and I don't know whether
they do now.

If we're going to do this it should be clearly documented as a GNU
extension; POSIX does not guarantee this.

zw

Re: [PATCH] Mark syslog as MT-unsafe (bug 26100)

[Carlos O'Donell]

On 6/15/20 10:18 AM, Zack Weinberg wrote:
> On Mon, Jun 15, 2020 at 8:31 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
>> On Jun 15 2020, Florian Weimer wrote:
>>
>>> If it's not too hard, I think it would be a reasonable enhancement.
>>> Lack of thread safety is surprising here, and I expect that a lot of
>>> code assumes it.
>>
>> Its use of shared global state makes it inherently non-safe.
> 
> Messages to syslog need to be serialized over the whole process
> _anyway_, so I don't see a problem with having
> syslog/openlog/closelog/setlogmask take an internal mutex around the
> shared global state.  However, for further bulletproofing we would
> need to promise that these functions never acquire any _other_ locks,
> in particular that they do not call malloc, and I don't know whether
> they do now.
> 
> If we're going to do this it should be clearly documented as a GNU
> extension; POSIX does not guarantee this.

When we add new locks we also have to consider MT-fork issues and if
any existing semi-legitimate uses may be impacted by the locks taken
by these functions.

-- 
Cheers,
Carlos.

Re: [PATCH] Mark syslog as MT-unsafe (bug 26100)

[Zack Weinberg]

On Mon, Jun 15, 2020 at 2:07 PM Carlos O'Donell <carlos@redhat.com> wrote:
> On 6/15/20 10:18 AM, Zack Weinberg wrote:
> > Messages to syslog need to be serialized over the whole process
> > _anyway_, so I don't see a problem with having
> > syslog/openlog/closelog/setlogmask take an internal mutex around the
> > shared global state.
>
> When we add new locks we also have to consider MT-fork issues and if
> any existing semi-legitimate uses may be impacted by the locks taken
> by these functions.

Hmm.  I can think of two scenarios where that could be a problem.
First, if one thread is calling syslog at the same time as another
thread calls fork, so the syslog lock is locked in the child process,
and then the child tries to call syslog, it will deadlock.  This seems
like something that could easily happen and people would want it to
work, but we could deal with it with another internal fork handler.

Second, if fork is called from an async signal handler that
interrupted the guts of syslog, syslog will again be unusable in the
child; but in that case the child is required to use only AS-safe
functions, so maybe we can get away with it?

Can you think of any others?

zw

Re: [PATCH] Mark syslog as MT-unsafe (bug 26100)

[Carlos O'Donell]

On 6/15/20 2:51 PM, Zack Weinberg wrote:
> On Mon, Jun 15, 2020 at 2:07 PM Carlos O'Donell <carlos@redhat.com> wrote:
>> On 6/15/20 10:18 AM, Zack Weinberg wrote:
>>> Messages to syslog need to be serialized over the whole process
>>> _anyway_, so I don't see a problem with having
>>> syslog/openlog/closelog/setlogmask take an internal mutex around the
>>> shared global state.
>>
>> When we add new locks we also have to consider MT-fork issues and if
>> any existing semi-legitimate uses may be impacted by the locks taken
>> by these functions.
> 
> Hmm.  I can think of two scenarios where that could be a problem.
> First, if one thread is calling syslog at the same time as another
> thread calls fork, so the syslog lock is locked in the child process,
> and then the child tries to call syslog, it will deadlock.  This seems
> like something that could easily happen and people would want it to
> work, but we could deal with it with another internal fork handler.

Yes, this is the "seim-legitimate uses" I was thinking of, and in the
standard it says you should only call AS-safe functions e.g. "Consequently,
to avoid errors, the child process may only execute async-signal-safe
operations until such time as one of the exec functions is called."

However, looking at bugzilla, I see Florian discovered this was non-working:
Bug 19429 - syslog unusable after fork from a multi-threaded program
https://sourceware.org/bugzilla/show_bug.cgi?id=19429

This is not a bug, and given that it doesn't work today, I don't want
to suggest we make syslog AS-Safe. I was only suggesting we double check
there wasn't any latent expectations from already existing user code.

It seems that because of bug 19429 there can't really be any such
expectations.

> Second, if fork is called from an async signal handler that
> interrupted the guts of syslog, syslog will again be unusable in the
> child; but in that case the child is required to use only AS-safe
> functions, so maybe we can get away with it?

This is the same as the other case, an MT-fork'd child must always
use only AS-safe functions.
 
> Can you think of any others?

No, those are the cases I was thinking about. However, for me, the
fact that bug 19429 exists, settles it for me, we just need to fix
the MT-safe case.

-- 
Cheers,
Carlos.