library: Comment on libtest's dicey internal soundness#158711
library: Comment on libtest's dicey internal soundness#158711workingjubilee wants to merge 1 commit into
Conversation
rustbot
added
S-waiting-on-review
|
r? @Darksonn rustbot has assigned @Darksonn. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| // 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". |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I mean, I suppose I am perhaps blithely assuming that libc is not pathologically interested in a particular variable that libtest has named.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Mm, you are correct.
There was a problem hiding this comment.
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.
rust-bors
Bot
added
S-waiting-on-bors
6 participants
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