[PATCH 2/2] stdlib: Re-implement free (environ) compatibility kludge for setenv
Carlos O'Donell
carlos@redhat.com
Fri Jan 24 21:10:53 GMT 2025
On 1/24/25 5:22 AM, Florian Weimer wrote:
> 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.
Agreed, there is a leak, but the leak should not be unbounded.
> 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.
Agreed. During the original review I considered what would happen if someone
set environ to data that was not heap allocated, but I hoped that wasn't
observed behaviour. Seems like I was wrong, and we have all possible observed
behaviours happening in userspace.
>
> 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").
Agreed, particularly for cases that don't alter environ.
> 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.
I've made two suggested changes:
- The comment that is incomplete.
- And a superfluous check in the test that can be simplified.
The change itself looks correct to me and continues to maximize backwards compat.
You can keep my RB if you make only the related test skeleton changes.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> Tested on x86_64-linux-gnu, and with an unpatched userhelper binary from
> usermode.
>
> ---
> stdlib/Makefile | 4 +
> stdlib/setenv.c | 26 +++---
> stdlib/setenv.h | 15 +++-
> stdlib/tst-environ-change-1.c | 3 +
> stdlib/tst-environ-change-2.c | 3 +
> stdlib/tst-environ-change-3.c | 3 +
> stdlib/tst-environ-change-4.c | 3 +
> stdlib/tst-environ-change-skeleton.c | 121 +++++++++++++++++++++++++++
> 8 files changed, 161 insertions(+), 17 deletions(-)
> create mode 100644 stdlib/tst-environ-change-1.c
> create mode 100644 stdlib/tst-environ-change-2.c
> create mode 100644 stdlib/tst-environ-change-3.c
> create mode 100644 stdlib/tst-environ-change-4.c
> create mode 100644 stdlib/tst-environ-change-skeleton.c
>
> diff --git a/stdlib/Makefile b/stdlib/Makefile
> index a5fbc1a27e..c8b96b329e 100644
> --- a/stdlib/Makefile
> +++ b/stdlib/Makefile
> @@ -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 \
OK. 4 new tests.
> tst-getenv-signal \
> tst-getenv-thread \
> tst-getenv-unsetenv \
> diff --git a/stdlib/setenv.c b/stdlib/setenv.c
> index 2a2eec9c98..0ef5dde373 100644
> --- a/stdlib/setenv.c
> +++ b/stdlib/setenv.c
> @@ -118,24 +118,21 @@ __environ_new_array (size_t required_size)
> else
> new_size = __environ_array_list->allocated * 2;
OK. Just after we enter __environ_new_array and compute the needed new_size.
> - 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))
OK. We don't compute new_size_in_bytes (no need).
> + /* Zero-initialize everything, so that getenv can only
> + observe valid or null pointers. */
> + char **new_array = calloc (new_size, sizeof (*new_array));
OK. Zero initialized. The array is of the size of new_array which is {sizeof elements x new_size}.
> + if (new_array == NULL)
> + return NULL;
OK. If calloc fails it sets errno to ENOMEM, and we return NULL (like other cases).
> +
> + struct environ_array *target_array = malloc (sizeof (*target_array));
OK. We need a new environ_array for tracking, but the target array is malloc'd and distinct
(for maximum compatibility).
> + if (target_array == NULL)
> {
> - __set_errno (ENOMEM);
> + free (new_array);
> return NULL;
OK.
> }
>
> - /* 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;
OK. Moved earlier.
> target_array->allocated = new_size;
> + target_array->array = new_array;
OK. The target_array array is now malloc'd (and can be independently freed for maximum
backwards compatibility).
> assert (new_size >= target_array->allocated);
>
> /* Put it onto the list. */
OK. At this point we're doing the book keeping to have a distinct target array that
can be freed.
> @@ -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. */
> - result_environ = &target_array->array[0];
> + result_environ = target_array->array;
OK. Yes, the target_array is now directly something we can put into result_environ.
> }
> }
>
> @@ -403,6 +400,7 @@ __libc_setenv_freemem (void)
> /* Clear all backing arrays. */
> while (__environ_array_list != NULL)
> {
> + free (__environ_array_list->array);
OK. Free the internal arrays we have allocated distinctly.
> 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 e4433f5f84..7cbf9f2059 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.
OK. Correct, and that will detect the uses and hopefully cause the developers to fix the
problem in the application.
> + With subsequent setenv calls, there is still heap corruption, but
> + that happened with the old realloc-based implementation, too. */
OK. You write that there is still heap corruption here, but I don't see why there should be
if the APIs are used correctly. I expect this text is about doing:
free (environ)
...
setenv (...)
Which is to say that setenv() is not made safe with this change. Though in the future it
could be made safe by always swapping in a new environ from a known good list, but then
that risks the regression when the application calls "free (environ)" on that, and we don't
know when that is going to happen.
In summary: This comment is OK.
> + char **array;
> size_t allocated; /* Number of allocated array elments. */
> - char *array[]; /* The actual environment array. */
> + struct environ_array *next; /* Previously used environment array. */
OK. Move environ_array to the end.
> };
>
> /* 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;
OK. Refactor after cleanup.
> }
>
> /* 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 0000000000..4241ad4c63
> --- /dev/null
> +++ 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"
OK.
> diff --git a/stdlib/tst-environ-change-2.c b/stdlib/tst-environ-change-2.c
> new file mode 100644
> index 0000000000..b20be12490
> --- /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"
OK.
> diff --git a/stdlib/tst-environ-change-3.c b/stdlib/tst-environ-change-3.c
> new file mode 100644
> index 0000000000..e77996a6cb
> --- /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"
OK.
> diff --git a/stdlib/tst-environ-change-4.c b/stdlib/tst-environ-change-4.c
> new file mode 100644
> index 0000000000..633ef7bda8
> --- /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"
OK.
> diff --git a/stdlib/tst-environ-change-skeleton.c b/stdlib/tst-environ-change-skeleton.c
> new file mode 100644
> index 0000000000..2f064efe1e
> --- /dev/null
> +++ b/stdlib/tst-environ-change-skeleton.c
> @@ -0,0 +1,121 @@
> +/* Test deallocation of the environ pointer.
OK.
> + 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. 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.
OK.
> +
> + 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 void
> +check_rewritten (void)
> +{
> + 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);
OK. Check a, and b are changed, but otherwise the other two are NULL.
> +}
> +
> +static int
> +do_test (void)
> +{
> + 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);
OK. Empty of the N=V pairs we're going to use.
> +#if DO_EARLY_SETENV
> + TEST_COMPARE (setenv ("tst_environ_change_early_setenv", "1", 1), 0);
OK. In the D_E_S case we call setenv which triggers the environ to add this entry, and may
change environ from the original value seen at startup.
> +#else
> + /* Must come back after environ reset. */
> + char *original_path = xstrdup (getenv ("PATH"));
OK. Get the PATH entry and store the pointer. We want to see that same pointer later
because we are exposing internal details.
> +#endif
> +
> + char **save_environ = environ;
OK. Save a copy of the environ.
> +#if DO_MALLOC
> + environ = xmalloc (sizeof (*environ));
> +#else
> + char *environ_array[1];
> + environ = environ_array;
> +#endif
OK. Straight forward one of two cases, malloc'd or on the heap (could have been .bss or .data too).
> + *environ = NULL;
> + TEST_COMPARE (setenv ("tst_environ_change_a", "1", 1), 0);
> + TEST_COMPARE (setenv ("tst_environ_change_b", "2", 1), 0);
OK. Set the two N=V pairs.
> +#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);
OK. In the non-D_E_S case we will change the environ here as we add entries.
> +#endif
> + check_rewritten ();
OK. Check that what we expect is in the array is in the array (checks environ,
and via API calls).
> +
> + 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;
OK. In the D_M case we may or may not have called setenv early, so we may or may
not have a reused environment.
> + /* Invalid: Causes internal use-after-free condition. Yet this has
> + to be supported for compatibility with some applications. */
> + free (environ);
OK. Yes, some applications do this and then set environ back to the original value.
> +#endif
> +
> + environ = save_environ;
OK. As expected.
> +
> +#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
Incomplete sentence? Do you mean "In the current code, 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);
TEST_COMPARE_STRING is superfluous because we do it in check_rewritten() as the last step?
This could be cleaned up:
e.g.
if (check_environ)
check_rewritten();
> + }
> +#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);
OK. Original values are back and not clobbered.
> +#endif
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
--
Cheers,
Carlos.
More information about the Libc-alpha
mailing list