[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.