[PATCH 2/2] stdlib: Re-implement free (environ) compatibility kludge for setenv
Andreas K. Huettel
dilfridge@gentoo.org
Fri Jan 24 20:12:15 GMT 2025
Am Freitag, 24. Januar 2025, 11:22:41 Mitteleuropäische Normalzeit schrieb Florian Weimer:
> 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 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.
OK so I *think* I understand what you are doing here. In the intermediate
version the array was not at the starting point of a memory allocation
(but at the end of struct environ_array), meaning a free (outside libc)
on the address of the array led to havoc. Now the array is a separate
allocation with a pointer to it at the start of struct environ_array.
(See also notes below.)
This makes sense to me, but I'm not really good enough for a R-b, please
get that from someone else.
That said, we're reaching the point now where additional fix-ups are
very late.
* Has the adressed problem been observed anywhere else *except*
userhelper?
* Any other ideas to estimate possible impact?
* In case this has no wider impact, would you be OK with backporting the
fix after the release?
We can make the final decision on monday, but that's cutting it fine.
>
> 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.
>
> 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 \
> 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;
>
> - 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))
Checks not needed anymore since the new code does not allocate per-byte,
see three lines below.
> + /* 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;
Allocate new array with new_size elements, pointer in new_array
Error handling
> +
> + struct environ_array *target_array = malloc (sizeof (*target_array));
> + if (target_array == NULL)
> {
> - __set_errno (ENOMEM);
> + free (new_array);
> return NULL;
> }
Allocate new struct, error handling
>
> - /* 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;
Store pointer to new array in array field
> 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. */
> - result_environ = &target_array->array[0];
> + result_environ = target_array->array;
> }
> }
The value for the environ pointer is not the address of the first element of
the variable length array at the end of the struct anymore, but the pointer stored
in the array field.
>
> @@ -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 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. */
Move to end
> + /* 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;
Add pointer to array field
> size_t allocated; /* Number of allocated array elments. */
> - char *array[]; /* The actual environment array. */
> + struct environ_array *next; /* Previously used environment array. */
Remove variable length array, add next pointer
> };
>
> /* 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;
Return correct pointer
> }
>
> /* Counter for detecting concurrent modification in unsetenv.
--
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/20250124/11a057c7/attachment-0001.sig>
More information about the Libc-alpha
mailing list