Bug 13271 - getaddrinfo is not thread safe against concurrent setenv
Summary: getaddrinfo is not thread safe against concurrent setenv
Status: RESOLVED DUPLICATE of bug 15607
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: 2.14
: P2 normal
Target Milestone: ---
Assignee: Ulrich Drepper
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-07 08:26 UTC by Jan David Mol
Modified: 2015-07-11 20:51 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jan David Mol 2011-10-07 08:26:00 UTC
The getaddrinfo() can trigger the following sequence of function calls:

getaddrinfo()
gaih_inet()
__gethostbyname2_r()
_nss_dns_gethostbyname2_r()
_nss_dns_gethostbyname3_r()
__res_hostalias()
getenv("HOSTALIASES")

The getenv() function is not thread-safe however, so getaddrinfo() should not call it. I suspect other getenv() functions can be reached as well, but I lack enough glibc knowledge to provide a good list so I won't try.
Comment 1 Rich Felker 2011-10-07 12:28:26 UTC
Not a bug, as far as I can tell.

2.9.1 Thread-Safety

Since multi-threaded applications are not allowed to use the environ variable to access or modify any environment variable while any other thread is concurrently modifying any environment variable, any function dependent on any environment variable is not thread-safe if another thread is modifying the environment; see XSH exec.

And the cross-referenced text (from exec):

Conforming multi-threaded applications shall not use the environ variable to access or modify any environment variable while any other thread is concurrently modifying any environment variable. A call to any function dependent on any environment variable shall be considered a use of the environ variable to access that environment variable.

The only way the issue you reported *may* be a bug is in that getaddrinfo is not documented by the standard to use any environment variables; this is a behavior specific to most implementations including glibc. I'm not sure how cases like this should be treated, but I think as long as the implementation documents the use of the environment it's probably technically okay..
Comment 2 Ulrich Drepper 2011-10-07 14:43:02 UTC
As soon as multiple threads are in use a program must not modify the environment and therefore calls to getenv are safe.
Comment 3 Jan David Mol 2011-10-07 14:56:17 UTC
According to section 2.9.1 you quote, a program is still thread safe if it
writes environment variables, and thus allows setenv(), it just can't read them at the same time. Thus the issue arises that getaddrinfo is not thread-safe under some circumstances, i.e. when there is no concurrent use of setenv() and friends. This violates the POSIX requirements for getaddrinfo:

The freeaddrinfo() and getaddrinfo() functions shall be thread-safe.

I don't see any exceptions being noted or any access to environment variables
being mentioned?

@Ulrich: where does it state that concurrent threads cannot modify the environment at all? Section 2.9.1 seems to directly contradict that statement. 
In fact, the glibc documentation does not provide any hint towards the fact that getaddrinfo will clash with setenv, nor does it forbid the use of setenv in multithreaded programs as you seem to imply.
Comment 4 Rich Felker 2011-10-07 16:29:26 UTC
2.9.1 states very explicitly, "any function dependent on any
environment variable is not thread-safe if another thread is modifying the
environment". The only potential wiggle-room to claim that glibc is non-conformant is that getaddrinfo is not documented (by the standard or by the implementation) as depending on the environment.

With that said, I agree that Ulrich Drepper is wrong to claim that modifying the environment in a multi-threaded program is not allowed. It can be done safely in many situations, such as when all but one thread is purely computational or only invoking only async-signal-safe functions. It should also be perfectly safe if you modify extern char **environ; (or the array it points to) directly in only atomic ways and don't clobber any environment memory that other threads could still be referencing.
Comment 5 Jan David Mol 2011-10-07 17:49:35 UTC
The reason I report this is because I saw my application crash repeatedly in a validly used getaddrinfo, because another thread invalidated environ through setenv. I've since replaced the setenv call, but what worries me is that nothing in the documentation of glibc or POSIX could have indicated for me that this could happen.

Although I personally would consider a function only thread safe if no valid use can cause undefined behaviour due to multithreaded use, I do recognise that in this case practicality is important to consider. After all, dropping support for the environment variables used in getaddrinfo and other functions, or wrapping getenv with the same lock that setenv already has, might not be desirable either.

Since you are the devs and not me, it is up to you, but I kindly request that you reopen this bug as a documentation one at least. The setenv man page family could carry a warning indicating that it makes the behaviour of concurrently running thread-safe (glibc) functions undefined?
Comment 6 Rich Felker 2011-10-07 18:52:26 UTC
I think a reasonable solution that would definitely be conformant would be for getenv and functions documented *by the standard* as depending on the environment to remain as-is, and not perform any locking, but for getaddrinfo and any other functions that use the environment but aren't documented by the standard as doing so to obtain the environment lock before using getenv. Adding locking to getaddrinfo would be a tiny cost anyway in comparison to all the syscalls involved in performing a DNS lookup..
Comment 7 Jan David Mol 2011-10-12 08:21:10 UTC
Reopened; this ticket cannot be regarded as 'invalid', nor 'resolved', because, as mentioned earlier, POSIX and glibc docs do allow setenv and getaddrinfo to run concurrently.

Rich Felker's idea of wrapping all getenv calls inside glibc is good, assuming the number of functions needing access to that global lock is limited. Getenv itself needs only very weak (in fact, insanely weak) guarantees by POSIX, so indeed that doesn't need change.
Comment 8 Ulrich Drepper 2011-10-15 13:58:25 UTC
I've already explained that multi-threaded programs are not allowed to modify the environment.  Stop reopening and educate yourself.
Comment 10 Ulrich Drepper 2011-10-15 22:42:53 UTC
(In reply to comment #9)
> Please educate yourself:

Just go away, you really know nothing.  Guess who of the two of us is in the POSIX committee?
Comment 11 Rich Felker 2011-10-16 03:19:20 UTC
I'm tired of childish insults as a substitute for reading simple text. Changing status to WONTFIX.
Comment 12 Jackie Rosen 2014-02-16 17:43:43 UTC Comment hidden (spam)
Comment 13 Carlos O'Donell 2015-03-23 15:25:18 UTC
Reopening since this should be fixed.
Comment 14 Ondrej Bilka 2015-07-11 20:51:19 UTC
It couldn't be fixed in general as Rich pointed out that user could setup custom environment and deallocate it.

Anyway this is duplicate of generic request for threadsafe getenv. That should be fixed in generic way.

*** This bug has been marked as a duplicate of bug 15607 ***