View Issue Details [ Jump to Notes ] [ Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0013156CMakeQtDialogpublic2012-04-20 10:512013-01-09 10:55
ReporterOrion Poplawski 
Assigned ToClinton Stimpson 
PrioritynormalSeveritycrashReproducibilityhave not tried
StatusclosedResolutionfixed 
PlatformLinuxOSFedoraOS Version15
Product VersionCMake 2.8.4 
Target VersionCMake 2.8.9Fixed in VersionCMake 2.8.9 
Summary0013156: getenv() is not thread safe, leads to segfault
DescriptionSee the following Fedora bug report:

https://bugzilla.redhat.com/show_bug.cgi?id=814235 [^]

The upshot is that getenv()/setenv() are not thread safe and care must be taken not to call them in multiple threads.
TagsNo tags attached.
Attached Files

 Relationships

  Notes
(0029268)
Brad King (manager)
2012-04-20 11:30

 CMake modifies its own environment freely because it was written as a single-threaded program intentionally. This is still true for cmake, ccmake, ctest, and cpack.

At some point the Qt dialog became threaded. The cmake-gui application will have to deal with this. Threads other than the one used for configure/generate steps should not modify the environment.
(0029269)
Brad King (manager)
2012-04-20 11:33

 According to the downstream report comment here:

 https://bugzilla.redhat.com/show_bug.cgi?id=814235#c7 [^]

we cannot allow a thread to exit while the configure/generate thread might be modifying the environment.
(0029289)
Clinton Stimpson (developer)
2012-04-20 20:29

 The main thread waits for a couple seconds for the configure/generate thread to stop in response to an interrupt. If the thread doesn't finish, it proceeds to quit, because it doesn't want to leave the app hanging there. CMake 2.8.8 greatly improved the interrupt granularity, so its less likely this would happen.

But its easy to produce a hang with something like this
execute_process(COMMAND "cat")

How then should we stop it?

Also, as far as I know, the main thread doesn't use putenv() after the second thread starts.
(0029291)
Clinton Stimpson (developer)
2012-04-21 09:45

 we could compare with this.
http://lists.alioth.debian.org/pipermail/pkg-mozilla-maintainers/2011-June/009981.html [^]
(0029292)
Clinton Stimpson (developer)
2012-04-21 11:40

 Brad, I'm not sure if this is the same thing, but there is a bug in SystemTools.cxx when using putenv() to set environment variables.

To see it the bug, apply this patch

diff --git a/Source/cmake.cxx b/Source/cmake.cxx
index 846aef5..83f2907 100644
--- a/Source/cmake.cxx
+++ b/Source/cmake.cxx
@@ -39,6 +39,17 @@
 #include <cmsys/Glob.hxx>
 #include <cmsys/RegularExpression.hxx>
 
+struct dummy
+{
+ ~dummy()
+ {
+ getenv("A");
+ getenv("Z");
+ }
+};
+
+static dummy mydummy;
+


Then run the single threaded cmake on a CMakeLists.txt that has just this:
set(ENV{MYVAR} "asdfasf")
message("MYVAR=$ENV{MYVAR}")


valgrind reports this:

==26088== Invalid read of size 2
==26088== at 0x5809988: getenv (getenv.c:66)
==26088== by 0x67263D: dummy::~dummy() (cmake.cxx:46)
==26088== by 0x580A4F1: exit (exit.c:78)
==26088== by 0x57EFD94: (below main) (libc-start.c:258)
==26088== Address 0x5c48aa0 is 0 bytes inside a block of size 11 free'd
==26088== at 0x4C276FF: operator delete[](void*) (vg_replace_malloc.c:409)
==26088== by 0x8292E2: cmsys::kwsysDeletingCharVector::~kwsysDeletingCharVector() (SystemTools.cxx:390)
==26088== by 0x580A4F1: exit (exit.c:78)
==26088== by 0x57EFD94: (below main) (libc-start.c:258)
==26088==
==26088== Invalid read of size 2
==26088== at 0x5809988: getenv (getenv.c:66)
==26088== by 0x672647: dummy::~dummy() (cmake.cxx:47)
==26088== by 0x580A4F1: exit (exit.c:78)
==26088== by 0x57EFD94: (below main) (libc-start.c:258)
==26088== Address 0x5c48aa0 is 0 bytes inside a block of size 11 free'd
==26088== at 0x4C276FF: operator delete[](void*) (vg_replace_malloc.c:409)
==26088== by 0x8292E2: cmsys::kwsysDeletingCharVector::~kwsysDeletingCharVector() (SystemTools.cxx:390)
==26088== by 0x580A4F1: exit (exit.c:78)
==26088== by 0x57EFD94: (below main) (libc-start.c:258)

glibc takes ownership of the strings passed in putenv(). We should not clean those up ourselves.
(0029293)
Clinton Stimpson (developer)
2012-04-21 11:49

 If I fix it to do
bool SystemTools::PutEnv(const char* value)
{
  return putenv(strdup(value));
}

the memory error goes away, but valgrind reports a leak unless I use the --run-libc-freeres=no option.

Can we just change the code to use the preferred setenv() instead? And for Windows, use SetEnvironmentVariable() which is identical.
(0029318)
Brad King (manager)
2012-04-23 16:35

 Re 0013156:0029293: IIRC SystemTools::PutEnv came from a library that pre-dated CMake and at the time putenv was used because it was more portable. From the glibc man pages:

 Feature Test Macro Requirements for glibc (see feature_test_macros(7)):

  putenv(): _SVID_SOURCE || _XOPEN_SOURCE
  setenv(), unsetenv(): _BSD_SOURCE || _POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600
(0029325)
Brad King (manager)
2012-04-24 11:17

 Some history of SystemTools::PutEnv:

 http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=222e9a28 [^]
 http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=fdff8eb3 [^]
 http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=e7601ca6 [^]

The static destruction cleanup approach does not account for access by the system (or other static destruction) after deletion. However, glibc does not take ownership of the memory or have responsibility to free it. We should still free it. The problem is that kwsysDeletingCharVector frees memory while "environ" still points to it.

As noted in 0013156:0029318 setenv is not necessarily as portable as putenv so we may still need a putenv fallback implementation. Also SetEnvironmentVariable is not guaranteed to exist on Windows as it was introduced in WinXP.
(0029334)
Brad King (manager)
2012-04-25 08:42

 Re 0013156:0029325: On Windows the SetEnvironmentVariable API interacts with the process environment block but getenv reads the C runtime library's copy which is initialized on program startup from the process block and does not seem to be affected by SetEnvironmentVariable. See:

  http://msdn.microsoft.com/en-us/library/tehxacec.aspx [^]
  http://msdn.microsoft.com/en-us/library/stxk41x1.aspx [^]
  http://msdn.microsoft.com/en-us/library/windows/desktop/ms686206.aspx [^]
  http://msdn.microsoft.com/en-us/library/windows/desktop/ms683188.aspx [^]

The _putenv implementation appears to update both the C runtime copy and the process block.
(0029340)
Clinton Stimpson (developer)
2012-04-25 09:56

 Let me modify the code to wait indefinitely for the thread to join before proceeding with shutdown. This shouldn't really be a problem now that we have http://cmake.org/gitweb?p=cmake.git;a=commit;h=131eed. [^]
(0029342)
Brad King (manager)
2012-04-25 10:36
edited on: 2012-04-26 08:57

 Re 0013156:0029340: Thanks. That may address the immediate issue.

Meanwhile I may continue to investigate how complicated it is to set an environment variable portably and without leaking memory.

Re 0013156:0029334: The Windows SetEnvironmentVariable never touches the C runtime environment so getenv does not see the changes (but GetEnvironmentVariable does). Windows toolchains provide only putenv to modify the C runtime environment. Direct modification of the "environ" global is dangerous because of interaction with "wenviron" hidden behind getenv/putenv and wgetenv/wputenv.

Behavior of putenv("A=B") observed by experimentation:

  Linux (glibc) putenv references the original memory.
  Cygwin putenv references the original memory.
  MinGW putenv copies the memory.
  MS putenv copies the memory.
  Borland putenv references the original memory.

Behavior of putenv("A=") observed by experimentation:

  Linux (glibc) putenv STORES empty values, returns 0.
  Cygwin putenv STORES empty values, returns 0.
  MinGW putenv REMOVES empty values, returns 0.
  MS putenv REMOVES empty values, returns 0.
  Borland putenv REMOVES empty values, returns 0.

Behavior of putenv("A") observed by experimentation:

  Linux (glibc) putenv REMOVES the value, returns 0.
  Cygwin putenv REMOVES the value, returns 0.
  MinGW putenv IGNORES the call, returns -1.
  MS putenv IGNORES the call, returns -1.
  Borland putenv IGNORES the call, returns -1.
(0029369)
Brad King (manager)
2012-04-30 09:19

 This should solve the environment memory handling problems:

  http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=e48796b2 [^]
(0029373)
Clinton Stimpson (developer)
2012-04-30 15:15

 And this should fix the thread safety at shutdown.
http://cmake.org/gitweb?p=cmake.git;a=commit;h=2c050a [^]
(0032025)
Robert Maynard (manager)
2013-01-09 10:55

 Closing resolved issues that have not been updated in more than 4 months.

 Issue History
Date Modified Username Field Change
2012-04-20 10:51 Orion Poplawski New Issue
2012-04-20 11:27 Brad King Assigned To => Clinton Stimpson
2012-04-20 11:27 Brad King Status new => assigned
2012-04-20 11:30 Brad King Note Added: 0029268
2012-04-20 11:33 Brad King Note Added: 0029269
2012-04-20 20:29 Clinton Stimpson Note Added: 0029289
2012-04-21 09:45 Clinton Stimpson Note Added: 0029291
2012-04-21 11:40 Clinton Stimpson Note Added: 0029292
2012-04-21 11:49 Clinton Stimpson Note Added: 0029293
2012-04-21 11:50 Clinton Stimpson Assigned To Clinton Stimpson => Brad King
2012-04-23 16:35 Brad King Note Added: 0029318
2012-04-24 11:17 Brad King Note Added: 0029325
2012-04-25 08:42 Brad King Note Added: 0029334
2012-04-25 09:56 Clinton Stimpson Note Added: 0029340
2012-04-25 09:57 Clinton Stimpson Assigned To Brad King => Clinton Stimpson
2012-04-25 10:36 Brad King Note Added: 0029342
2012-04-26 08:57 Brad King Note Edited: 0029342
2012-04-30 09:19 Brad King Note Added: 0029369
2012-04-30 14:34 Brad King Target Version => CMake 2.8.9
2012-04-30 15:15 Clinton Stimpson Note Added: 0029373
2012-04-30 15:15 Clinton Stimpson Status assigned => resolved
2012-04-30 15:15 Clinton Stimpson Resolution open => fixed
2012-08-09 17:49 David Cole Fixed in Version => CMake 2.8.9
2013-01-09 10:55 Robert Maynard Note Added: 0032025
2013-01-09 10:55 Robert Maynard Status resolved => closed

Copyright © 2000 - 2018 MantisBT Team