Fix unsound `File` methods #95469

ChrisDenton · 2022-03-30T12:05:13Z

This is a fix for #81357 (unsound File methods on Windows). That issue has an in-depth description of the problem.

If read, write, read_at, write_at, read_buf or other I/O method fails to complete synchronously then they will now abort the process with the message "I/O error: operation failed to complete synchronously".

rust-highfive · 2022-03-30T12:05:17Z

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

jstarks · 2022-03-31T19:52:41Z

NtReadFile and NtWriteFile are publicly documented and so are fine from the Windows team's perspective.

I don't think trying CancelIo is worth it. Whether it will work synchronously like this is highly dependent on the machine, Windows version, configured file system drivers, etc. Better to be deterministic.

ChrisDenton · 2022-03-31T20:42:25Z

Thanks! Using it does appear dubious at best so I'll remove it.

joshtriplett · 2022-04-03T18:38:39Z

The general approach here seems reasonable to me. However, rather than manually printing a message and aborting, could this use rtabort!?

r=me with that change, assuming it's possible.

joshtriplett · 2022-04-04T05:51:57Z

@bors r+

bors · 2022-04-04T05:51:58Z

📌 Commit be16d650dadc758244b38e41a766d13730707a91 has been approved by joshtriplett

joshtriplett · 2022-04-04T05:53:38Z

@bors r-

(This looks ready to me, but I'll hold off until this is no longer a draft. Feel free to r=me when ready, and once #95467 has gone in.)

bors · 2022-04-04T23:06:24Z

☔ The latest upstream changes (presumably #95653) made this pull request unmergeable. Please resolve the merge conflicts.

ChrisDenton · 2022-04-05T09:01:28Z

Ok, I've rebased on master so this should now be ready for merge.

Dylan-DPC · 2022-04-05T12:32:36Z

@bors r=joshtriplett

bors · 2022-04-05T12:32:37Z

📌 Commit d2ce150 has been approved by joshtriplett

rust-timer · 2022-04-06T06:10:15Z

Finished benchmarking commit (26b5e0c): comparison url.

Summary:

Primary benchmarks: changes not relevant. 2 results were found to be statistically significant but the changes were too small to be relevant.
Secondary benchmarks: 🎉 relevant improvements found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	1	2	6	2
mean²	N/A	0.3%	-0.4%	-1.1%	-0.4%
max	N/A	0.3%	-0.4%	-1.3%	-0.4%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

…isDenton make windows compat_fn (crudely) work on Miri With rust-lang#95469, Windows `compat_fn!` now has to be supported by Miri to even make stdout work. Unfortunately, it relies on some outside-of-Rust linker hacks (`#[link_section = ".CRT$XCU"]`) that are rather hard to make work in Miri. So I came up with this crude hack to make this stuff work in Miri regardless. It should come at no cost for regular executions, so I hope this is okay. Cc rust-lang#95627 `@ChrisDenton`

…Denton make windows compat_fn (crudely) work on Miri With rust-lang#95469, Windows `compat_fn!` now has to be supported by Miri to even make stdout work. Unfortunately, it relies on some outside-of-Rust linker hacks (`#[link_section = ".CRT$XCU"]`) that are rather hard to make work in Miri. So I came up with this crude hack to make this stuff work in Miri regardless. It should come at no cost for regular executions, so I hope this is okay. Cc rust-lang#95627 `@ChrisDenton`

…implementation This fixes #4801, where, as a result of rust-lang/rust#95469, our implementation of cat used for this test no longer works, as stdio functions on windows now can abort the process if the pipe is set to nonblocking mode. Unfortunately in windows, setting one end of the pipe to be nonblocking makes the whole thing nonblocking, so when, in tokio::process we set the child pipes to nonblocking mode, it causes serious problems for any rust program at the other end. Fixing this issue is for another day, but fixing the tests is for today.

…implementation (#4803) This fixes #4801, where, as a result of rust-lang/rust#95469, our implementation of cat used for this test no longer works, as stdio functions on windows now can abort the process if the pipe is set to nonblocking mode. Unfortunately in windows, setting one end of the pipe to be nonblocking makes the whole thing nonblocking, so when, in tokio::process we set the child pipes to nonblocking mode, it causes serious problems for any rust program at the other end. Fixing this issue is for another day, but fixing the tests is for today.

Add rust-lang#95469 to the release notes rust-lang#95469 may break programs using async file handles so it should've been noted in compatibility notes (sorry).

Rollup of 6 pull requests Successful merges: - rust-lang#98701 (Add regression test for rust-lang#50439) - rust-lang#98715 (add ice test for rust-lang#97047) - rust-lang#98753 (Fix `x dist rust-dev` on a fresh checkout) - rust-lang#98805 (Add rust-lang#95469 to the release notes) - rust-lang#98812 (feat: Add a documentation problem issue template) - rust-lang#98819 (update Miri) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

make windows compat_fn (crudely) work on Miri With rust-lang/rust#95469, Windows `compat_fn!` now has to be supported by Miri to even make stdout work. Unfortunately, it relies on some outside-of-Rust linker hacks (`#[link_section = ".CRT$XCU"]`) that are rather hard to make work in Miri. So I came up with this crude hack to make this stuff work in Miri regardless. It should come at no cost for regular executions, so I hope this is okay. Cc rust-lang/rust#95627 `@ChrisDenton`

Add fallback implementation in stdio redirection for when named pipes are not available Since Windows 9X/ME does not support creating named pipes (only connecting to remote pipes created on NT), we'll have to make do with anonymous pipes, without overlapped I/O. In particular, this means that we'll have to spawn another thread in the case where both stdout and stderr are being piped and read from (`read2`). We also use the fallback implementation on NT before 4.0, as the `Drop` impl of `AsyncPipe` needs to be able to cancel I/O via `CancelIo`. Add fallbacks for `NtReadFile` and `NtWriteFile` in `synchronous_{read, write}` These might be unsound for handles that _can_ be asynchronous on 9x/ME. See rust-lang#95469 for more info

rust-highfive assigned yaahc Mar 30, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2022

ChrisDenton force-pushed the unsound-read-write branch from b716b4b to 51061bf Compare March 30, 2022 12:19

yaahc added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Mar 30, 2022

ChrisDenton force-pushed the unsound-read-write branch 3 times, most recently from 6170c7f to e6f3191 Compare April 1, 2022 07:29

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 Apr 4, 2022

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 4, 2022

ChrisDenton added 5 commits April 5, 2022 08:11

Correct definition of IO_STATUS_BLOCK

66faaa8

Complete reads and writes synchronously or abort

36aa75e

Document synchronicity

084b71a

Make synchronous_write safe to call

88c05ed

Use rtabort

d2ce150

ChrisDenton force-pushed the unsound-read-write branch from be16d65 to d2ce150 Compare April 5, 2022 07:22

ChrisDenton changed the title ~~[DRAFT] Fix unsound File methods~~ Fix unsound File methods Apr 5, 2022

ChrisDenton marked this pull request as ready for review April 5, 2022 07:22

bors mentioned this pull request Apr 6, 2022

Add ReadBufRef #93359

Closed

This was referenced Apr 6, 2022

Update libc to 0.2.121 #95688

Merged

[bootstrap] Print the full relative path to failed tests #95352

Merged

ChrisDenton mentioned this pull request Apr 7, 2022

Bump bootstrap compiler to 1.61.0 beta #95678

Merged

This was referenced Apr 7, 2022

Rustup rust-lang/miri#2055

Merged

miri no longer builds after rust-lang/rust#95624 #95627

Closed

make windows compat_fn (crudely) work on Miri #95775

Merged

ChrisDenton restored the unsound-read-write branch April 10, 2022 21:15

ChrisDenton deleted the unsound-read-write branch April 28, 2022 20:23

pie-flavor mentioned this pull request Jul 1, 2022

process: Windows tests failing on 1.62 tokio-rs/tokio#4801

Closed

Noah-Kennedy mentioned this pull request Jul 2, 2022

process: fix tokio process abort issues on windows tokio-rs/tokio#4802

Closed

Noah-Kennedy mentioned this pull request Jul 2, 2022

tests: alter integration tests for stdio to not use a handrolled cat implementation tokio-rs/tokio#4803

Merged

ChrisDenton added a commit to ChrisDenton/rust that referenced this pull request Jul 2, 2022

Add rust-lang#95469 to the release notes

ee3e518

ChrisDenton mentioned this pull request Jul 2, 2022

Add #95469 to the release notes #98805

Merged

RalfJung mentioned this pull request Jul 2, 2022

Rollup of 6 pull requests #98820

Merged

smmalis37 mentioned this pull request Jul 5, 2022

Writing a sufficiently large string to a git bash pipe on windows regressed in 1.62 #98947

Closed

sunfishcode mentioned this pull request Aug 17, 2022

Implement I/O-safe traits on types async-rs/async-std#1036

Merged

fredizzimo mentioned this pull request Feb 11, 2024

Crashes on Windows 10 neovide/neovide#2299

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unsound `File` methods #95469

Fix unsound `File` methods #95469

ChrisDenton commented Mar 30, 2022 •

edited

Loading

rust-highfive commented Mar 30, 2022

jstarks commented Mar 31, 2022

ChrisDenton commented Mar 31, 2022

joshtriplett commented Apr 3, 2022 •

edited

Loading

joshtriplett commented Apr 4, 2022

bors commented Apr 4, 2022

joshtriplett commented Apr 4, 2022 •

edited

Loading

bors commented Apr 4, 2022

ChrisDenton commented Apr 5, 2022

Dylan-DPC commented Apr 5, 2022

bors commented Apr 5, 2022

rust-timer commented Apr 6, 2022

Fix unsound File methods #95469

Fix unsound File methods #95469

Conversation

ChrisDenton commented Mar 30, 2022 • edited Loading

rust-highfive commented Mar 30, 2022

jstarks commented Mar 31, 2022

ChrisDenton commented Mar 31, 2022

joshtriplett commented Apr 3, 2022 • edited Loading

joshtriplett commented Apr 4, 2022

bors commented Apr 4, 2022

joshtriplett commented Apr 4, 2022 • edited Loading

bors commented Apr 4, 2022

ChrisDenton commented Apr 5, 2022

Dylan-DPC commented Apr 5, 2022

bors commented Apr 5, 2022

rust-timer commented Apr 6, 2022

Footnotes

Fix unsound `File` methods #95469

Fix unsound `File` methods #95469

ChrisDenton commented Mar 30, 2022 •

edited

Loading

joshtriplett commented Apr 3, 2022 •

edited

Loading

joshtriplett commented Apr 4, 2022 •

edited

Loading