[Bug libc/15607] New: Add threadsafe version of getenv()

[Bug libc/15607] New: Add threadsafe version of getenv()

[hadess at hadess dot net]

http://sourceware.org/bugzilla/show_bug.cgi?id=15607

            Bug ID: 15607
           Summary: Add threadsafe version of getenv()
           Product: glibc
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: hadess at hadess dot net
                CC: drepper.fsp at gmail dot com

gnome-settings-daemon is a daemon running in a GNOME session to apply stored
settings (such as the user's preferred locale) to the running session and its
own children (such as applications launched using the media-keys).

We started hitting SEGVs somewhat randomly recently:
https://bugzilla.gnome.org/show_bug.cgi?id=701322

which are caused by races between getenv() (as used by the glibc's gettext
implementation) and setenv() (called by gnome-settings-daemon to change its own
environment).

Having getenv_r()/setenv_r() would avoid this problem and make our
implementation more straight-forward and cleaner.

[1]: https://bug701322.bugzilla-attachments.gnome.org/attachment.cgi?id=245695

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[ondra at iuuk dot mff.cuni.cz]

http://sourceware.org/bugzilla/show_bug.cgi?id=15607

OndrejBilka <ondra at iuuk dot mff.cuni.cz> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ondra at iuuk dot mff.cuni.cz
           Severity|normal                      |enhancement

--- Comment #1 from OndrejBilka <ondra at iuuk dot mff.cuni.cz> ---
As 2.18 is release close this will have to wait for 2.19

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[jakub at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15607

--- Comment #2 from Jakub Jelinek <jakub at redhat dot com> ---
It is certainly not straightforward or cleaner, if all the uses of getenv in
glibc are converted to a version that requires locking, there will be a
significant cost to all apps out there, not just one particular that does
something like this.
The current behavior of setenv/putenv is clearly documented.
Why exactly do you want to change environment of the running multi-threaded
process, as opposed just making sure that upon exec the executed programs will
get the desired environment?
You can use execle or execvpe functions for that.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[hadess at hadess dot net]

http://sourceware.org/bugzilla/show_bug.cgi?id=15607

--- Comment #3 from Bastien Nocera <hadess at hadess dot net> ---
(In reply to Jakub Jelinek from comment #2)
> It is certainly not straightforward or cleaner, if all the uses of getenv in
> glibc are converted to a version that requires locking, there will be a
> significant cost to all apps out there, not just one particular that does
> something like this.
> The current behavior of setenv/putenv is clearly documented.

"The implementation of getenv() is not required to be reentrant."
and
"POSIX.1-2001 does not require setenv() or unsetenv() to be reentrant."

"does not require" vs. "does not require and glibc's implementation is not".

> Why exactly do you want to change environment of the running multi-threaded
> process, as opposed just making sure that upon exec the executed programs
> will get the desired environment?

This is very early in running gnome-settings-daemon (not after minutes, or even
seconds, it's the first thing we do), but we read the stored configuration
using GSettings, which uses D-Bus. That D-Bus implementation (the one in glib)
uses threads, thus with a single call to get the configuration value, we're
using threads.

> You can use execle or execvpe functions for that.

It still makes for pretty complicated and unfriendly code.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[bugdal at aerifal dot cx]

http://sourceware.org/bugzilla/show_bug.cgi?id=15607

Rich Felker <bugdal at aerifal dot cx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugdal at aerifal dot cx

--- Comment #4 from Rich Felker <bugdal at aerifal dot cx> ---
I agree with Jakub. Programs that are or may be (due to libraries that may use
threads) multi-threaded cannot modify their own environments. There's little
point in modifying your own environment anyway; for executing external
programs, the exec-family functions and posix_spawn both provide interfaces by
which you can specify your own environment for the new process image in a fully
safe manner.

Note that getenv_r could not protect against modifications to the environment
through extern char **environ, nor could setenv_r protect against reads via
environ. Thus, they could only have limited success in making environment
modification "thread-safe".

As far as I'm concerned, this bug report should be filed against
gnome-settings-daemon, not glibc, and it should be fixed by (preferably)
avoiding modification to the environment in the process itself and using the
appropriate exec-type functions, or by generating a completely new environment
and replacing extern char **environ atomically with a pointer to the new
environment.

Note that, if others do end up deeming it desirable to change glibc, the
appropriate change would be having setenv do the above-described atomic
replacement and simply leak the old environment. This would be fully safe with
no locking.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[hadess at hadess dot net]

http://sourceware.org/bugzilla/show_bug.cgi?id=15607

--- Comment #5 from Bastien Nocera <hadess at hadess dot net> ---
(In reply to Rich Felker from comment #4)
<snip>
> As far as I'm concerned, this bug report should be filed against
> gnome-settings-daemon,

The bug has already been reported and fixed against gnome-settings-daemon. Did
you follow the link?

> not glibc, and it should be fixed by (preferably)
> avoiding modification to the environment in the process itself and using the
> appropriate exec-type functions, or by generating a completely new
> environment and replacing extern char **environ atomically with a pointer to
> the new environment.

The problem is that even if we do that, the code sucks, and it's a huge amount
of code compared to what it could be. Do you have a better way to do this?

> Note that, if others do end up deeming it desirable to change glibc, the
> appropriate change would be having setenv do the above-described atomic
> replacement and simply leak the old environment. This would be fully safe
> with no locking.

Per-thread env?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[hadess at hadess dot net]

http://sourceware.org/bugzilla/show_bug.cgi?id=15607

--- Comment #6 from Bastien Nocera <hadess at hadess dot net> ---
I forgot to mention that we have half-a-dozen apps that have similar problems
in the typical desktop stack, including pkexec from PolicyKit, and gdm.

If at least glibc's gettext used getenv_r(), then it would certainly cause less
problems.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[neleai at seznam dot cz]

https://sourceware.org/bugzilla/show_bug.cgi?id=15607

Ondrej Bilka <neleai at seznam dot cz> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |neleai at seznam dot cz

--- Comment #7 from Ondrej Bilka <neleai at seznam dot cz> ---
A setenv does locking, you could try to send a patch that adds locking to
getenv. 

> Note that, if others do end up deeming it desirable to change glibc, the 
> appropriate change would be having setenv do the above-described atomic 
> replacement and simply leak the old environment.
> This would be fully safe with no locking.

A atomic part is already done. For leaking part you have problem with current
code:

 /* We allocated this space; we can extend it.  */
      new_environ = (char **) realloc (last_environ,
                                       (size + 2) * sizeof (char *));

which you would need to change to standard doubling of sizes.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[jackie.rosen at hushmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15607

Jackie Rosen <jackie.rosen at hushmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jackie.rosen at hushmail dot com

--- Comment #8 from Jackie Rosen <jackie.rosen at hushmail dot com> ---
*** Bug 260998 has been marked as a duplicate of this bug. ***
Seen from the domain http://volichat.com
Page where seen: http://volichat.com/adult-chat-rooms
Marked for reference. Resolved as fixed @bugzilla.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[schwab at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=15607

Andreas Schwab <schwab at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|jackie.rosen at hushmail dot com   |

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15607

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15607

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |security-

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[walters at verbum dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=15607

Colin <walters at verbum dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |SUSPENDED
                 CC|                            |walters at verbum dot org

--- Comment #9 from Colin <walters at verbum dot org> ---
Rust issue on this: https://github.com/rust-lang/rust/pull/24741

Also marking SUSPENDED because I feel like that represents reality.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[neleai at seznam dot cz]

https://sourceware.org/bugzilla/show_bug.cgi?id=15607

Ondrej Bilka <neleai at seznam dot cz> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mol at astron dot nl

--- Comment #10 from Ondrej Bilka <neleai at seznam dot cz> ---
*** Bug 13271 has been marked as a duplicate of this bug. ***

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[crrodriguez at opensuse dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=15607

Cristian Rodríguez <crrodriguez at opensuse dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |crrodriguez at opensuse dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[jwakely.gcc at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15607

Jonathan Wakely <jwakely.gcc at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jwakely.gcc at gmail dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15607

--- Comment #11 from Florian Weimer <fweimer at redhat dot com> ---
Patch posted:

[PATCH] stdlib: Make getenv thread-safe in more cases
<https://inbox.sourceware.org/libc-alpha/87le2od4xh.fsf@oldenburg.str.redhat.com/>

It does not add locking to getenv because that might be incompatible with
existing signal handlers and mallocs. Instead the patch implements the
exponential resizing of backing arrays, never deallocating them (as already
mentioned). The environment strings themselves are never deallocated in the
existing implementation if setenv is used. A special snapshotting technique is
used to avoid false negatives if unsetenv is called concurrently with getenv.

The patch as posted still misses special environ handling for execve.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15607

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|SUSPENDED                   |ASSIGNED
           Assignee|unassigned at sourceware dot org   |fweimer at redhat dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15607] Add threadsafe version of getenv()

[sam at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=15607

Sam James <sam at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sam at gentoo dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.