[PATCH] stdlib: Support malloc-managed environ arrays for compatibility
Carlos O'Donell
carlos@redhat.com
Thu Jan 23 16:32:43 GMT 2025
On 1/22/25 7:48 AM, Florian Weimer wrote:
> 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.
Overall I think we should be moving towards being MT-safe for as much of the
APIs here as we can, and this is as step backwards, but for the sake of the
applications and being conservative this is the right choice for the release.
> 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.
Just for clarity, we've had downstream reports of failures that are this specific
case, and we want to conservatively fix this before the release. If we make
subsequent changes it will be with more consideration and review of the data.
LGTM.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> Tested on x86_64-linux-gnu.
>
> I'd prefer to get this into the upcoming
> release if possible, assuming that I can get a review in a timely
> fashion.
>
> Thanks,
> Florian
>
> ---
> csu/init-first.c | 1 +
> csu/libc-start.c | 1 +
> include/unistd.h | 3 +++
> posix/environ.c | 2 ++
> stdlib/Makefile | 1 +
> stdlib/setenv.c | 64 +++++++++++++++++++++++-----------------------
> stdlib/tst-setenv-malloc.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
> 7 files changed, 104 insertions(+), 32 deletions(-)
>
> diff --git a/csu/init-first.c b/csu/init-first.c
> index e35e4ce84f..0ad6f75dcd 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;
OK. Store the startup environment. No threads present.
>
> #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 6f3d52e223..4e15b6191d 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;
OK. Likewise. No threads present.
>
> /* 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 e241603b81..ada957f9d0 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;
OK.
> /* Various internal function. */
> extern void __libc_check_standard_fds (void) attribute_hidden;
> diff --git a/posix/environ.c b/posix/environ.c
> index a0ed0d80ea..2430b47d8e 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;
OK. Actual global.
> diff --git a/stdlib/Makefile b/stdlib/Makefile
> index a5fbc1a27e..ee95b2e79a 100644
> --- a/stdlib/Makefile
> +++ b/stdlib/Makefile
> @@ -316,6 +316,7 @@ tests := \
> tst-setcontext9 \
> tst-setcontext10 \
> tst-setcontext11 \
> + tst-setenv-malloc \
OK. Adds 1 test for the behaviour.
> tst-stdbit-Wconversion \
> tst-stdbit-builtins \
> tst-stdc_bit_ceil \
> 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
> {
OK. We are in the else clause of __add_to_environ's check to determine if we found
the variable. This means we did not find the variable and need to add it, requiring
a larger array.
> - /* We cannot use __environ as is and need to copy over the
> - __environ contents into an array managed via
> - __environ_array_list. */
> + /* We cannot use __environ as is and need a larger allocation. */
>
> - 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;
OK. We stop using the __environ_array_list.
> - else
> + if (start_environ == __environ_startup
> + || __environ_is_from_array_list (start_environ))
OK. If you either have the startup environ, or it is from the array list (managed by setenv).
> {
> - /* 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;
> }
> - }
> -
> - /* 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];
> + result_environ = &target_array->array[0];
OK. This path sets result_environ.
>
> - 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);
> + /* 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. */
OK. This is the new code, and the scenario where we have detected a changed environ.
> + result_environ = __libc_reallocarray (start_environ,
> + required_size,
OK. This path sets result_environ.
> + sizeof (*result_environ));
OK. We realloc the array to allow it to be freed by the caller later.
> + if (result_environ == NULL)
> + {
> + UNLOCK;
> + return -1;
> + }
> + }
>
> /* 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];
> }
> }
OK. And we're done changing the code to add the path where the environ has been written
to by the application.
>
> diff --git a/stdlib/tst-setenv-malloc.c b/stdlib/tst-setenv-malloc.c
> new file mode 100644
> index 0000000000..18a9d36842
> --- /dev/null
> +++ b/stdlib/tst-setenv-malloc.c
> @@ -0,0 +1,64 @@
> +/* Test using setenv with a malloc-allocated environ variable.
> + Copyright (C) 2025 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +/* 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. */
> +
> +#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)
> +{
> + save_environ = environ;
> + environ = xmalloc (sizeof (*environ));
OK. Updates environ directly.
> + *environ = NULL;
OK. Terminates empty environ.
> + TEST_COMPARE (setenv ("A", "1", 1), 0);
> + TEST_COMPARE (setenv ("B", "2", 1), 0);
OK. Two pointers passed to setenv, which the implementation now controls
and which setenv puts into the environ.
> + TEST_VERIFY (environ != save_environ);
OK. Verifies environ was not changed back by setenv.
> + TEST_COMPARE_STRING (environ[0], "A=1");
> + TEST_COMPARE_STRING (environ[1], "B=2");
> + TEST_COMPARE_STRING (environ[2], NULL);
> + TEST_COMPARE_STRING (getenv ("PATH"), NULL);
OK. Yes, A, and B are present, and PATH is not (because it was originally empty).
> + free (environ);
OK. So this assumes environ is the same as the originally set environment
after setenv was called and didn't change environ which is not true, but
is a compatibility issue we're trying to fix. Previous to this fix it
faults.
> + environ = save_environ;
OK.
> + TEST_COMPARE_STRING (getenv ("PATH"), original_path);
OK. Show that this works again because you have original path.
> +}
> +
> +static int
> +do_test (void)
> +{
> + original_path = getenv ("PATH");
OK. Expected set, since this is required from the shell.
> + rewrite_environ ();
OK. Rewrite the environment.
> +
> + /* Test again after reallocated the environment due to an initial
> + setenv call. */
> + TEST_COMPARE (setenv ("TST_SETENV_MALLOC", "1", 1), 0);
OK. setenv called.
> + TEST_VERIFY (environ != save_environ);
> + rewrite_environ ();
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
>
> base-commit: d4626340b997d662ddca4a48f8a3bf03ccb7a603
>
--
Cheers,
Carlos.
More information about the Libc-alpha
mailing list