Bug 25158

Summary: warning when using non-thread-safe functions
Product: glibc Reporter: Tom Tromey <tromey>
Component: nptlAssignee: Not yet assigned to anyone <unassigned>
Status: NEW ---    
Severity: normal CC: carlos, drepper.fsp, fweimer, tim.ruehsen
Priority: P2 Flags: fweimer: security-
Version: unspecified   
Target Milestone: ---   
Host: Target:
Build: Last reconfirmed:

Description Tom Tromey 2019-11-03 02:10:00 UTC
We're converting gdb to use threads, and while doing so we're
finding that gdb uses a variety of functions which are
not thread-safe, for example strtok.

I noticed that linking will give a warning when trying to
use some functions like mktemp, and I wondered it if would
be possible to extend this method to warn about the use of
non-thread-safe functions when -lpthread is used.  This way,
we could be sure that not only gdb but also supporting
libraries in the tree have been updated.
Comment 1 jsm-csl@polyomino.org.uk 2019-11-04 16:53:18 UTC
Given the proposal to merge libpthread into libc 
<https://sourceware.org/ml/libc-alpha/2019-10/msg00080.html>, I'm not sure 
"when -lpthread is used" is a good basis for warning.  (I don't know if 
putting a linker warning for a symbol in libpthread, when the symbol 
itself is in libc, would work, either.)

Note the proposal 
<http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2444.htm> to allow 
strtok state to be thread-local (as in Solaris) and so avoid problems that 
way.
Comment 2 Florian Weimer 2019-11-04 17:06:10 UTC
I'd prefer to make established uses thread-safe where the cost is not prohibitive and there is little risk of breaking programs. N2444 goes in that direction. For other interfaces (such as getpwuid), we will need some clarification from POSIX. 

In several cases, making data thread-local actually reduces RSS for programs that do not use these facilities because we currently allocate many pointers and locks in the data segment in case we need them.
Comment 3 Tim Rühsen 2019-11-19 15:50:19 UTC
A quick helper to find thread-unsafe functions can be found via https://blog.josefsson.org/2009/06/23/thread-safe-functions/
Comment 4 Tom Tromey 2019-11-19 16:15:38 UTC
That script is cool but it has:

UNSAFE="$UNSAFE dirname"

while the libc info page says

 -- Function: char * dirname (char *PATH)

     Preliminary: | MT-Safe | AS-Safe | AC-Safe | *Note POSIX Safety
     Concepts::.


It sure would be handy if this posix safety metadata were
available in some reusable form.
Comment 5 Florian Weimer 2019-11-19 16:20:49 UTC
The glibc manual annotations are not useful for use in a tool because they are based on the implementation, not the intended properties of the interface.

There is also an unresolved ambiguity regarding what thread safety means for function that updates objects whose address has been passed as parameters. In the past, some of functions which only operate on their arguments have been considered as not thread-safe. I don't think this particular approach to thread safety is useful.
Comment 6 Tim Rühsen 2019-11-19 17:18:14 UTC
Surely does it help, but needs some changes to not spill out too many false positives. Just found using strerror() and getenv() in libwget (supposed to be reentrant) library code.

It can also be extended to use ldd + nm to check linked dependencies.

But anyways, as I said, a helper for the short term.
Comment 7 Carlos O'Donell 2019-11-19 19:49:47 UTC
(In reply to Florian Weimer from comment #5)
> The glibc manual annotations are not useful for use in a tool because they
> are based on the implementation, not the intended properties of the
> interface.

At the time we wrote the annotations we wrote down what the implementation did, and knew that this wasn't exactly what we wanted, but consensus around exactly what we should do was not yet clear.

Thus the annotations are marked "Preliminary."

> There is also an unresolved ambiguity regarding what thread safety means for
> function that updates objects whose address has been passed as parameters.

Can you give an exmaple of such a function? Are we talking about APIs like strtok?

> In the past, some of functions which only operate on their arguments have
> been considered as not thread-safe. I don't think this particular approach
> to thread safety is useful.

Could you expand on this please?
Comment 8 Florian Weimer 2019-11-19 20:07:21 UTC
(In reply to Carlos O'Donell from comment #7)
>> There is also an unresolved ambiguity regarding what thread safety means for
>> function that updates objects whose address has been passed as parameters.
> 
> Can you give an exmaple of such a function? Are we talking about APIs like
> strtok?

I think “MT-Safe race:obstack-ptr” is such an example.

>> In the past, some of functions which only operate on their arguments have
>> been considered as not thread-safe. I don't think this particular approach
>> to thread safety is useful.
> 
> Could you expand on this please?

readdir vs readdir_r is the canonical example where things went really, really wrong.
Comment 9 Carlos O'Donell 2019-11-19 22:17:30 UTC
(In reply to Florian Weimer from comment #8)
> (In reply to Carlos O'Donell from comment #7)
> >> There is also an unresolved ambiguity regarding what thread safety means for
> >> function that updates objects whose address has been passed as parameters.
> > 
> > Can you give an exmaple of such a function? Are we talking about APIs like
> > strtok?
> 
> I think “MT-Safe race:obstack-ptr” is such an example.

Perfect. Good example. Where do you see the ambiguity?

Have you had a chance to read all of `info libc 'Conditionally Safe Features'`?

Particularly the "race" section?

Quoting from the manual:
~~~
     As for objects that are opaque or opaque-like, in that they are to
     be manipulated only by passing them to library functions (e.g.,
     ‘FILE’, ‘DIR’, ‘obstack’, ‘iconv_t’), there might be additional
     expectations as to internal coordination of access by the library.
     We will annotate, with ‘race’ followed by a colon and the argument
     name, functions that take such objects but that do not take care of
     synchronizing access to them by default.  For example, ‘FILE’
     stream ‘unlocked’ functions will be annotated, but those that
     perform implicit locking on ‘FILE’ streams by default will not,
     even though the implicit locking may be disabled on a per-stream
     basis.
~~~
obstack is not synchronized so we add race:obstack-ptr.

To some degree this is a concession to the legacy obstack ABI since it doesn't, AFAIK, have room to add a local mutex for protection (the type is exposed directly to users in obstack.h and is ABI).

However, in some cases there isn't an object to use. Consider two memcpy's from two different threads to the same memory. memcpy is *always* MT-safe, and the pointer to pointed memory cannot be meaningfully synchronized.

If we had a lock in obstack then we could just mark it MT-safe with no notation.

In summary
- obstack is MT-safe so long as you avoid data races with the objects in question, or following the POSIX model you synchronize memory accesses.

> >> In the past, some of functions which only operate on their arguments have
> >> been considered as not thread-safe. I don't think this particular approach
> >> to thread safety is useful.
> > 
> > Could you expand on this please?
> 
> readdir vs readdir_r is the canonical example where things went really,
> really wrong.

My opinion is that this is simply historical baggage and POSIX calls this out:
https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html#tag_22_02_09_09
~~~
Early proposals had provided "_r" versions for functions that returned pointers to variable-size buffers without providing a means for determining the required buffer size. This would have made using such functions exceedingly clumsy, potentially requiring iteratively calling them with increasingly larger guesses for the amount of storage required. Hence, sysconf() variables have been provided for such functions that return the maximum required buffer size.
~~~
This still isn't a great design. They call out later in that text that you should explicitly provide size information.

So POSIX added readdir_r, got the design wrong, and then we deprecated it.

Today readdir has internal locking, but the returned memory still comes from the DIR's buffer, so there is still synchronization issues accessing the returned data.

You could say "memory synchronization" covers this:
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_12
~~~
Applications shall ensure that access to any memory location by more than one thread of control (threads or processes) is restricted such that no thread of control can read or modify a memory location while another thread of control may be modifying it. Such access is restricted using functions that synchronize thread execution and also synchronize memory with respect to other threads.
~~~
In this case we know by the definition of the interface for readdir that we can't have multiple threads use the results of readdir without creating a data race.

See also:
"A.4.12 Memory Synchronization"
https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap04.html


The glibc manual doesn't say "MT-safe race:dirstream" because there is library synchronization provided. We do this even though the results returned to the other thread _may_ be inconsistent at this point, but that's a memory synchronization issue with the result of the function.

In summary
- readdir is MT-safe because POSIX says it shoudl be and we use locking.
- the results of readdir require memory synchronization to access otherwise you get a data race
- readdir_r is unsafe for many other historical reasons and so is deprecated.

We should probably continue this discussion on libc-alpha.