deny(unsafe_op_in_unsafe_fn) in libstd/process.rs #73955
Conversation
|
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
src/libstd/process.rs
Outdated
| @@ -311,7 +312,8 @@ impl Read for ChildStdout { | |||
|
|
|||
| #[inline] | |||
| unsafe fn initializer(&self) -> Initializer { | |||
| Initializer::nop() | |||
| // SAFETY: Read is guaranteed to work on uninitialized memory | |||
There was a problem hiding this comment.
This is not quite accurate. In order for this to be safe, we basically need to guarantee that AnonPipe's read function is not going to read from the passed buffer. That's not a local property, nor is it an easy thing to check -- we have a bunch of AnonPipe impls most of which delegate to other struct themselves :/
It might be better to add a function to AnonPipe that returns an initializer and return that here, rather than making this assertion here. In the future this'll get replaced anyway once rust-lang/rfcs#2930 is implemented, though, so that may not be worth it -- it might be best to
There was a problem hiding this comment.
So, what would be the correct comment here? It isn't safe, but that's how we do it currently? ;)
There was a problem hiding this comment.
It probably is safe and fine currently, but this comment makes it seem like that's "just true." Ideally, we would change the code to make reasoning about this local (e.g., add functions providing access to Initializer to each pipe and use those here). But that's a fairly large task. Alternatively, we can just write this comment being explicit about what has been checked. Something like "This is safe only if all implementations of AnonPipe correctly handle uninitialized data being passed into their read methods, which has not been confirmed to be the case" or something along those lines.
|
☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@hellow554 |
hellow554
force-pushed
the
unsafe_process
branch
from
5206078
to
73e27b3
Compare
|
Resolved the merge conflict, but I'm still not sure what to write about the safety. I just copied the comments found in |
crlf0710
added
T-libs
|
I'm going to approve this since it seems pretty harmless in the meantime and I suspect we'll shortly be replacing these unsafe blocks with the ReadBuf abstraction once the RFC goes through... @bors r+ rollup |
|
📌 Commit 73e27b3 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 10 pull requests Successful merges: - rust-lang#73955 (deny(unsafe_op_in_unsafe_fn) in libstd/process.rs) - rust-lang#75146 (Detect overflow in proc_macro_server subspan) - rust-lang#75304 (Note when a a move/borrow error is caused by a deref coercion) - rust-lang#75749 (Consolidate some duplicate code in the sys modules.) - rust-lang#75882 (Use translated variable for test string) - rust-lang#75886 (Test that bounds checks are elided for [..index] after .position()) - rust-lang#76048 (Initial support for riscv32gc_unknown_linux_gnu) - rust-lang#76198 (Make some Ordering methods const) - rust-lang#76689 (Upgrade to pulldown-cmark 0.8.0) - rust-lang#76763 (Update cargo) Failed merges: r? `@ghost`
The libstd/process.rs part of #73904 . Wraps the two calls to an unsafe fn Initializer::nop() in an unsafe block.
Will have to wait for #73909 to be merged, because of the feature in the libstd/lib.rs