[PATCH] stdlib: Support malloc-managed environ arrays for compatibility
Andreas K. Huettel
dilfridge@gentoo.org
Wed Jan 22 14:01:46 GMT 2025
[...]
> 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.
>
> 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.
Makes sense. [Where did you find this, and how big is the impact?]
I guess you know who to poke for the review.
>
> 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;
>
> #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;
>
> /* 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;
>
> /* 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;
> 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 \
> 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
> {
> - /* 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;
> - else
> + 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;
> }
> - }
> -
> - /* 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];
>
> - 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. */
> + result_environ = __libc_reallocarray (start_environ,
> + required_size,
> + sizeof (*result_environ));
> + 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];
> }
> }
>
> 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));
> + *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[2], 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 again after reallocated the environment due to an initial
> + setenv call. */
> + TEST_COMPARE (setenv ("TST_SETENV_MALLOC", "1", 1), 0);
> + TEST_VERIFY (environ != save_environ);
> + rewrite_environ ();
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
>
> base-commit: d4626340b997d662ddca4a48f8a3bf03ccb7a603
>
>
--
Andreas K. Hüttel
dilfridge@gentoo.org
Gentoo Linux developer
(council, toolchain, base-system, perl, libreoffice)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 981 bytes
Desc: This is a digitally signed message part.
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20250122/f66081a9/attachment-0001.sig>
More information about the Libc-alpha
mailing list