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.

Sign up for GitHub

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

Jump to bottom

Fix assertion failures in OwnedHandle with windows_subsystem. #88798

Merged
merged 3 commits into from
Nov 11, 2021
Merged

Fix assertion failures in OwnedHandle with windows_subsystem. #88798

merged 3 commits into from
Nov 11, 2021

Conversation

sunfishcode
Copy link
Member

As discussed in #88576, raw handle values in Windows can be null, such
as in windows_subsystem mode, or when consoles are detached from a
process. So, don't use NonNull to hold them, don't assert that they're
not null, and remove OwnedHandle's repr(transparent). Introduce a
new HandleOrNull type, similar to HandleOrInvalid, to cover the FFI
use case.

r? @joshtriplett

@sunfishcode
Fix assertion failures in OwnedHandle with windows_subsystem.
3b97481 
As discussed in rust-lang#88576, raw handle values in Windows can be null, such
as in `windows_subsystem` mode, or when consoles are detached from a
process. So, don't use `NonNull` to hold them, don't assert that they're
not null, and remove `OwnedHandle`'s `repr(transparent)`. Introduce a
new `HandleOrNull` type, similar to `HandleOrInvalid`, to cover the FFI
use case.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2021
@sunfishcode sunfishcode mentioned this pull request Sep 9, 2021
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@the8472 the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 21, 2021
@joshtriplett
Copy link
Member

Can you please clarify in the comments whether HandleOrNull can be invalid, and whether HandleOrInvalid can be NULL?

I don't know which way around makes sense, but I would expect that there's some type that doesn't allow either invalid or NULL, and whose try_into() implementation rejects both; I'd expect that to be common in FFI. I don't think we need the entire 2x2 matrix to handle disallowing NULL, invalid, or both; I think it'd make sense to have a type for the "both" case and a type for whichever one can appear without the other.

@joshtriplett joshtriplett 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 Oct 4, 2021
@sunfishcode
Copy link
Member Author

Can you please clarify in the comments whether HandleOrNull can be invalid, and whether HandleOrInvalid can be NULL?

I don't know which way around makes sense, but I would expect that there's some type that doesn't allow either invalid or NULL, and whose try_into() implementation rejects both; I'd expect that to be common in FFI. I don't think we need the entire 2x2 matrix to handle disallowing NULL, invalid, or both; I think it'd make sense to have a type for the "both" case and a type for whichever one can appear without the other.

I've now added a patch documenting this.

My current understanding of windows_subsystem = "windows" mode and detached consoles is that we basically have to treat NULL as a valid handle value except in APIs where it's the documented error value. And my understanding of GetCurrentProcess() is that we basically have to treat INVALID_HANDLE_VALUE as a valid handle value except in APIs where it's the documented error value. Under that assumption, neither of HandleOrNull or HandleOrInvalid make sense as a "both" type.

@sunfishcode
Copy link
Member Author

@joshtriplett I've now added comments clarifying the valid values for HandleIfNull and HandleIfInvalid.

@sunfishcode
Copy link
Member Author

I've now addressed the review comments.

r? @joshtriplett

@sunfishcode sunfishcode added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2021
@camelid camelid added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2021
@sunfishcode
Copy link
Member Author

I've addressed the comments, so this is ready for review.

r? @joshtriplett

@Subsentient
Copy link

@joshtriplett

@joshtriplett joshtriplett added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 10, 2021
@joshtriplett
Copy link
Member

Will review this today; sorry for the delay. I feel like this might not be exactly the right design for handling NULL, but that's a secondary concern. It's a nightly API, and we can always evolve it further, but we need to fix the issue first.

@joshtriplett joshtriplett added the stable-nominated Nominated for backporting to the compiler in the stable channel. label Nov 10, 2021
@joshtriplett
Copy link
Member

Nominating for both stable and beta backport. T-libs thinks this is definitely worth a beta backport, but it's only worth a stable backport if we're already going to do a stable release for some other reason.

@Subsentient
Copy link

Nominating for both stable and beta backport. T-libs thinks this is definitely worth a beta backport, but it's only worth a stable backport if we're already going to do a stable release for some other reason.

Personally I'd backport to stable as well -- I feel like having the GUI target broken on stable Rust does not look good or inspire confidence in Rust for anyone who hits this bug. It's core functionality for Windows targets.

@joshtriplett
Copy link
Member

Reviewing this carefully, I'm not sure this is the right API, but I am sure that we should prioritize a fix for the observed assertion failures that occur with stable APIs backed by this over further refinements to the nightly-only API.

@bors r+ rollup=never p=10

@bors
Copy link
Contributor

bors commented Nov 11, 2021

📌 Commit 5d79870 has been approved by joshtriplett

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 11, 2021
@bors
Copy link
Contributor

bors commented Nov 11, 2021

⌛ Testing commit 5d79870 with merge b95b96d44ba66d4615822c169c0c924d50b670dc...

@bors
Copy link
Contributor

bors commented Nov 11, 2021

💔 Test failed - checks-actions

@bors 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 Nov 11, 2021
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-msvc-cargo failed! Check out the build log: (web) (plain)

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

testing https://github.com/BurntSushi/xsv
Initialized empty Git repository in D:/a/rust/rust/build/ct/xsv/.git/
fatal: Could not parse object '3de6c04269a7d315f7e9864b9013451cd9580a08'.
fatal: unable to access 'https://github.com/BurntSushi/xsv/': Failed to connect to github.com port 443 after 21055 ms: Timed out
thread 'main' panicked at 'assertion failed: status.success()', src\tools\cargotest\main.rs:125:13


command did not execute successfully: "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage0-tools-bin\\cargotest.exe" "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage2-tools-bin\\cargo.exe" "D:\\a\\rust\\rust\\build\\ct"
expected success, got: exit code: 101

@Subsentient
Copy link

@joshtriplett Restart the CI? ^^

@joshtriplett
Copy link
Member

@bors retry

@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 Nov 11, 2021
@bors
Copy link
Contributor

bors commented Nov 11, 2021

⌛ Testing commit 5d79870 with merge d71ba74...

@bors
Copy link
Contributor

bors commented Nov 11, 2021

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing d71ba74 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 11, 2021
@bors bors merged commit d71ba74 into rust-lang:master Nov 11, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 11, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d71ba74): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

@ChrisDenton ChrisDenton mentioned this pull request Nov 13, 2021
Closed
@sunfishcode sunfishcode added the I-libs-nominated Nominated for discussion during a libs team meeting. label Nov 13, 2021
@joshtriplett joshtriplett added beta-accepted Accepted for backporting to the compiler in the beta channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels Nov 15, 2021
@cuviper cuviper mentioned this pull request Nov 16, 2021
Merged
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 16, 2021
@cuviper cuviper modified the milestones: 1.58.0, 1.57.0 Nov 16, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 19, 2021
@bors
Auto merge of rust-lang#90938 - cuviper:beta-next, r=Mark-Simulacrum
7611e64 
[beta] backports

-  Fix assertion failures in OwnedHandle with windows_subsystem. rust-lang#88798
-  Ensure that pushing empty path works as before on verbatim paths rust-lang#89665
-  Feature gate + make must_not_suspend allow-by-default rust-lang#89826
-  Only use clone3 when needed for pidfd rust-lang#89930
-  Fix documentation header sizes rust-lang#90186
-  Fixes incorrect handling of ADT's drop requirements rust-lang#90218
-  Fix ICE when forgetting to Box a parameter to a Self::func call rust-lang#90221
-  Prevent duplicate caller bounds candidates by exposing default substs in Unevaluated rust-lang#90266
-  Update odht crate to 0.3.1 (big-endian bugfix) rust-lang#90403
-  rustdoc: Go back to loading all external crates unconditionally rust-lang#90489
-  Split doc_cfg and doc_auto_cfg features rust-lang#90502
-  Apply adjustments for field expression even if inaccessible rust-lang#90508
-  Warn for variables that are no longer captured rust-lang#90597
-  Properly register text_direction_codepoint_in_comment lint. rust-lang#90626
-  CI: Use ubuntu image to download openssl, curl sources, cacert.pem for x86 dist builds rust-lang#90457
-  Android is not GNU rust-lang#90834
-  Update llvm submodule rust-lang#90954

Additionally, this bumps the stage 0 compiler from beta to stable 1.56.1.

r? `@Mark-Simulacrum`
@joshtriplett joshtriplett removed the I-libs-nominated Nominated for discussion during a libs team meeting. label Dec 1, 2021
@pietroalbini pietroalbini removed stable-nominated Nominated for backporting to the compiler in the stable channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshtriplett joshtriplett Awaiting requested review from joshtriplett

Assignees

@joshtriplett joshtriplett

Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` beta-accepted Accepted for backporting to the compiler in the beta channel. 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
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.