Skip to content
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.

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

Fix unsound File methods #95469

Merged
merged 5 commits into from
Apr 6, 2022
Merged

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Mar 30, 2022

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
Copy link
Collaborator

r? @yaahc

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2022
@yaahc yaahc added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Mar 30, 2022
@jstarks
Copy link

jstarks commented Mar 31, 2022

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
Copy link
Member Author

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

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

joshtriplett commented Apr 3, 2022

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
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 4, 2022

📌 Commit be16d650dadc758244b38e41a766d13730707a91 has been approved by joshtriplett

@bors 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
@joshtriplett
Copy link
Member

joshtriplett commented Apr 4, 2022

@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 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
@bors
Copy link
Contributor

bors commented Apr 4, 2022

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

@ChrisDenton ChrisDenton changed the title [DRAFT] Fix unsound File methods Fix unsound File methods Apr 5, 2022
@ChrisDenton ChrisDenton marked this pull request as ready for review April 5, 2022 07:22
@ChrisDenton
Copy link
Member Author

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

@Dylan-DPC
Copy link
Member

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented Apr 5, 2022

📌 Commit d2ce150 has been approved by joshtriplett

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 7, 2022
…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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 8, 2022
…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`
@ChrisDenton ChrisDenton restored the unsound-read-write branch April 10, 2022 21:15
@ChrisDenton ChrisDenton deleted the unsound-read-write branch April 28, 2022 20:23
Noah-Kennedy added a commit to tokio-rs/tokio that referenced this pull request Jul 2, 2022
…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.
Noah-Kennedy added a commit to tokio-rs/tokio that referenced this pull request Jul 2, 2022
…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.
ChrisDenton added a commit to ChrisDenton/rust that referenced this pull request Jul 2, 2022
RalfJung added a commit to RalfJung/rust that referenced this pull request Jul 2, 2022
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).
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 2, 2022
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
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
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`
seritools added a commit to rust9x/rust that referenced this pull request Dec 29, 2023
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
seritools added a commit to rust9x/rust that referenced this pull request Nov 30, 2024
- 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
seritools added a commit to rust9x/rust that referenced this pull request Dec 1, 2024
- 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
seritools added a commit to rust9x/rust that referenced this pull request Dec 1, 2024
- 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
seritools added a commit to rust9x/rust that referenced this pull request Dec 3, 2024
- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.