Bug 15607 - Add threadsafe version of getenv()
Summary: Add threadsafe version of getenv()
Status: ASSIGNED
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: unspecified
: P2 enhancement
Target Milestone: ---
Assignee: Florian Weimer
URL:
Keywords:
: 13271 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-06-12 14:45 UTC by Bastien Nocera
Modified: 2024-07-25 19:50 UTC (History)
11 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 Bastien Nocera 2013-06-12 14:45:19 UTC
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
Comment 1 OndrejBilka 2013-06-12 14:55:05 UTC
As 2.18 is release close this will have to wait for 2.19
Comment 2 Jakub Jelinek 2013-06-12 14:58:21 UTC
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.
Comment 3 Bastien Nocera 2013-06-12 15:06:53 UTC
(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.
Comment 4 Rich Felker 2013-06-12 18:34:24 UTC
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.
Comment 5 Bastien Nocera 2013-06-13 08:06:43 UTC
(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?
Comment 6 Bastien Nocera 2013-06-13 09:06:19 UTC
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.
Comment 7 Ondrej Bilka 2013-10-14 18:51:50 UTC
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.
Comment 8 Jackie Rosen 2014-02-16 19:41:59 UTC Comment hidden (spam)
Comment 9 Colin 2015-05-14 01:22:43 UTC
Rust issue on this: https://github.com/rust-lang/rust/pull/24741

Also marking SUSPENDED because I feel like that represents reality.
Comment 10 Ondrej Bilka 2015-07-11 20:51:19 UTC
*** Bug 13271 has been marked as a duplicate of this bug. ***
Comment 11 Florian Weimer 2024-07-01 14:09:21 UTC
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.