Bug 4887 - Wordexp() should be thread safe.
Summary: Wordexp() should be thread safe.
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: manual (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Ulrich Drepper
URL:
Keywords:
: 5069 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-08-03 02:21 UTC by Takashi Nishiie
Modified: 2014-06-13 20:04 UTC (History)
4 users (show)

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


Attachments
Test Program (997 bytes, application/x-gzip)
2007-08-03 03:06 UTC, Takashi Nishiie
Details
Patch (2.05 KB, patch)
2007-08-03 03:07 UTC, Takashi Nishiie
Details | Diff
Test Program 2 (838 bytes, application/x-gzip)
2007-08-06 01:56 UTC, Takashi Nishiie
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Nishiie 2007-08-03 02:21:09 UTC
Wordexp() should be thread safe. 

1. Summary:

  wordexp() should be thread safe. 
  However, because wordexp() calls setenv()/unsetenv() internally,
wordexp() is not actually thread safe. 

2. hardware dependency:

  none.

3. Description of Problem:

  As IEEE Std 10003.1-20001 says, wordexp() should be thread 
  safe. See the following URL:
  http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_09.html

  However, when wordexp() is called simultaneously with another
  MT-safe functions which uses getenv() internally among functions
  defined by glibc, the following problems may occur:

  Problem No.1:
    getenv() may generate SIGSEGV.

  Problem No.2:
    getenv() sometimes fails to acquire the environment variable.

4. How reproducible:

  Always

5. Step to Reproduce:
  The following test program can reproduce the problems.
    tst-wordexp.tar.gz
  To make reproduction easy, this program executes wordexp()
  and getenv() at the same time by a lot of threads. 

  1) Extract the archive file.
     The following files are extracted. 
        Makefile
        tst-wordexp.c
  2) Compile the test program. 
        # make
  3) Run the test program.
        # ./tst-wordexp

     - If Problem No.1 occurs, the test program will be 
       terminated by segmentation fault.

     - If Problem  No.2 happens, the test program will 
       display the following messages. 
       Example:
         A00019 getenv V03077=NULL
       This message means getenv("V03077") returned NULL
       though the test program had defined V03077 as an 
       environment variable beforehand.

6. Actual Results:
     Example:
     # ./tst-setenv
     A00019 getenv V03077=NULL
     A00473 getenv V03699=NULL
     A00173 getenv V04256=NULL
     A00099 getenv V03367=NULL
     A00267 getenv V03899=NULL
     Segmentation fault
     #

7. Expected Resulst:
     # ./tst-setenv
     end
     #

8. Cause
  Glibc contains a lot of functions assumed to be thread safe
  which executes getenv(). These problems occur because 
  wordexp() indirectly executes setenv() and getenv() from 
  different threads at the same time. As for unsetenv(), 
  it is similar. 

  Problem No.1:
  - wordexp() may call setenv()
  - On adding a new environment variable, the memory block 
    bound to __environ is changed by realloc(). Hence calling
    getenv() simultaneously with setenv() may access freed 
    memory block and cause SIGSEGV.

  Problem No.2:
  - wordexp() may call unsetenv()
  - On calling unsetenv(), the contents in the __environ is 
    shifted one by one to delete an element. Hence calling 
    getenv() simultaneously with unsetenv() may skip existing
    element.

9. Solutions
  I propose a following patch.
    glibc-setenv-fj20070713.patch

  This patch solves the problems when setenv()/unsetenv()
  and getenv() are called at the same time. 

  Problem No.1:
  - This correction reduces frequency to acquire the memory 
    block bound to __environ. 
  - If the memory block bound to __environ is exhausted, new
    block is allocated but previous one is not released to 
    avoid SIGSEGV. In addition, the size of new block is 
    doubled to avoid aquiring memory on every environment 
    variable addition.

  Problem No.2:
  - This correction does not shift elements bounding to 
    __environ as much as possible.
  - Not to shift elements on unsetenv(), the element going 
    to be deleted is not deleted but overwritten by copy of
    other element instead.
Comment 1 Takashi Nishiie 2007-08-03 03:06:14 UTC
Created attachment 1942 [details]
Test Program
Comment 2 Takashi Nishiie 2007-08-03 03:07:13 UTC
Created attachment 1943 [details]
Patch
Comment 3 Ulrich Drepper 2007-08-03 04:33:27 UTC
Nonsense.  If you modify the environment everything can and will break.  You
simply cannot do it.
Comment 4 Takashi Nishiie 2007-08-03 08:45:46 UTC
I do not think that wordexp() is described clearly as 
MT-unsafe on the specification. 
However, isn't wordexp() actually thread safe?
Comment 5 Ulrich Drepper 2007-08-03 14:11:29 UTC
Stop reopneing.  wordexp is thread-safe.  setenv, putenv, clearenve, etc are not
allowed in  threaded code without killing the rest.  It's your fault if you rely
on these functions.
Comment 6 Takashi Nishiie 2007-08-06 01:56:15 UTC
Created attachment 1947 [details]
Test Program 2

Wordexp() is not thread safe. Because, wordexp() uses 
__setenv() and __unsetenv() internally.

# grep -n setenv ./posix/wordexp.c
857:  __unsetenv ("IFS");
1869:	  __setenv (env, value, 1);

Therefore, if a lot of wordexp() is executed at the same
 time at the same time, SIGSEGV can be generated. 
Example:tst-worexp2.tar.gz

Can you say that wordexp() is thread safe why ?
Comment 7 Takashi Nishiie 2007-08-29 06:45:51 UTC
  There is a mistake in the manual of libc.
  Ulrich Drepper said:
---
setenv, putenv, clearenve, etc are not allowed in  threaded
 code without killing the rest.
---

  The manual of GNU libc has the following description.
---
http://www.gnu.org/software/libc/manual/html_mono/libc.html#Environment-Access
25.4.1 Environment Access
The value of an environment variable can be accessed with
the getenv function. This is declared in the header file
stdlib.h. All of the following functions can be safely used
 in multi-threaded programs. It is made sure that concurrent
 modifications to the environment do not lead to errors.

char * getenv (const char *name)
...
int putenv (char *string)
...
int setenv (const char *name, const char *value, int replace)
...
int unsetenv (const char *name)
---

  If description of a manual is right, you should make 
functions like setenv(), putenv(), and clearenv() into thread-safe.
  You should correct description of the manual, if that is not right.
Comment 8 Ulrich Drepper 2007-09-20 18:33:26 UTC
(In reply to comment #7)
> You should [...]

Ever thought about doing anything yourself?  I have no intend to spend time
working on the manual.  Maybe somebody else will but if you want a change to
happen, submit a patch.

Comment 9 Takashi Nishiie 2007-09-26 00:41:03 UTC
(In reply to comment #8)
I propose a following patch.

  *putenv()/setenv()/clearenv()/unsetenv() are not thread safe.

diff -ruN manual.orig/startup.texi manual/startup.texi
--- manual.orig/startup.texi	2006-11-11 06:10:22.000000000 +0900
+++ manual/startup.texi	2007-09-25 16:26:36.500000000 +0900
@@ -309,9 +309,9 @@
 
 The value of an environment variable can be accessed with the
 @code{getenv} function.  This is declared in the header file
-@file{stdlib.h}.  All of the following functions can be safely used in
-multi-threaded programs.  It is made sure that concurrent modifications
-to the environment do not lead to errors.
+@file{stdlib.h}. Modifications of enviroment variables are not
+allowed in Multi-threaded programs. The @code{getenv} function
+can be safely used in multi-threaded programs.
 @pindex stdlib.h
 
 @comment stdlib.h
Comment 10 Andreas Jaeger 2007-10-27 16:29:09 UTC
*** Bug 5069 has been marked as a duplicate of this bug. ***
Comment 11 Ulrich Drepper 2007-10-28 01:54:57 UTC
Manual updated in trunk.
Comment 12 Florian Weimer 2014-06-13 20:04:59 UTC
POSIX 2008 removes the requirement for thread-safety in case the environment is modified explicitly.  The other unsetenv call is in the child and thus cannot race with threads in the parent (which are not allowed to change the environment due to the implicit read operation in wordexp).