Avoid closing invalid handles#119029

dylni · 2023-12-16T22:45:28Z

Documentation for HandleOrInvalid has this note:

If holds a handle other than INVALID_HANDLE_VALUE, it will close the handle on drop.

Documentation for HandleOrNull has this note:

If this holds a non-null handle, it will close the handle on drop.

Currently, both will call CloseHandle on their invalid handles as a result of using OwnedHandle internally, contradicting the above paragraphs. This PR adds destructors that match the documentation.

@rustbot label A-io O-windows T-libs

rustbot · 2023-12-16T22:45:36Z

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

thomcc · 2024-02-01T22:51:30Z

I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks.

r? libs

joboet

That's a very good catch! Though I think this could be written a bit simpler without ManuallyDrop.

joboet · 2024-02-11T10:22:49Z

library/std/src/os/windows/io/handle.rs

 #[stable(feature = "io_safety", since = "1.63.0")]
 #[derive(Debug)]
-pub struct HandleOrNull(OwnedHandle);
+pub struct HandleOrNull(ManuallyDrop<OwnedHandle>);


I'd simply use RawHandle here, so that we don't have to do the ManuallyDrop::take dance below.

Done. Since it's not possible to move out the inner field with a custom Drop implementation, I was looking for a decent way to implement TryFrom. However, I can just forget the struct altogether.

joboet · 2024-02-11T10:25:10Z

library/std/src/os/windows/io/handle.rs

-#[stable(feature = "io_safety", since = "1.63.0")]
-impl TryFrom<HandleOrNull> for OwnedHandle {
-    type Error = NullHandleError;
+macro_rules! impl_handle_or {


Especially when combined with the suggestion above, I don't think that a macro is really necessary here.

I would likely still use a macro due to the shared code between the implementations, but I don't have strong opinions if the preference is to avoid macros here.

ChrisDenton · 2024-02-15T13:32:03Z

Hi @dylni could you address the comments? Use @rustbot ready when you're done, thanks!

@rustbot author

dylni · 2024-02-17T16:10:19Z

@rustbot ready

joboet

Looks great!

ChrisDenton · 2024-02-19T10:34:08Z

Thanks!

@bors r+ rollup

bors · 2024-02-19T10:34:11Z

📌 Commit 24556e0 has been approved by ChrisDenton

It is now in the queue for this repository.

klensy · 2024-02-19T10:52:45Z

library/std/src/os/windows/io/handle.rs

+    }
+}
+
+#[stable(feature = "handle_or_drop", since = "1.75.0")]


From where that 1.75? And next one.

Hm, you're right. And this doesn't need a new feature as the fact it needs dropping isn't itself new.

ChrisDenton · 2024-02-19T10:57:55Z

@bors r-

dylni · 2024-03-09T16:40:57Z

Updated and rebased on the latest master

diff --git a/library/std/src/os/windows/io/handle.rs b/library/std/src/os/windows/io/handle.rs
index 5566f525ea4..30a809daa21 100644
--- a/library/std/src/os/windows/io/handle.rs
+++ b/library/std/src/os/windows/io/handle.rs
@@ -173,7 +173,7 @@ fn try_from(handle_or_null: HandleOrNull) -> Result<Self, NullHandleError> {
     }
 }
 
-#[stable(feature = "handle_or_drop", since = "1.75.0")]
+#[stable(feature = "io_safety", since = "1.63.0")]
 impl Drop for HandleOrNull {
     #[inline]
     fn drop(&mut self) {
@@ -251,7 +251,7 @@ fn try_from(handle_or_invalid: HandleOrInvalid) -> Result<Self, InvalidHandleErr
     }
 }
 
-#[stable(feature = "handle_or_drop", since = "1.75.0")]
+#[stable(feature = "io_safety", since = "1.63.0")]
 impl Drop for HandleOrInvalid {
     #[inline]
     fn drop(&mut self) {

@rustbot ready

ChrisDenton · 2024-03-14T03:40:25Z

Thanks!

@bors r+ rollup

bors · 2024-03-14T03:40:27Z

📌 Commit a82587c has been approved by ChrisDenton

It is now in the queue for this repository.

…s, r=ChrisDenton Avoid closing invalid handles Documentation for [`HandleOrInvalid`] has this note: > If holds a handle other than `INVALID_HANDLE_VALUE`, it will close the handle on drop. Documentation for [`HandleOrNull`] has this note: > If this holds a non-null handle, it will close the handle on drop. Currently, both will call `CloseHandle` on their invalid handles as a result of using `OwnedHandle` internally, contradicting the above paragraphs. This PR adds destructors that match the documentation. `@rustbot` label A-io O-windows T-libs [`HandleOrInvalid`]: https://doc.rust-lang.org/std/os/windows/io/struct.HandleOrInvalid.html [`HandleOrNull`]: https://doc.rust-lang.org/std/os/windows/io/struct.HandleOrNull.html

…s, r=ChrisDenton Avoid closing invalid handles Documentation for [`HandleOrInvalid`] has this note: > If holds a handle other than `INVALID_HANDLE_VALUE`, it will close the handle on drop. Documentation for [`HandleOrNull`] has this note: > If this holds a non-null handle, it will close the handle on drop. Currently, both will call `CloseHandle` on their invalid handles as a result of using `OwnedHandle` internally, contradicting the above paragraphs. This PR adds destructors that match the documentation. ``@rustbot`` label A-io O-windows T-libs [`HandleOrInvalid`]: https://doc.rust-lang.org/std/os/windows/io/struct.HandleOrInvalid.html [`HandleOrNull`]: https://doc.rust-lang.org/std/os/windows/io/struct.HandleOrNull.html

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#119029 (Avoid closing invalid handles) - rust-lang#122238 (Document some builtin impls in the next solver) - rust-lang#122247 (rustdoc-search: depth limit `T<U>` -> `U` unboxing) - rust-lang#122287 (add test ensuring simd codegen checks don't run when a static assertion failed) - rust-lang#122368 (chore: remove repetitive words) - rust-lang#122397 (Various cleanups around the const eval query providers) - rust-lang#122406 (Fix WF for `AsyncFnKindHelper` in new trait solver) - rust-lang#122477 (Change some attribute to only_local) - rust-lang#122482 (Ungate the `UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES` lint) - rust-lang#122490 (Update build instructions for OpenHarmony) Failed merges: - rust-lang#122471 (preserve span when evaluating mir::ConstOperand) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#119029 - dylni:avoid-closing-invalid-handles, r=ChrisDenton Avoid closing invalid handles Documentation for [`HandleOrInvalid`] has this note: > If holds a handle other than `INVALID_HANDLE_VALUE`, it will close the handle on drop. Documentation for [`HandleOrNull`] has this note: > If this holds a non-null handle, it will close the handle on drop. Currently, both will call `CloseHandle` on their invalid handles as a result of using `OwnedHandle` internally, contradicting the above paragraphs. This PR adds destructors that match the documentation. ```@rustbot``` label A-io O-windows T-libs [`HandleOrInvalid`]: https://doc.rust-lang.org/std/os/windows/io/struct.HandleOrInvalid.html [`HandleOrNull`]: https://doc.rust-lang.org/std/os/windows/io/struct.HandleOrNull.html

rustbot assigned thomcc Dec 16, 2023

This comment has been minimized.

Sign in to view

dylni force-pushed the avoid-closing-invalid-handles branch 3 times, most recently from ed08061 to 599b092 Compare December 18, 2023 00:48

rustbot assigned m-ou-se and unassigned thomcc Feb 1, 2024

joboet approved these changes Feb 11, 2024

View reviewed changes

m-ou-se assigned ChrisDenton and unassigned m-ou-se Feb 14, 2024

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 Feb 15, 2024

dylni force-pushed the avoid-closing-invalid-handles branch from 599b092 to 24556e0 Compare February 17, 2024 15:58

rustbot 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 Feb 17, 2024

joboet approved these changes Feb 17, 2024

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 Feb 19, 2024

klensy reviewed Feb 19, 2024

View reviewed changes

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 Feb 19, 2024

dylni force-pushed the avoid-closing-invalid-handles branch from 24556e0 to f3b5b08 Compare March 9, 2024 16:38

rustbot 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 Mar 9, 2024

This comment has been minimized.

Sign in to view

Avoid closing invalid handles

a82587c

dylni force-pushed the avoid-closing-invalid-handles branch from f3b5b08 to a82587c Compare March 9, 2024 16:43

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 Mar 14, 2024

jhpratt mentioned this pull request Mar 14, 2024

Rollup of 10 pull requests #122472

Closed

This was referenced Mar 14, 2024

Rollup of 6 pull requests #122475

Closed

Rollup of 10 pull requests #122497

Merged

bors merged commit 280a1da into rust-lang:master Mar 14, 2024

rustbot added this to the 1.78.0 milestone Mar 14, 2024

Uh oh!

Comments

Conversation

dylni commented Dec 16, 2023

Uh oh!

rustbot commented Dec 16, 2023

Uh oh!

This comment has been minimized.

thomcc commented Feb 1, 2024

Uh oh!

joboet left a comment

Choose a reason for hiding this comment

Uh oh!

joboet Feb 11, 2024

Choose a reason for hiding this comment

Uh oh!

dylni Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

joboet Feb 11, 2024

Choose a reason for hiding this comment

Uh oh!

dylni Feb 17, 2024

Choose a reason for hiding this comment

Uh oh!

ChrisDenton commented Feb 15, 2024

Uh oh!

dylni commented Feb 17, 2024

Uh oh!

joboet left a comment

Choose a reason for hiding this comment

Uh oh!

ChrisDenton commented Feb 19, 2024

Uh oh!

bors commented Feb 19, 2024

Uh oh!

klensy Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisDenton Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

ChrisDenton commented Feb 19, 2024

Uh oh!

dylni commented Mar 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

ChrisDenton commented Mar 14, 2024

Uh oh!

bors commented Mar 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

klensy Feb 19, 2024 •

edited

Loading

dylni commented Mar 9, 2024 •

edited

Loading