Bug 15607 - Add threadsafe version of getenv()
Summary: Add threadsafe version of getenv()
Status: SUSPENDED
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: unspecified
: P2 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
: 13271 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-06-12 14:45 UTC by Bastien Nocera
Modified: 2015-07-11 20:51 UTC (History)
7 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. ***