Repository navigation
Fix races and dangling pointers in tl environment access - #2471
nikosavola wants to merge 2 commits into
Conversation
- the mutex was created lazily without synchronization - `set_env` handed `putenv` pointers into `std::string`s that were reassigned later; use `setenv` / `unsetenv` on POSIX
| #else | ||
| putenv (const_cast<char *> (s.c_str ())); | ||
| // setenv copies the value, so no dangling pointer in environ (unlike putenv) | ||
| setenv (name.c_str (), value.c_str (), 1); |
There was a problem hiding this comment.
Why dangling pointer? the string is kept in a static storage, so it can be referenced.
There was a problem hiding this comment.
The map keeps the string alive, yes. But the next set_env for the same name replaces the map entry. That frees the old string while environ still points to it, until putenv sets the new one. Other code like Qt or Python calls getenv without our lock, so it can read freed memory in that moment. The window is small, so "dangling pointer" was too strong. setenv copies the value, so the problem goes away and we do not need the map on POSIX.
| tl::MutexLocker env_locker (s_env_lock); | ||
| std::string get_env (const std::string &name, const std::string &def_value) | ||
| { | ||
| tl::MutexLocker env_locker (env_lock ()); |
There was a problem hiding this comment.
I don't see how does fixes this problem: "the mutex was created lazily without synchronization". Since "env_lock" is called before the MutexLocker is entered and since the Mutex is initialized when the function is entered the first time (without protection AFAIK), the effect should be exactly the same as before.
I don't recall why there isn't simply a static Mutex. Maybe static Mutexes did not work under some conditions or were not available in the past.
There was a problem hiding this comment.
Since C++11 a function-local static is created only once, even with many threads. The compiler adds a guard for it. gcc 4.8.5 does this too, I checked. With the old code, two threads can both see a null pointer and create two different mutexes. A plain static mutex would also work. I used a pointer so the mutex is never destroyed at exit. I can change it if you prefer.
set_envhandedputenvpointers intostd::strings that were reassigned later; usesetenv/unsetenvon POSIX