library: Comment on libtest's dicey internal soundness by workingjubilee · Pull Request #158711 · rust-lang/rust

workingjubilee · 2026-07-03T01:12:23Z

This is the sort of thing that technically violates safety constraints but the requirements for hitting it are "write a custom test harness and call into an env-inspecting part of libc", which seems specific enough that we have no real reason to "fix" it. However, a safety comment seems warranted.

This resolves #158602 cc @Manishearth

rustbot · 2026-07-03T01:12:28Z

r? @Darksonn

rustbot has assigned @Darksonn.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: libs
libs expanded to 12 candidates
Random selection from 6 candidates

ChrisDenton · 2026-07-03T01:38:52Z

+        // 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".


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

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

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.

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.

Mm, you are correct.

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.

Darksonn

Seems reasonable to me.

@bors r+

View changes since this review

rust-bors · 2026-07-03T11:30:43Z

📌 Commit 4e2363a has been approved by Darksonn

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 10. This pull request will be tested once the tree is reopened.

…sketchy, r=Darksonn library: Comment on libtest's dicey internal soundness This is the sort of thing that technically violates safety constraints but the requirements for hitting it are "write a custom test harness and call into an env-inspecting part of libc", which seems specific enough that we have no real reason to "fix" it. However, a safety comment seems warranted.

Rollup of 18 pull requests Successful merges: - #158692 (Add release notes for 1.96.1) - #134021 (Implement `IntoIterator` for `[&[mut]] Box<[T; N], A>`) - #152860 (Port the `without_debuginfo` test from `backtrace-rs` to the testsuite) - #155932 (MIR Call terminator: evaluate destination place before arguments) - #156777 (Add -Zautodiff_post_passes flag to limit which llvm passes to run after enzyme to make autodiff tests more robust) - #157151 (JSON target specs: remove 'x86-softfloat' compatibility alias) - #157835 (expand free alias types in the auto-trait orphan check) - #158377 (add `-Zforce-intrinsic-fallback` flag) - #158434 (delegation: refactor AST -> HIR lowering) - #158552 (make some tidy errors around python easier to understand) - #158624 (borrowck: Introduce BlameConstraint::to_obligation_cause_from_path()) - #158704 (Optimize `ArrayChunks::try_rfold` with `DoubleEndedIterator::next_chunk_back`) - #158711 (library: Comment on libtest's dicey internal soundness) - #158539 (Move `SizeHint` and `IoHandle` to `core::io`) - #158659 (refactor the normalization in `coerce_shared_info`) - #158689 (resolver: don't use `Finalize` when resolving visibilities during AST expansion) - #158698 (Update TypeVisitable implementation) - #158706 (Tweaks to MIR building scope API)

…sketchy, r=Darksonn library: Comment on libtest's dicey internal soundness This is the sort of thing that technically violates safety constraints but the requirements for hitting it are "write a custom test harness and call into an env-inspecting part of libc", which seems specific enough that we have no real reason to "fix" it. However, a safety comment seems warranted.

…uwer Rollup of 20 pull requests Successful merges: - #158692 (Add release notes for 1.96.1) - #134021 (Implement `IntoIterator` for `[&[mut]] Box<[T; N], A>`) - #155932 (MIR Call terminator: evaluate destination place before arguments) - #155989 (Update `transmute_copy` to ub_checks and `?Sized`) - #156777 (Add -Zautodiff_post_passes flag to limit which llvm passes to run after enzyme to make autodiff tests more robust) - #157151 (JSON target specs: remove 'x86-softfloat' compatibility alias) - #157835 (expand free alias types in the auto-trait orphan check) - #157857 (Stabilize `#[my_macro] mod foo;` (part of `proc_macro_hygiene`)) - #158377 (add `-Zforce-intrinsic-fallback` flag) - #158434 (delegation: refactor AST -> HIR lowering) - #158552 (make some tidy errors around python easier to understand) - #158624 (borrowck: Introduce BlameConstraint::to_obligation_cause_from_path()) - #158704 (Optimize `ArrayChunks::try_rfold` with `DoubleEndedIterator::next_chunk_back`) - #158711 (library: Comment on libtest's dicey internal soundness) - #158751 (rustdoc: Fix crash when trying to inline foreign item which cannot have attributes) - #158539 (Move `SizeHint` and `IoHandle` to `core::io`) - #158659 (refactor the normalization in `coerce_shared_info`) - #158689 (resolver: don't use `Finalize` when resolving visibilities during AST expansion) - #158698 (Update TypeVisitable implementation) - #158706 (Tweaks to MIR building scope API)

…uwer Rollup of 19 pull requests Successful merges: - #158692 (Add release notes for 1.96.1) - #134021 (Implement `IntoIterator` for `[&[mut]] Box<[T; N], A>`) - #155932 (MIR Call terminator: evaluate destination place before arguments) - #155989 (Update `transmute_copy` to ub_checks and `?Sized`) - #156777 (Add -Zautodiff_post_passes flag to limit which llvm passes to run after enzyme to make autodiff tests more robust) - #157151 (JSON target specs: remove 'x86-softfloat' compatibility alias) - #157835 (expand free alias types in the auto-trait orphan check) - #157857 (Stabilize `#[my_macro] mod foo;` (part of `proc_macro_hygiene`)) - #158434 (delegation: refactor AST -> HIR lowering) - #158552 (make some tidy errors around python easier to understand) - #158624 (borrowck: Introduce BlameConstraint::to_obligation_cause_from_path()) - #158704 (Optimize `ArrayChunks::try_rfold` with `DoubleEndedIterator::next_chunk_back`) - #158711 (library: Comment on libtest's dicey internal soundness) - #158751 (rustdoc: Fix crash when trying to inline foreign item which cannot have attributes) - #158539 (Move `SizeHint` and `IoHandle` to `core::io`) - #158659 (refactor the normalization in `coerce_shared_info`) - #158689 (resolver: don't use `Finalize` when resolving visibilities during AST expansion) - #158698 (Update TypeVisitable implementation) - #158706 (Tweaks to MIR building scope API)

Rollup merge of #158711 - workingjubilee:explain-libtest-is-sketchy, r=Darksonn library: Comment on libtest's dicey internal soundness This is the sort of thing that technically violates safety constraints but the requirements for hitting it are "write a custom test harness and call into an env-inspecting part of libc", which seems specific enough that we have no real reason to "fix" it. However, a safety comment seems warranted.

library: Comment on libtest's dicey internal soundness

4e2363a

rustbot assigned Darksonn Jul 3, 2026

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 3, 2026

ChrisDenton reviewed Jul 3, 2026

View reviewed changes

Manishearth approved these changes Jul 3, 2026

View reviewed changes

Darksonn approved these changes Jul 3, 2026

View reviewed changes

rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2026

jhpratt mentioned this pull request Jul 3, 2026

Rollup of 18 pull requests #158753

Closed

JonathanBrouwer mentioned this pull request Jul 5, 2026

Rollup of 20 pull requests #158798

Closed

JonathanBrouwer mentioned this pull request Jul 5, 2026

Rollup of 19 pull requests #158811

Merged

rust-bors Bot merged commit 6a55427 into rust-lang:main Jul 5, 2026
13 checks passed

rustbot added this to the 1.98.0 milestone Jul 5, 2026

theemathas modified the milestones: 1.98.0, 1.99.0 Jul 6, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

library: Comment on libtest's dicey internal soundness#158711
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
workingjubilee:explain-libtest-is-sketchy

workingjubilee commented Jul 3, 2026 •

edited

Loading

Uh oh!

rustbot commented Jul 3, 2026

Uh oh!

ChrisDenton Jul 3, 2026 •

edited by rustbot

Loading

Uh oh!

workingjubilee Jul 3, 2026

Uh oh!

workingjubilee Jul 3, 2026

Uh oh!

ChrisDenton Jul 3, 2026 •

edited

Loading

Uh oh!

workingjubilee Jul 3, 2026

Uh oh!

bjorn3 Jul 3, 2026

Uh oh!

Darksonn left a comment •

edited by rustbot

Loading

Uh oh!

rust-bors Bot commented Jul 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

		// 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".

Uh oh!

Uh oh!

Conversation

workingjubilee commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 3, 2026

Uh oh!

ChrisDenton Jul 3, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

ChrisDenton Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubilee Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

bjorn3 Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

Darksonn left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rust-bors Bot commented Jul 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

workingjubilee commented Jul 3, 2026 •

edited

Loading

ChrisDenton Jul 3, 2026 •

edited by rustbot

Loading

ChrisDenton Jul 3, 2026 •

edited

Loading

Darksonn left a comment •

edited by rustbot

Loading