GNU bug report logs - #62729
[PATCH] Fix dangling pointers in `environ'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 62729 in the body.
You can then email your comments to 62729 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Olivier Dion <olivier.dion <at> polymtl.ca>
To: bug-guile <at> gnu.org
Cc: Olivier Dion <olivier-dion <at> proton.me>
Subject: [PATCH] Fix dangling pointers in `environ'
Date: Sat,  8 Apr 2023 16:48:01 -0400

From: Olivier Dion <olivier-dion <at> proton.me>

When calling `environ', Guile set the global variable `environ' to a
list allocated with the GC.  Strings in it are also allocated with the
GC.

However, if an user call the Scheme setenv() procedure, the resulting
call to putenv() in libc might reallocate `environ' to a new pointer
while copying sub-pointers owned by Guile in it.

This results in the GC marking these strings for reclamation when they
are actually still present in `environ'.  Thus, the values in the
environment are now undefined.

To fix this, Guile should only manipulate the `environ' using the
standard libc functions.  This ensures that concurrent modification of
it is safe in multi-threaded program.  Therefore, the procedure
`environ' now call the libc clearenv() procedure to purge the
environment.  Then, the desired values are put in `environ' using
scm_putenv().  At the end, no GC allocated memory is put in `environ'.

Also, since `environ' can be changed at anytime in a multi-thread
program, emit a warning stipulating that the result is undefined
behavior if multiple threads are created in the program.  Consider for
example a thread iterating over `environ' while another one do a call to
putenv().  The latter would do a realloc() on `environ' and thus the old
array read by the former now contains garbage.

On system where clearenv() is not present, an atomic store of NULL with
sequential consistency to `environ' should be sufficient but see the
NOTES of clearenv(3).

* libguile/posix.c (scm_environ): Do not store GC allocated memory in
environ.
---
 libguile/posix.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/libguile/posix.c b/libguile/posix.c
index 3adc743c4..808a3f93b 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1736,11 +1736,28 @@ SCM_DEFINE (scm_environ, "environ", 0, 1, 0,
 	    "then the return value is unspecified.")
 #define FUNC_NAME s_scm_environ
 {
+  /* Accessing `environ' directly in a multi-threaded program is
+     undefined behavior since at anytime it could point to anything else
+     while reading it.  Not only that, but all accesses are protected by
+     an internal mutex of libc.  Thus, it is only truly safe to modify
+     the environment directly in a single-threaded program.  */
+  if (scm_ilength (scm_all_threads ()) != 1)
+    scm_display
+      (scm_from_latin1_string
+       ("warning: call to environ while multiple threads are running;\n"
+        "         further behavior unspecified.\n"),
+       scm_current_warning_port ());
+
   if (SCM_UNBNDP (env))
     return scm_makfromstrs (-1, environ);
   else
     {
-      environ = scm_i_allocate_string_pointers (env);
+      clearenv();
+      while (!scm_is_null (env))
+        {
+          scm_putenv (scm_car (env));
+          env = scm_cdr (env);
+        }
       return SCM_UNSPECIFIED;
     }
 }
-- 
2.39.2

Message #10 received at 62729-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Olivier Dion <olivier.dion <at> polymtl.ca>
Cc: 62729-done <at> debbugs.gnu.org, Olivier Dion <olivier-dion <at> proton.me>
Subject: Re: bug#62729: [PATCH] Fix dangling pointers in `environ'
Date: Sun, 16 Jul 2023 22:18:54 +0200

Hi Olivier,

Olivier Dion <olivier.dion <at> polymtl.ca> skribis:

> From: Olivier Dion <olivier-dion <at> proton.me>
>
> When calling `environ', Guile set the global variable `environ' to a
> list allocated with the GC.  Strings in it are also allocated with the
> GC.
>
> However, if an user call the Scheme setenv() procedure, the resulting
> call to putenv() in libc might reallocate `environ' to a new pointer
> while copying sub-pointers owned by Guile in it.
>
> This results in the GC marking these strings for reclamation when they
> are actually still present in `environ'.  Thus, the values in the
> environment are now undefined.
>
> To fix this, Guile should only manipulate the `environ' using the
> standard libc functions.  This ensures that concurrent modification of
> it is safe in multi-threaded program.  Therefore, the procedure
> `environ' now call the libc clearenv() procedure to purge the
> environment.  Then, the desired values are put in `environ' using
> scm_putenv().  At the end, no GC allocated memory is put in `environ'.
>
> Also, since `environ' can be changed at anytime in a multi-thread
> program, emit a warning stipulating that the result is undefined
> behavior if multiple threads are created in the program.  Consider for
> example a thread iterating over `environ' while another one do a call to
> putenv().  The latter would do a realloc() on `environ' and thus the old
> array read by the former now contains garbage.
>
> On system where clearenv() is not present, an atomic store of NULL with
> sequential consistency to `environ' should be sufficient but see the
> NOTES of clearenv(3).
>
> * libguile/posix.c (scm_environ): Do not store GC allocated memory in
> environ.

Thanks for the clear explanation and patch.  Finally applied with an
added comment in the code.

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.