Bug 27585 - Unsafe modification to process environment
Summary: Unsafe modification to process environment
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.2.x
Hardware: All All
: medium normal
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard: threads
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-11 09:11 UTC by Remi Denis-Courmont
Modified: 2013-11-03 02:52 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Remi Denis-Courmont 2010-04-11 09:11:09 UTC
During initialization, libdbus calls putenv() in an attempt to clear two environment variables, DBUS_ACTIVATION_ADDRESS and DBUS_ACTIVATION_BUS_TYPE.
This occurs in the init_connections_unlocked() function.

As specified by POSIX, any modification to the environment variables is not thread-safe. In practice, it can corrupt the earlier results from getenv(). This can lead to crash if another thread tries to read a variable at the same time. (It could also crash if the thread that calls libdbus stores the result of getenv() first, then uses libdbus, then uses the result from getenv(), but maybe this is OK?).

Paradoxically, the init_connections_unlocked() is called with a lock. This does protect concurrent DBus initializations against one another. But this does not protect any other use of environment variables, whether in other libdbus code paths or in completely different components. So the use of putenv()/setenv()/unsetenv() seems to be a bug.

It is not clear to me why this is needed. But in any case, I assume using an extra static variable instead of putenv() would solve the problem.
Comment 1 Havoc Pennington 2010-07-19 07:27:05 UTC
The reason for this is that those two env variables apply only to the current process, and should not be exported down to any child processes.

I think it's bad practice in general to keep the result of getenv() without making a copy, since that would preclude anyone ever modifying the environment.

There's still a theoretical concurrency issue but perhaps starting a thread to init dbus while reading the environment simultaneously in another thread is just something to avoid. i.e. read the environment before, or after, dbus setup, and make a copy of the getenv'd value.

Another approach would be to do your getenv() prior to starting threads and save the values.

Another workaround would be to putenv() these two vars at the start of main() before other stuff runs, so the putnev in dbus would be a no-op

There isn't really a great solution that I see, since the libc interface just isn't threadsafe. But there are tons of workarounds.
Comment 2 Remi Denis-Courmont 2010-07-19 08:03:40 UTC
I wonder if it wouldn't be possible to store some flags in a static variable. libdbus could then remember it has already 'read' those two environment variables. If init_connections_unlocked() is executed once more, it can then pretend the variables don't exist. That avoids using putenv().

Agreeably, this won't propagate to child processes, won't survive dlclose() (which libdbus does not really support anyway). It also won't work if there is another dbus implementation running in the same process, but that seems unlikely.

Unfortunately, they're usage scenarios where you simply can't ensure that libdbus is initialized before pthread_create().
Comment 3 Havoc Pennington 2010-07-19 09:21:09 UTC
The only goal here is to keep those vars away from child processes, though. It's already OK within the same process (or easy to make OK, anyhow).

I'm not sure these vars are really used much, so maybe they could just never be set by the parent, or cleared here, but then again, I'm not sure this thread safety issue really happens much either ... it's all obscure situations.

Since multithreaded libdbus use is relatively rare I'd lean toward keeping things correct in single-threaded apps, and let threaded apps adapt or come up with a fix.
Comment 4 Simon McVittie 2013-08-27 16:37:19 UTC
(In reply to comment #2)
> Unfortunately, they're usage scenarios where you simply can't ensure that
> libdbus is initialized before pthread_create().

Then you can't safely use libdbus. Try GDBus?

(In reply to comment #3)
> then again, I'm not sure this
> thread safety issue really happens much either ... it's all obscure
> situations

Story of libdbus' life...
Comment 5 Simon McVittie 2013-10-08 12:19:04 UTC
(In reply to comment #1)
> The reason for this is that those two env variables apply only to the
> current process, and should not be exported down to any child processes.

True. On the other hand, we are in fact clearing the wrong environment variables anyway (see Bug #68308), so it can't have mattered that much. I'd be tempted to just delete this.
Comment 6 Chengwei Yang 2013-11-03 02:52:12 UTC
these putenv() were removed in #bug68308


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.