Merge remote-tracking branch 'up/main' into main-riscv64

Signed-off-by: David Abdurachmanov <davidlt@rivosinc.com>

glibc-environ-malloc.patch

@@ -1,182 +1,182 @@
 Author: Florian Weimer <fweimer@redhat.com>
-Date:   Wed Jan 22 12:11:39 2025 +0100
+Date:   Fri Jan 24 10:40:28 2025 +0100
 
-    stdlib: Support malloc-managed environ arrays for compatibility
+    stdlib: Re-implement free (environ) compatibility kludge for setenv
     
-    Some allocations 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.
+    For the originally failing application (userhelper from usermode),
+    it is not actually necessary to call realloc on the environ
+    pointer.  Yes, there will be a memory leak because the application
+    assigns a heap-allocated pointer to environ that it never frees,
+    but this leak was always there: the old realloc-based setenv had
+    a hidden internal variable, last_environ, that was used in a similar
+    way to __environ_array_list.  The application is not impacted by
+    the leak anyway because the relevant operations do not happen in
+    a loop.
     
-    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.
+    The change here just uses a separte heap allocation and points
+    environ to that.  This means that if an application calls
+    free (environ) and restores the environ pointer to the value
+    at process start, and does not modify the environment further,
+    nothing bad happens.
+    
+    This change should not invalidate any previous testing that went into
+    the original getenv thread safety change, commit 7a61e7f557a97ab597d6
+    ("stdlib: Make getenv thread-safe in more cases").
+    
+    The new test cases are modeled in part on the env -i use case from
+    bug 32588 (with !DO_MALLOC && !DO_EARLY_SETENV), and the previous
+    stdlib/tst-setenv-malloc test.  The DO_MALLOC && !DO_EARLY_SETENV
+    case in the new test should approximate what userhelper from the
+    usermode package does.
 
-diff --git a/csu/init-first.c b/csu/init-first.c
-index e35e4ce84f104683..0ad6f75dcdde4a33 100644
---- a/csu/init-first.c
-+++ b/csu/init-first.c
-@@ -61,6 +61,7 @@ _init_first (int argc, char **argv, char **envp)
-   __libc_argc = argc;
-   __libc_argv = argv;
-   __environ = envp;
-+  __environ_startup = envp;
- 
- #ifndef SHARED
-   /* First the initialization which normally would be done by the
-diff --git a/csu/libc-start.c b/csu/libc-start.c
-index 6f3d52e223d8f32d..4e15b6191dc809fa 100644
---- a/csu/libc-start.c
-+++ b/csu/libc-start.c
-@@ -244,6 +244,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
-   char **ev = &argv[argc + 1];
- 
-   __environ = ev;
-+  __environ_startup = ev;
- 
-   /* Store the lowest stack address.  This is done in ld.so if this is
-      the code for the DSO.  */
-diff --git a/include/unistd.h b/include/unistd.h
-index e241603b8131a9e9..ada957f9d04d272a 100644
---- a/include/unistd.h
-+++ b/include/unistd.h
-@@ -203,6 +203,9 @@ libc_hidden_proto (__tcsetpgrp)
- extern int __libc_enable_secure attribute_relro;
- rtld_hidden_proto (__libc_enable_secure)
- 
-+/* Original value of __environ.  Initialized by _init_first (dynamic)
-+   or __libc_start_main (static).  */
-+extern char **__environ_startup attribute_hidden;
- 
- /* Various internal function.  */
- extern void __libc_check_standard_fds (void) attribute_hidden;
-diff --git a/posix/environ.c b/posix/environ.c
-index a0ed0d80eab207f8..2430b47d8eee148c 100644
---- a/posix/environ.c
-+++ b/posix/environ.c
-@@ -10,3 +10,5 @@ weak_alias (__environ, environ)
- /* The SVR4 ABI says `_environ' will be the name to use
-    in case the user overrides the weak alias `environ'.  */
- weak_alias (__environ, _environ)
-+
-+char **__environ_startup;
 diff --git a/stdlib/Makefile b/stdlib/Makefile
-index a5fbc1a27e194656..ee95b2e79a2409b6 100644
+index a5fbc1a27e194656..c8b96b329e181d0a 100644
 --- a/stdlib/Makefile
 +++ b/stdlib/Makefile
-@@ -316,6 +316,7 @@ tests := \
-   tst-setcontext9 \
-   tst-setcontext10 \
-   tst-setcontext11 \
-+  tst-setenv-malloc \
-   tst-stdbit-Wconversion \
-   tst-stdbit-builtins \
-   tst-stdc_bit_ceil \
+@@ -277,6 +277,10 @@ tests := \
+   tst-concurrent-quick_exit \
+   tst-cxa_atexit \
+   tst-environ \
++  tst-environ-change-1 \
++  tst-environ-change-2 \
++  tst-environ-change-3 \
++  tst-environ-change-4 \
+   tst-getenv-signal \
+   tst-getenv-thread \
+   tst-getenv-unsetenv \
 diff --git a/stdlib/setenv.c b/stdlib/setenv.c
-index 2a2eec9c987923d5..c6dc9f7945aa52ba 100644
+index 2a2eec9c987923d5..0ef5dde37348a98f 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);
+@@ -118,24 +118,21 @@ __environ_new_array (size_t required_size)
+   else
+     new_size = __environ_array_list->allocated * 2;
  
- 	  /* This is the new place where we should add the element.  */
--	  ep = target_array->array + i;
-+	  ep = result_environ + (required_size - 2);
+-  size_t new_size_in_bytes;
+-  if (__builtin_mul_overflow (new_size, sizeof (char *),
+-			      &new_size_in_bytes)
+-      || __builtin_add_overflow (new_size_in_bytes,
+-				 offsetof (struct environ_array,
+-					   array),
+-				 &new_size_in_bytes))
++  /* Zero-initialize everything, so that getenv can only
++     observe valid or null pointers.  */
++  char **new_array = calloc (new_size, sizeof (*new_array));
++  if (new_array == NULL)
++    return NULL;
++
++  struct environ_array *target_array = malloc (sizeof (*target_array));
++  if (target_array == NULL)
+     {
+-      __set_errno (ENOMEM);
++      free (new_array);
+       return NULL;
+     }
  
- 	  /* Add the null terminator in case there was a pointer there
- 	     previously.  */
+-  /* Zero-initialize everything, so that getenv can only
+-     observe valid or null pointers.  */
+-  struct environ_array *target_array = calloc (1, new_size_in_bytes);
+-  if (target_array == NULL)
+-    return NULL;
+   target_array->allocated = new_size;
++  target_array->array = new_array;
+   assert (new_size >= target_array->allocated);
+ 
+   /* Put it onto the list.  */
+@@ -236,7 +233,7 @@ __add_to_environ (const char *name, const char *value, const char *combined,
  	  ep[1] = NULL;
--
--	  /* And __environ should be repointed to our array.  */
+ 
+ 	  /* And __environ should be repointed to our array.  */
 -	  result_environ = &target_array->array[0];
++	  result_environ = target_array->array;
  	}
      }
  
-diff --git a/stdlib/tst-setenv-malloc.c b/stdlib/tst-setenv-malloc.c
+@@ -403,6 +400,7 @@ __libc_setenv_freemem (void)
+   /* Clear all backing arrays.  */
+   while (__environ_array_list != NULL)
+     {
++      free (__environ_array_list->array);
+       void *ptr = __environ_array_list;
+       __environ_array_list = __environ_array_list->next;
+       free (ptr);
+diff --git a/stdlib/setenv.h b/stdlib/setenv.h
+index e4433f5f849eb3c4..7cbf9f2059f91da1 100644
+--- a/stdlib/setenv.h
++++ b/stdlib/setenv.h
+@@ -29,9 +29,18 @@
+    of environment values used before.  */
+ struct environ_array
+ {
+-  struct environ_array *next;   /* Previously used environment array.  */
++  /* The actual environment array.  Use a separate allocation (and not
++     a flexible array member) so that calls like free (environ) that
++     have been encountered in some applications do not crash
++     immediately.  With such a call, if the application restores the
++     original environ pointer at process start and does not modify the
++     environment again, a use-after-free situation only occurs during
++     __libc_freeres, which is only called during memory debugging.
++     With subsequent setenv calls, there is still heap corruption, but
++     that happened with the old realloc-based implementation, too.  */
++  char **array;
+   size_t allocated;             /* Number of allocated array elments.  */
+-  char *array[];               /* The actual environment array.  */
++  struct environ_array *next;   /* Previously used environment array.  */
+ };
+ 
+ /* After initialization, and until the user resets environ (perhaps by
+@@ -44,7 +53,7 @@ static inline bool
+ __environ_is_from_array_list (char **ep)
+ {
+   struct environ_array *eal = atomic_load_relaxed (&__environ_array_list);
+-  return eal != NULL && &eal->array[0] == ep;
++  return eal != NULL && eal->array == ep;
+ }
+ 
+ /* Counter for detecting concurrent modification in unsetenv.
+diff --git a/stdlib/tst-environ-change-1.c b/stdlib/tst-environ-change-1.c
 new file mode 100644
-index 0000000000000000..18a9d36842e67aa5
+index 0000000000000000..4241ad4c63ea2e33
 --- /dev/null
-+++ b/stdlib/tst-setenv-malloc.c
-@@ -0,0 +1,64 @@
-+/* Test using setenv with a malloc-allocated environ variable.
++++ b/stdlib/tst-environ-change-1.c
+@@ -0,0 +1,3 @@
++#define DO_EARLY_SETENV 0
++#define DO_MALLOC 0
++#include "tst-environ-change-skeleton.c"
+diff --git a/stdlib/tst-environ-change-2.c b/stdlib/tst-environ-change-2.c
+new file mode 100644
+index 0000000000000000..b20be124902125e8
+--- /dev/null
++++ b/stdlib/tst-environ-change-2.c
+@@ -0,0 +1,3 @@
++#define DO_EARLY_SETENV 0
++#define DO_MALLOC 1
++#include "tst-environ-change-skeleton.c"
+diff --git a/stdlib/tst-environ-change-3.c b/stdlib/tst-environ-change-3.c
+new file mode 100644
+index 0000000000000000..e77996a6cb0ac601
+--- /dev/null
++++ b/stdlib/tst-environ-change-3.c
+@@ -0,0 +1,3 @@
++#define DO_EARLY_SETENV 1
++#define DO_MALLOC 0
++#include "tst-environ-change-skeleton.c"
+diff --git a/stdlib/tst-environ-change-4.c b/stdlib/tst-environ-change-4.c
+new file mode 100644
+index 0000000000000000..633ef7bda84eb2a8
+--- /dev/null
++++ b/stdlib/tst-environ-change-4.c
+@@ -0,0 +1,3 @@
++#define DO_EARLY_SETENV 1
++#define DO_MALLOC 1
++#include "tst-environ-change-skeleton.c"
+diff --git a/stdlib/tst-environ-change-skeleton.c b/stdlib/tst-environ-change-skeleton.c
+new file mode 100644
+index 0000000000000000..2f064efe1ea938a9
+--- /dev/null
++++ b/stdlib/tst-environ-change-skeleton.c
+@@ -0,0 +1,121 @@
++/* Test deallocation of the environ pointer.
 +   Copyright (C) 2025 Free Software Foundation, Inc.
 +   This file is part of the GNU C Library.
 +
@@ -196,45 +196,102 @@ index 0000000000000000..18a9d36842e67aa5
 +
 +/* This test is not in the scope for POSIX or any other standard, but
 +   some applications assume that environ is a heap-allocated pointer
-+   after a call to setenv on an empty environment.  */
++   after a call to setenv on an empty environment.  They also try to
++   save and restore environ in an attempt to undo a temporary
++   modification of the environment array, but this does not work if
++   setenv was called before.
++
++   Before including this file, these macros need to be defined
++   to 0 or 1:
++
++   DO_EARLY_SETENV  If 1, perform a setenv call before changing environ.
++   DO_MALLOC        If 1, use a heap pointer for the empty environment.
++
++   Note that this test will produce errors under valgrind and other
++   memory tracers that call __libc_freeres because free (environ)
++   deallocates a pointer still used internally.  */
 +
 +#include <stdlib.h>
 +#include <unistd.h>
 +#include <support/check.h>
 +#include <support/support.h>
 +
-+static const char *original_path;
-+static char **save_environ;
-+
 +static void
-+rewrite_environ (void)
++check_rewritten (void)
 +{
-+  save_environ = environ;
-+  environ = xmalloc (sizeof (*environ));
-+  *environ = NULL;
-+  TEST_COMPARE (setenv ("A", "1", 1), 0);
-+  TEST_COMPARE (setenv ("B", "2", 1), 0);
-+  TEST_VERIFY (environ != save_environ);
-+  TEST_COMPARE_STRING (environ[0], "A=1");
-+  TEST_COMPARE_STRING (environ[1], "B=2");
++  TEST_COMPARE_STRING (environ[0], "tst_environ_change_a=1");
++  TEST_COMPARE_STRING (environ[1], "tst_environ_change_b=2");
 +  TEST_COMPARE_STRING (environ[2], NULL);
++  TEST_COMPARE_STRING (getenv ("tst_environ_change_a"), "1");
++  TEST_COMPARE_STRING (getenv ("tst_environ_change_b"), "2");
++  TEST_COMPARE_STRING (getenv ("tst_environ_change_early"), NULL);
 +  TEST_COMPARE_STRING (getenv ("PATH"), NULL);
-+  free (environ);
-+  environ = save_environ;
-+  TEST_COMPARE_STRING (getenv ("PATH"), original_path);
 +}
 +
 +static int
 +do_test (void)
 +{
-+  original_path = getenv ("PATH");
-+  rewrite_environ ();
++  TEST_COMPARE_STRING (getenv ("tst_environ_change_a"), NULL);
++  TEST_COMPARE_STRING (getenv ("tst_environ_change_b"), NULL);
++  TEST_COMPARE_STRING (getenv ("tst_environ_change_early_setenv"), NULL);
++#if DO_EARLY_SETENV
++  TEST_COMPARE (setenv ("tst_environ_change_early_setenv", "1", 1), 0);
++#else
++  /* Must come back after environ reset.  */
++  char *original_path = xstrdup (getenv ("PATH"));
++#endif
 +
-+  /* Test again after reallocated the environment due to an initial
-+     setenv call.  */
-+  TEST_COMPARE (setenv ("TST_SETENV_MALLOC", "1", 1), 0);
++  char **save_environ = environ;
++#if DO_MALLOC
++  environ = xmalloc (sizeof (*environ));
++#else
++  char *environ_array[1];
++  environ = environ_array;
++#endif
++  *environ = NULL;
++  TEST_COMPARE (setenv ("tst_environ_change_a", "1", 1), 0);
++  TEST_COMPARE (setenv ("tst_environ_change_b", "2", 1), 0);
++#if !DO_EARLY_SETENV
++  /* Early setenv results in reuse of the heap-allocated environ array
++     that does not change as more pointers are added to it.  */
 +  TEST_VERIFY (environ != save_environ);
-+  rewrite_environ ();
++#endif
++  check_rewritten ();
++
++  bool check_environ = true;
++#if DO_MALLOC
++  /* Disable further checks if the free call clobbers the environ
++     contents.  Whether that is the case depends on the internal
++     setenv allocation policy and the heap layout.  */
++  check_environ = environ != save_environ;
++  /* Invalid: Causes internal use-after-free condition.  Yet this has
++     to be supported for compatibility with some applications. */
++  free (environ);
++#endif
++
++  environ = save_environ;
++
++#if DO_EARLY_SETENV
++  /* With an early setenv, the internal environ array was overwritten.
++     Historically, this triggered a use-after-free problem because of
++     the use of realloc internally in setenv, but it may appear as if
++     the original environment had been restored.  In the current, we
++     can only support this if the free (environ) above call did not
++     clobber the array, otherwise getenv will see invalid pointers.
++     Due to the use-after-free, invalid pointers could be seen with
++     the old implementation as well, but the triggering conditions
++     were different.  */
++  if (check_environ)
++    {
++      check_rewritten ();
++      TEST_COMPARE_STRING (getenv ("PATH"), NULL);
++    }
++#else
++  TEST_VERIFY (check_environ);
++  TEST_COMPARE_STRING (getenv ("PATH"), original_path);
++  TEST_COMPARE_STRING (getenv ("tst_environ_change_a"), NULL);
++  TEST_COMPARE_STRING (getenv ("tst_environ_change_b"), NULL);
++#endif
 +
 +  return 0;
 +}

glibc.spec

@@ -152,7 +152,7 @@ Version: %{glibcversion}
 # - It allows using the Release number without the %%dist tag in the dependency
 #   generator to make the generated requires interchangeable between Rawhide
 #   and ELN (.elnYY < .fcXX).
-%global baserelease 33
+%global baserelease 34
 Release: %{baserelease}.0.riscv64%{?dist}
 
 # Licenses:
@@ -2389,9 +2389,12 @@ update_gconv_modules_cache ()
 %endif
 
 %changelog
-* Fri Jan 24 2025 David Abdurachmanov <davidlt@rivosinc.com> - 2.40.9000-33.0.riscv64
+* Sat Jan 25 2025 David Abdurachmanov <davidlt@rivosinc.com> - 2.40.9000-34.0.riscv64
 - Move lp64d symlink on riscv64 to filesystem
 
+* Fri Jan 24 2025 Florian Weimer <fweimer@redhat.com> - 2.40.9000-34
+- Avoid env -i crash due to free (environ) compatibility patch
+
 * Thu Jan 23 2025 Florian Weimer <fweimer@redhat.com> - 2.40.9000-33
 - Apply patch to improve compatibility with environ/malloc misuse