Fix UB in `std::sys::os::getenv()` #114968

ShE3py · 2023-08-18T13:31:43Z

Reduced the loops to 1k iterations (100k was taking way too long), Miri no longer shows any UB.

@rustbot label +A-process +C-bug +I-unsound +O-unix

rustbot · 2023-08-18T13:31:51Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thomcc (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Nekrolm · 2023-08-18T13:44:09Z

Same changes seems also needed for https://github.com/rust-lang/rust/blob/master/library/std/src/sys/solid/os.rs#L173
and https://github.com/rust-lang/rust/blob/master/library/std/src/sys/wasi/os.rs#L230

there are same patterns

ShE3py · 2023-08-18T14:18:20Z

Same changes seems also needed for https://github.com/rust-lang/rust/blob/master/library/std/src/sys/solid/os.rs#L173 and https://github.com/rust-lang/rust/blob/master/library/std/src/sys/wasi/os.rs#L230

there are same patterns

Thanks for pointing out, I also copied env_read_lock() to Solid as I guess it too should ignore poisoning (furthermore the result was ignored, resulting in UB as _guard was a Result<Guard, NotAGuard>).

EDIT: Same problem with let _guard = ENV_LOCK.write();, imma fix that.

thomcc · 2023-08-18T14:37:09Z

Wasn't the Err case of the guard something that held a nested guard?

thomcc · 2023-08-18T14:37:44Z

library/std/src/sys/solid/os.rs

@@ -81,6 +81,10 @@ pub fn current_exe() -> io::Result<PathBuf> {

 static ENV_LOCK: RwLock<()> = RwLock::new(());

+pub fn env_read_lock() -> impl Drop {
+    ENV_LOCK.read().unwrap_or_else(PoisonError::into_inner)


This is fine, but isn't actually needed.

thomcc · 2023-08-18T14:48:19Z

@bors r+

bors · 2023-08-18T14:48:21Z

📌 Commit 83c713b has been approved by thomcc

It is now in the queue for this repository.

ShE3py · 2023-08-18T14:52:53Z

Wasn't the Err case of the guard something that held a nested guard?

pub type LockResult<Guard> = Result<Guard, PoisonError<Guard>>;

pub struct PoisonError<T> {
    guard: T,
}

You're right, the lock is acquired regardless of the variant, so the result can be used as-is.

klensy · 2023-08-18T15:45:54Z

Topic mentioned some loop iterations, but test didn't added here?

ShE3py · 2023-08-18T17:16:10Z

I couldn't find any text or code (to copy from) to make a Miri test, but if you have one, I can add a test.

bors · 2023-08-18T17:59:45Z

⌛ Testing commit 83c713b with merge d740f54773594676ef4bb6d1cdac26c968323494...

rust-log-analyzer · 2023-08-18T19:02:57Z

The job i686-mingw failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

failures:

---- [rustdoc] tests\rustdoc\inline_cross\macro-vis.rs stdout ----

error: rustdoc failed!
status: exit code: 0xc0000005
command: PATH="C:\a\rust\rust\build\i686-pc-windows-gnu\stage2\bin;C:\a\rust\rust\build\i686-pc-windows-gnu\stage0-bootstrap-tools\i686-pc-windows-gnu\release\deps;C:\a\rust\rust\build\i686-pc-windows-gnu\stage0\bin;C:\a\rust\rust\ninja;C:\a\rust\rust\mingw32\bin;C:\hostedtoolcache\windows\Python\3.11.4\x64\Scripts;C:\hostedtoolcache\windows\Python\3.11.4\x64;C:\msys64\usr\bin;C:\a\rust\rust\sccache;C:\Program Files\MongoDB\Server\5.0\bin;C:\aliyun-cli;C:\vcpkg;C:\cf-cli;C:\Program Files (x86)\NSIS;C:\tools\zstd;C:\Program Files\Mercurial;C:\hostedtoolcache\windows\stack\2.11.1\x64;C:\cabal\bin;C:\ghcup\bin;C:\Program Files\dotnet;C:\mysql\bin;C:\Program Files\R\R-4.3.1\bin\x64;C:\SeleniumWebDrivers\GeckoDriver;C:\Program Files (x86)\sbt\bin;C:\Program Files (x86)\GitHub CLI;C:\Program Files\Git\bin;C:\Program Files (x86)\pipx_bin;C:\npm\prefix;C:\hostedtoolcache\windows\go\1.20.7\x64\bin;C:\hostedtoolcache\windows\Python\3.7.9\x64\Scripts;C:\hostedtoolcache\windows\Python\3.7.9\x64;C:\hostedtoolcache\windows\Ruby\2.5.9\x64\bin;C:\Program Files\OpenSSL\bin;C:\tools\kotlinc\bin;C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\8.0.382-5\x64\bin;C:\Program Files\ImageMagick-7.1.1-Q16-HDRI;C:\Program Files (x86)\Microsoft SDKs\Azure\CLI2\wbin;C:\ProgramData\kind;C:\Program Files\Eclipse Foundation\jdk-8.0.302.8-hotspot\bin;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\Windows\System32\OpenSSH;C:\ProgramData\Chocolatey\bin;C:\Program Files\PowerShell\7;C:\Program Files\Microsoft\Web Platform Installer;C:\Program Files\Microsoft SQL Server\130\Tools\Binn;C:\Program Files\Microsoft SQL Server\Client SDK\ODBC\170\Tools\Binn;C:\Program Files (x86)\Windows Kits\10\Windows Performance Toolkit;C:\Program Files (x86)\Microsoft SQL Server\110\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\120\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\130\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\140\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\150\DTS\Binn;C:\Program Files (x86)\Microsoft SQL Server\160\DTS\Binn;C:\Strawberry\c\bin;C:\Strawberry\perl\site\bin;C:\Strawberry\perl\bin;C:\ProgramData\chocolatey\lib\pulumi\tools\Pulumi\bin;C:\Program Files\TortoiseSVN\bin;C:\Program Files\CMake\bin;C:\ProgramData\chocolatey\lib\maven\apache-maven-3.8.7\bin;C:\Program Files\Microsoft Service Fabric\bin\Fabric\Fabric.Code;C:\Program Files\Microsoft SDKs\Service Fabric\Tools\ServiceFabricLocalClusterManager;C:\Program Files\nodejs;C:\Program Files\Git\cmd;C:\Program Files\Git\mingw64\bin;C:\Program Files\Git\usr\bin;C:\Program Files\GitHub CLI;C:\tools\php;C:\Program Files (x86)\sbt\bin;C:\SeleniumWebDrivers\ChromeDriver;C:\SeleniumWebDrivers\EdgeDriver;C:\Program Files\Amazon\AWSCLIV2;C:\Program Files\Amazon\SessionManagerPlugin\bin;C:\Program Files\Amazon\AWSSAMCLI\bin;C:\Program Files (x86)\Google\Cloud SDK\google-cloud-sdk\bin;C:\Program Files (x86)\Microsoft BizTalk Server;C:\Program Files\LLVM\bin;C:\Users\runneradmin\.dotnet\tools;C:\Users\runneradmin\.cargo\bin;C:\Users\runneradmin\AppData\Local\Microsoft\WindowsApps" "C:\\a\\rust\\rust\\build\\i686-pc-windows-gnu\\stage2\\bin\\rustdoc.exe" "-L" "C:\\a\\rust\\rust\\build\\i686-pc-windows-gnu\\stage2\\lib\\rustlib\\i686-pc-windows-gnu\\lib" "-L" "C:\\a\\rust\\rust\\build\\i686-pc-windows-gnu\\test\\rustdoc\\inline_cross\\macro-vis\\auxiliary" "-o" "C:\\a\\rust\\rust\\build\\i686-pc-windows-gnu\\test\\rustdoc\\inline_cross\\macro-vis" "--deny" "warnings" "C:\\a\\rust\\rust\\tests\\rustdoc\\inline_cross\\macro-vis.rs" "-A" "internal_features"
stderr: none

bors · 2023-08-18T19:03:29Z

💔 Test failed - checks-actions

thomcc · 2023-08-18T20:14:30Z

@rustbot author

ChrisDenton · 2023-08-18T20:39:24Z

Hm, this PR shouldn't be affecting windows targets, not even mingw? Also weird that it's a rustdoc failure. My guess would be it's spurious.

thomcc · 2023-08-18T20:40:25Z

@bors retry

thomcc · 2023-08-19T02:30:14Z

@bors ping

bors · 2023-08-19T02:30:15Z

😪 I'm awake I'm awake

thomcc · 2023-08-19T02:30:24Z

@bors r+

bors · 2023-08-19T02:30:26Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Enable constant folding across crates (weak linkage + comdat) #114816

bors · 2023-08-19T02:30:27Z

📌 Commit 83c713b has been approved by thomcc

It is now in the queue for this repository.

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#114834 (Avoid side-effects from `try_coerce` when suggesting borrowing LHS of cast) - rust-lang#114968 (Fix UB in `std::sys::os::getenv()`) - rust-lang#114976 (Ignore unexpected incr-comp session dirs) - rust-lang#114999 (Migrate GUI colors test to original CSS color format) - rust-lang#115000 (custom_mir: change Call() terminator syntax to something more readable) r? `@ghost` `@rustbot` modify labels: rollup

Add data race test to `std::env::{get, set}` Complements rust-lang#114968, closes rust-lang#114949.

…ison-error-in-os, r=cuviper kmc-solid: Import `std::sync::PoisonError` in `std::sys::solid::os` Follow-up to rust-lang#114968. Fixes a missing import in [`*-kmc-solid_*`](https://doc.rust-lang.org/nightly/rustc/platform-support/kmc-solid.html) Tier 3 targets. ``` error[E0433]: failed to resolve: use of undeclared type `PoisonError` C:\Users\xxxxx\.rustup\toolchains\nightly-2023-08-23-x86_64-pc-windows-gnu\lib\rustlib\src\rust\library\std\src\sys\solid\os.rs(85,36) | 85 | ENV_LOCK.read().unwrap_or_else(PoisonError::into_inner) | ^^^^^^^^^^^ use of undeclared type `PoisonError` | ```

rustbot assigned thomcc Aug 18, 2023

lqd removed I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness C-bug Category: This is a bug. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Aug 18, 2023

Fix UB in std::sys::os::getenv()

83c713b

ShE3py force-pushed the unix-getsetenv-ub branch from ce33746 to 83c713b Compare August 18, 2023 14:07

ShE3py changed the title ~~Fix UB in std::sys::unix::os::getenv()~~ Fix UB in std::sys::os::getenv() Aug 18, 2023

thomcc reviewed Aug 18, 2023

View reviewed changes

bors 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 Aug 18, 2023

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

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2023

bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 18, 2023

matthiaskrgr mentioned this pull request Aug 20, 2023

Rollup of 5 pull requests #115018

Merged

bors merged commit 7b66abe into rust-lang:master Aug 20, 2023
11 checks passed

rustbot added this to the 1.73.0 milestone Aug 20, 2023

ShE3py mentioned this pull request Aug 20, 2023

Add data race test to std::env::{get, set} #115035

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 21, 2023

Auto merge of rust-lang#115035 - ShE3py:getsetenv-miri-test, r=thomcc

c40cfcf

Add data race test to `std::env::{get, set}` Complements rust-lang#114968, closes rust-lang#114949.

kawadakk mentioned this pull request Aug 23, 2023

kmc-solid: Import std::sync::PoisonError in std::sys::solid::os #115124

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix UB in `std::sys::os::getenv()` #114968

Fix UB in `std::sys::os::getenv()` #114968

ShE3py commented Aug 18, 2023 •

edited

Loading

rustbot commented Aug 18, 2023

Nekrolm commented Aug 18, 2023

ShE3py commented Aug 18, 2023 •

edited

Loading

thomcc commented Aug 18, 2023

thomcc Aug 18, 2023

thomcc commented Aug 18, 2023

bors commented Aug 18, 2023

ShE3py commented Aug 18, 2023

klensy commented Aug 18, 2023

ShE3py commented Aug 18, 2023

bors commented Aug 18, 2023

rust-log-analyzer commented Aug 18, 2023

bors commented Aug 18, 2023

thomcc commented Aug 18, 2023

ChrisDenton commented Aug 18, 2023

thomcc commented Aug 18, 2023

thomcc commented Aug 19, 2023

bors commented Aug 19, 2023

thomcc commented Aug 19, 2023

bors commented Aug 19, 2023

bors commented Aug 19, 2023

Fix UB in std::sys::os::getenv() #114968

Fix UB in std::sys::os::getenv() #114968

Conversation

ShE3py commented Aug 18, 2023 • edited Loading

rustbot commented Aug 18, 2023

Nekrolm commented Aug 18, 2023

ShE3py commented Aug 18, 2023 • edited Loading

thomcc commented Aug 18, 2023

thomcc Aug 18, 2023

Choose a reason for hiding this comment

thomcc commented Aug 18, 2023

bors commented Aug 18, 2023

ShE3py commented Aug 18, 2023

klensy commented Aug 18, 2023

ShE3py commented Aug 18, 2023

bors commented Aug 18, 2023

rust-log-analyzer commented Aug 18, 2023

bors commented Aug 18, 2023

thomcc commented Aug 18, 2023

ChrisDenton commented Aug 18, 2023

thomcc commented Aug 18, 2023

thomcc commented Aug 19, 2023

bors commented Aug 19, 2023

thomcc commented Aug 19, 2023

bors commented Aug 19, 2023

bors commented Aug 19, 2023

Fix UB in `std::sys::os::getenv()` #114968

Fix UB in `std::sys::os::getenv()` #114968

ShE3py commented Aug 18, 2023 •

edited

Loading

ShE3py commented Aug 18, 2023 •

edited

Loading