stdlib: Support malloc-managed environ arrays for compatibility

Some applications set environ to a heap-allocated pointer, call
setenv (expecting it to call realloc), free environ, and then
restore the original environ pointer.  This breaks after
commit 7a61e7f557a97ab597d6fca5e2d1f13f65685c61 ("stdlib: Make
getenv thread-safe in more cases") because after the setenv call,
the environ pointer does not point to the start of a heap allocation.
Instead, setenv creates a separate allocation and changes environ
to point into that.  This means that the free call in the application
results in heap corruption.

The interim approach was more compatible with other libcs because
it does not assume that the incoming environ pointer is allocated
as if by malloc (if it was written by the application).  However,
it seems to be more important to stay compatible with previous
glibc version: assume the incoming pointer is heap allocated,
and preserve this property after setenv calls.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

1 files changed, 33 insertions, 33 deletions

diff --git a/stdlib/setenv.c b/stdlib/setenv.c
index 2a2eec9c98..c6dc9f7945 100644
--- a/stdlib/setenv.c
+++ b/stdlib/setenv.c
@@ -191,52 +191,52 @@ __add_to_environ (const char *name, const char *value, const char *combined,
 	ep[1] = NULL;
       else
 	{
-	  /* We cannot use __environ as is and need to copy over the
-	     __environ contents into an array managed via
-	     __environ_array_list.  */
-
-	  struct environ_array *target_array;
-	  if (__environ_array_list != NULL
-	      && required_size <= __environ_array_list->allocated)
-	    /* Existing array has enough room.  Contents is copied below.  */
-	    target_array = __environ_array_list;
-	  else
+	  /* We cannot use __environ as is and need a larger allocation.  */
+
+	  if (start_environ == __environ_startup
+	      || __environ_is_from_array_list (start_environ))
 	    {
-	      /* Allocate a new array.  */
-	      target_array = __environ_new_array (required_size);
+	      /* Allocate a new array, managed in the list.  */
+	      struct environ_array *target_array
+		= __environ_new_array (required_size);
 	      if (target_array == NULL)
 		{
 		  UNLOCK;
 		  return -1;
 		}
+	      result_environ = &target_array->array[0];
+
+	      /* Copy over the __environ array contents.  This code
+		 handles the case start_environ == ep == NULL, too.  */
+	      size_t i;
+	      for (i = 0; start_environ + i < ep; ++i)
+		/* Regular store because unless there has been direct
+		   manipulation of the environment, target_array is still
+		   a private copy.  */
+		result_environ[i] = atomic_load_relaxed (start_environ + i);
+	    }
+	  else
+	    {
+	      /* Otherwise the application installed its own pointer.
+		 Historically, this pointer was managed using realloc.
+		 Continue doing so.  This disables multi-threading
+		 support.  */
+	      result_environ = __libc_reallocarray (start_environ,
+						    required_size,
+						    sizeof (*result_environ));
+	      if (result_environ == NULL)
+		{
+		  UNLOCK;
+		  return -1;
+		}
 	    }
-
-	  /* Copy over the __environ array contents.  This forward
-	     copy slides backwards part of the array if __environ
-	     points into target_array->array.  This happens if an
-	     application makes an assignment like:
-
-	       environ = &environ[1];
-
-	     The forward copy avoids clobbering values that still
-	     needing copying.  This code handles the case
-	     start_environ == ep == NULL, too.  */
-	  size_t i;
-	  for (i = 0; start_environ + i < ep; ++i)
-	    /* Regular store because unless there has been direct
-	       manipulation of the environment, target_array is still
-	       a private copy.  */
-	    target_array->array[i] = atomic_load_relaxed (start_environ + i);
 
 	  /* This is the new place where we should add the element.  */
-	  ep = target_array->array + i;
+	  ep = result_environ + (required_size - 2);
 
 	  /* Add the null terminator in case there was a pointer there
 	     previously.  */
 	  ep[1] = NULL;
-
-	  /* And __environ should be repointed to our array.  */
-	  result_environ = &target_array->array[0];
 	}
     }