Skip to content

shadow-core: replace the environment by swapping the pointer, not clearenv - #325

Merged
pierre-warnier merged 1 commit into
mainfrom
fix/environ-swap-not-clearenv
Sep 28, 2026
Merged

pierre-warnier merged 1 commit into
mainfrom
fix/environ-swap-not-clearenv

Conversation

@pierre-warnier

Copy link
Copy Markdown
Collaborator

Two Alpine nightlies (26 and 27 September) crashed with SIGSEGV inside unit tests: the integration test binary once, uu_passwd's lib tests once. Both binaries call a tool's entry point in-process from some tests, and since #323 that entry point replaces the process environment with clearenv(3) followed by one setenv(3) per kept variable. In a test process other threads are reading the environment at the same moment (tempfile asks for TMPDIR), the C library takes no lock around environ, and musl's setenv reallocates the array on each call, so a reader walking the previous array reads freed memory.

Fix. replace_environment builds a fresh, NUL-terminated array of fresh KEY=value strings and points environ at it: one aligned pointer store, nothing freed. The exec-time array and its strings stay where they are, unfreed (a few kilobytes, once per process). A concurrent reader sees the old array or the new one, both intact. POSIX allows a program to assign environ, and every libc getenv/setenv copes with an array it did not allocate. The libc crate does not declare environ for Linux, so the module declares it. What the tool ends up with is unchanged.

Reproduced and discriminated. A 60-line program on Alpine, 16 threads calling getenv and walking environ in a loop while the main thread replaces the environment 2000 times, run 20 times each way:

mechanism crashes / 20
clearenv + setenv (before) 19
pointer swap (this PR) 0

The real lib tests do not reproduce it on demand: 32 test threads, 120 runs on Alpine, 0 crashes on this branch and 0 on main's tree too. The window is narrow in the real suite (two hits in seven nightlies on the runners), which is why the reproducer above is the evidence, not the suite.

Gates: make check 2842 / 0 in the container; e2e 416 assertions, all passed; the PAM probe from #323 still sees the replaced environment and the fixed PATH.

…arenv

Two Alpine nightlies crashed with SIGSEGV inside unit tests: the
integration test binary once, uu_passwd's lib tests once. Both call a
tool's entry point in-process from some tests, and since the tool
replaces its environment first thing, that meant clearenv(3) followed
by one setenv(3) per kept variable in a process whose other threads
were reading the environment (tempfile asks for TMPDIR). The C library
takes no lock around environ, and musl's setenv reallocates the array
on each call, so a reader walking the previous array reads freed
memory.

replace_environment now builds a fresh NUL-terminated array of fresh
KEY=value strings and points environ at it: one aligned pointer store,
nothing freed. The exec-time array and its strings stay where they
are, unfreed -- a few kilobytes, once per process. A concurrent reader
sees the old array or the new one, both intact. POSIX allows a program
to assign environ, and every libc getenv/setenv copes with an array it
did not allocate; the libc crate does not declare environ for Linux,
so the module does. What the tool ends up with is unchanged.

Reproduced with a small program on Alpine: 16 threads calling getenv
and walking environ while the main thread replaces the environment
2000 times. clearenv+setenv crashed 19 runs of 20; the swap, 0 of 20.
The real lib tests do not reproduce it on demand (32 test threads, 120
runs, 0 crashes on either tree); the window is narrow there.

make check in the container: 2842 tests, 0 failed. e2e: 416
assertions, the PAM probe still sees the replaced environment and the
fixed PATH.
@pierre-warnier
pierre-warnier merged commit 55a881a into main Sep 28, 2026
13 checks passed
@pierre-warnier
pierre-warnier deleted the fix/environ-swap-not-clearenv branch September 28, 2026 13:16
@pierre-warnier pierre-warnier mentioned this pull request Oct 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant