Repository navigation
shadow-core: replace the environment by swapping the pointer, not clearenv - #325
Merged
Merged
Conversation
…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.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two Alpine nightlies (26 and 27 September) crashed with
SIGSEGVinside 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 withclearenv(3)followed by onesetenv(3)per kept variable. In a test process other threads are reading the environment at the same moment (tempfileasks forTMPDIR), the C library takes no lock aroundenviron, and musl'ssetenvreallocates the array on each call, so a reader walking the previous array reads freed memory.Fix.
replace_environmentbuilds a fresh, NUL-terminated array of freshKEY=valuestrings and pointsenvironat 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 assignenviron, and every libcgetenv/setenvcopes with an array it did not allocate. The libc crate does not declareenvironfor 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
getenvand walkingenvironin a loop while the main thread replaces the environment 2000 times, run 20 times each way:clearenv+setenv(before)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 check2842 / 0 in the container; e2e 416 assertions, all passed; the PAM probe from #323 still sees the replaced environment and the fixedPATH.