Skip to content

library: Comment on libtest's dicey internal soundness #158711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rust-bors merged 1 commit into from
Jul 5, 2026
+9 −0
Merged

library: Comment on libtest's dicey internal soundness #158711

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions library/test/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,13 +207,22 @@ pub fn test_main_static_abort(tests: &[&TestDescAndFn]) {
// If we're being run in SpawnedSecondary mode, run the test here. run_test
// will then exit the process.
if let Ok(name) = env::var(SECONDARY_TEST_INVOKER_VAR) {
// SAFETY: Technically, this is a racy access that we probably shouldn't do?
// In practice, this is completely fine as long as the test harness is made of Rust,
// as std will synchronize the racy accesses that occur when they happen through std::env.
// Any unsoundness can only be exposed in practice if e.g. C code also takes an interest
// in these variables.
//
// If we ever grow an actual story for libtest and start documenting custom harness reqs,
// we should either fix this being racy or say "write it in Rust, please".
Comment on lines +216 to +217

@ChrisDenton ChrisDenton Jul 3, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is totally correct. It not only needs to be written in rust, it also needs to not call into libc. Which being single threaded will ensure but in a multithreaded application it is a hard condition to uphold because so many things in std call into libc.

View changes since the review

@workingjubilee workingjubilee Jul 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I suppose I am perhaps blithely assuming that libc is not pathologically interested in a particular variable that libtest has named.

@workingjubilee workingjubilee Jul 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason manipulating env vars is not sound generally and must be considered unsafe does include "because libc is interested in random variables", but that's because the safety constraint for a function like remove_var has to cover every possible env var's name string, especially including the ones that libc actually cares about.

@ChrisDenton ChrisDenton Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well remove_var modifies the environment which posix says must not be done in a multi-threaded application. The specific variable doesn't matter. To quote the docs:

The exact requirement is: you must ensure that there are no other threads concurrently writing or reading(!) the environment through functions or global variables other than the ones in this module.

@workingjubilee workingjubilee Jul 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm, you are correct.

@bjorn3 bjorn3 Jul 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another reason this set_var is fine in practice is that test_main_static_abort should be called directly by the main function without running anything else. Code running before the test_main_static_abort call would run once for each individual test.

unsafe {
env::remove_var(SECONDARY_TEST_INVOKER_VAR);
}

// Convert benchmarks to tests if we're not benchmarking.
let mut tests = tests.iter().map(make_owned_test).collect::<Vec<_>>();
if env::var(SECONDARY_TEST_BENCH_BENCHMARKS_VAR).is_ok() {
// SAFETY: Same as for SECONDARY_TEST_INVOKER_VAR
unsafe {
env::remove_var(SECONDARY_TEST_BENCH_BENCHMARKS_VAR);
}
Expand Down
Loading