MantisBT - CMake
View Issue Details
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.
Steps To Reproduce
Additional Information
TagsNo tags attached.
Relationships
Attached Files
Issue History
Date ModifiedUsernameFieldChange
2012-04-20 10:51Orion PoplawskiNew Issue
2012-04-20 11:27Brad KingAssigned To => Clinton Stimpson
2012-04-20 11:27Brad KingStatusnew => assigned
2012-04-20 11:30Brad KingNote Added: 0029268
2012-04-20 11:33Brad KingNote Added: 0029269
2012-04-20 20:29Clinton StimpsonNote Added: 0029289
2012-04-21 09:45Clinton StimpsonNote Added: 0029291
2012-04-21 11:40Clinton StimpsonNote Added: 0029292
2012-04-21 11:49Clinton StimpsonNote Added: 0029293
2012-04-21 11:50Clinton StimpsonAssigned ToClinton Stimpson => Brad King
2012-04-23 16:35Brad KingNote Added: 0029318
2012-04-24 11:17Brad KingNote Added: 0029325
2012-04-25 08:42Brad KingNote Added: 0029334
2012-04-25 09:56Clinton StimpsonNote Added: 0029340
2012-04-25 09:57Clinton StimpsonAssigned ToBrad King => Clinton Stimpson
2012-04-25 10:36Brad KingNote Added: 0029342
2012-04-26 08:57Brad KingNote Edited: 0029342bug_revision_view_page.php?bugnote_id=29342#r631
2012-04-30 09:19Brad KingNote Added: 0029369
2012-04-30 14:34Brad KingTarget Version => CMake 2.8.9
2012-04-30 15:15Clinton StimpsonNote Added: 0029373
2012-04-30 15:15Clinton StimpsonStatusassigned => resolved
2012-04-30 15:15Clinton StimpsonResolutionopen => fixed
2012-08-09 17:49David ColeFixed in Version => CMake 2.8.9
2013-01-09 10:55Robert MaynardNote Added: 0032025
2013-01-09 10:55Robert MaynardStatusresolved => closed

Notes
(0029268)
Brad King   
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   
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   
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   
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   
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   
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   
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   
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   
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   
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   
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   
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   
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   
2013-01-09 10:55   
Closing resolved issues that have not been updated in more than 4 months.