Implement From<OwnedFd/Handle> for ChildStdin/out/err object #98704

vthib · 2022-06-30T09:31:46Z

Summary

Comments in library/std/src/process.rs ( ab08639 ) indicates that ChildStdin, ChildStdout, ChildStderr implements some traits that are not actually implemented: FromRawFd, FromRawHandle, and the From<OwnedFd>/From<OwnedHandle> from the io_safety feature.

In this PR I implement FromRawHandle and FromRawFd for those 3 objects.

Usecase

I have a usecase where those implementations are basically needed. I want to customize
in the Command::spawn API how the pipes for the parent/child communications are created (mainly to strengthen the security attributes on them). I can properly setup the pipes,
and the "child" handles can be provided to Child::spawn easily using Stdio::from_raw_handle. However, there is no way to generate the ChildStd* objects from the raw handle of the created name pipe, which would be very useful to still expose the same API
than in other OS (basically a spawn(...) -> (Child, ChildStdin, ChildStdout, ChildSterr), where on windows this is customized), and to for example use tokio::ChildStdin::from_std afterwards.

Questions

Are those impls OK to add? I have searched to see if those impls were missing on purpose, or if it was just never implemented because never needed. I haven't found any indication on why they couldn't be added, although the user clearly has to be very careful that the handle provided makes sense (i think, mainly that it is in overlapped mode for windows).
If this change is ok, adding the impls for the io_safety feature would probably be best, or should it be done in another PR?
I just copy-pasted the #[stable(...)] attributes, but the since value has to be updated, I'm not sure to which value.

rustbot · 2022-06-30T09:31:51Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-06-30T09:31:51Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

Please see the contribution instructions for more information.

vthib · 2022-06-30T09:37:20Z

@rustbot label +T-libs-api -T-libs

sunfishcode · 2022-07-05T16:22:38Z

I'm concerned that this exposes std code that isn't expecting to handle non-pipe file descriptors to non-pipe file descriptors. The current implementation of unix AnonPipe assumes it can set and clear the non-blocking flag and use poll, which looks like it would be some awkward surface area if ChildStdout et al supported other file descriptor types.

Independently of that, please do add implementations of the io_safety traits, From<OwnedFd> and From<OwnedHandle> for anything that gets FromRawFd and FromRawHandle implementations, so that we don't need to start keeping track of which things in std have one impl and not the other.

vthib · 2022-07-06T14:26:51Z

I have added implementations for From<OwnedFd> and From<OwnedHandle>.

As for your remark, I agree, but i'm not really sure how this is really handled in other modules. AFAICT FromRawFd is also implemented for other objects where you probably have to be careful about which fd you provide (TcpListener, UdpSocket, ...), so this MR doesn't appear to bring From impls that are too different from those.

the8472 · 2022-07-27T18:41:35Z

which looks like it would be some awkward surface area if ChildStdout et al supported other file descriptor types.

Isn't this already possible via https://doc.rust-lang.org/std/process/struct.Stdio.html#impl-From%3CFile%3E ?

yaahc · 2022-07-27T18:53:28Z

r? rust-lang/libs

joshtriplett · 2022-07-27T18:59:06Z

A potential policy question: now that we have OwnedFd/BorrowedFd, should we still provide RawFd-related types and traits in new additions?

joshtriplett · 2022-08-09T15:16:15Z

We discussed this in today's @rust-lang/libs-api meeting.

For now, we'd like to just add the impls related to OwnedFd and BorrowedFd, and hold off on the RawFd-related impls for the time being. We can re-evaluate if someone specifically needs RawFd.

ChrisDenton · 2022-08-09T17:14:14Z

although the user clearly has to be very careful that the handle provided makes sense (i think, mainly that it is in overlapped mode for windows).

I think this is a concern. The standard library needs to know whether the read or write method should perform async or synchronous read/write.

So I think our options are:

commit to our side of process pipes always being async (and document that)
provide a way for the user to specify whether the pipe is synchronous or async (so it couldn't simply implement the From traits)
try to auto-detect the mode (using NtQueryInformationFile to get FILE_MODE_INFORMATION), but this may potentially fail.

In a reverted PR I did internally make the distinction between async and sync pipes. But the reason that was reverted was that people were already relying on it being async (via AsRawHandle). So we might effectively be promising this anyway and thus we're already committed to the first option.

vthib · 2022-08-09T20:03:52Z

I have removed the implementation for RawFd/RawHandle, keeping only those for OwnedFd/OwnedHandle.

joshtriplett · 2022-08-10T14:48:03Z

provide a way for the user to specify whether the pipe is synchronous or async (so it couldn't simply implement the From traits)

We could document that by default they're async, and perhaps add methods to OwnedHandle to change to sync/async.

ChrisDenton · 2022-08-10T15:56:09Z

We could document that by default they're async, and perhaps add methods to OwnedHandle to change to sync/async.

Hm, if we did that I do think we would need a way to both state that a handle is sync and a way to query sync/async for those that don't use the stdlib read/write methods. It would technically still be a breaking change but maybe the impact would be minimal if only user constructed ChildStd* objects could be sync.

vthib · 2022-08-18T21:46:14Z

r? libs

vthib · 2022-09-04T23:03:21Z

What is the status on this PR @joshtriplett ? I have made the changes that were asked and afaict it is simply waiting for review.

joshtriplett · 2022-09-04T23:32:53Z

So, it's already possible to get a file descriptor or handle from these. If you do that, you are going to end up relying on whether it's in sync or async mode. Given that, I think that assumption is already present, and should be documented.

However, I also think that people will commonly have a file descriptor in sync mode and want to feed it into these types.

Nonetheless, if we handle it as if it were async, checking if it's ready before using it, that shouldn't break with a sync handle (unlike the other way around).

Given that assumption, I think it's reasonable to convert the other way. But I don't think we should change the non-blocking flag; we should just operate as if we need to poll.

Does that sound reasonable, @ChrisDenton? @sunfishcode?

ChrisDenton · 2022-09-05T11:27:22Z

I have been looking at this again. From a Windows implementation perspective I think adding From implementations would be ok but it's unfortunate that this is instantly stable.

Some notes:

Currently, std's internal implementation of Read and Write for ChildStd will not work with synchronous handles. Which is fine implementation wise; there's no UB. We could enhance it to support synchronous handles but that can safely be left to future work.
As has been said, stdlib's implementation of process pipes, etc is already leaky due to existing unsafe methods so I am currently of the opinion there's not much we can do about that. People rely on it to some extent so changes already risk breaking people's code.
Third parties using ChildStd* already must assume async handles and so will break if new code creates synchronous ChildStd*. That's also fine. That could won't work either before or after the change.
I guess the only risk would be to new code taking ChildStd* and using handles in a way that assumes they're synchronous, which would now be possible although perhaps unlikely.

rfcbot · 2023-07-25T14:25:30Z

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot · 2023-08-01T15:07:57Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · 2023-08-11T15:10:59Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

dtolnay

Thanks!

dtolnay · 2023-09-28T07:14:59Z

@bors r+

bors · 2023-09-28T07:15:01Z

📌 Commit 9bdf9e7 has been approved by dtolnay

It is now in the queue for this repository.

bors · 2023-09-28T09:59:06Z

⌛ Testing commit 9bdf9e7 with merge e2d6aa7...

bors · 2023-09-28T11:46:45Z

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing e2d6aa7 to master...

rust-timer · 2023-09-28T13:36:26Z

Finished benchmarking commit (e2d6aa7): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.6%	[-2.6%, -2.6%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.5%	[4.7%, 6.5%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.917s -> 630.726s (-0.19%)
Artifact size: 317.29 MiB -> 317.32 MiB (0.01%)

Pkgsrc changes: * Remove NetBSD-8 support (embedded LLVm requires newer C++ than what is in -8; it's conceivable that this could still build with an external LLVM) * undo powerpc 9.0 file naming tweak, since we no longer support -8. * Remove patch to LLVM for powerpc now included by upstream. * Minor adjustments, checksum changes etc. Upstream changes: Version 1.74.1 (2023-12-07) =========================== - [Resolved spurious STATUS_ACCESS_VIOLATIONs in LLVM] (rust-lang/rust#118464) - [Clarify guarantees for std::mem::discriminant] (rust-lang/rust#118006) - [Fix some subtyping-related regressions] (rust-lang/rust#116415) Version 1.74.0 (2023-11-16) ========================== Language -------- - [Codify that `std::mem::Discriminant<T>` does not depend on any lifetimes in T] (rust-lang/rust#104299) - [Replace `private_in_public` lint with `private_interfaces` and `private_bounds` per RFC 2145] (rust-lang/rust#113126) Read more in [RFC 2145](https://rust-lang.github.io/rfcs/2145-type-privacy.html). - [Allow explicit `#[repr(Rust)]`] (rust-lang/rust#114201) - [closure field capturing: don't depend on alignment of packed fields] (rust-lang/rust#115315) - [Enable MIR-based drop-tracking for `async` blocks] (rust-lang/rust#107421) Compiler -------- - [stabilize combining +bundle and +whole-archive link modifiers] (rust-lang/rust#113301) - [Stabilize `PATH` option for `--print KIND=PATH`] (rust-lang/rust#114183) - [Enable ASAN/LSAN/TSAN for `*-apple-ios-macabi`] (rust-lang/rust#115644) - [Promote loongarch64-unknown-none* to Tier 2] (rust-lang/rust#115368) - [Add `i686-pc-windows-gnullvm` as a tier 3 target] (rust-lang/rust#115687) Libraries --------- - [Implement `From<OwnedFd/Handle>` for ChildStdin/out/err] (rust-lang/rust#98704) - [Implement `From<{&,&mut} [T; N]>` for `Vec<T>` where `T: Clone`] (rust-lang/rust#111278) - [impl Step for IP addresses] (rust-lang/rust#113748) - [Implement `From<[T; N]>` for `Rc<[T]>` and `Arc<[T]>`] (rust-lang/rust#114041) - [`impl TryFrom<char> for u16`] (rust-lang/rust#114065) - [Stabilize `io_error_other` feature] (rust-lang/rust#115453) - [Stabilize the `Saturating` type] (rust-lang/rust#115477) - [Stabilize const_transmute_copy] (rust-lang/rust#115520) Stabilized APIs --------------- - [`core::num::Saturating`] (https://doc.rust-lang.org/stable/std/num/struct.Saturating.html) - [`impl From<io::Stdout> for std::process::Stdio`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStdout%3E-for-Stdio) - [`impl From<io::Stderr> for std::process::Stdio`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio) - [`impl From<OwnedHandle> for std::process::Child{Stdin, Stdout, Stderr}`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio) - [`impl From<OwnedFd> for std::process::Child{Stdin, Stdout, Stderr}`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio) - [`std::ffi::OsString::from_encoded_bytes_unchecked`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.from_encoded_bytes_unchecked) - [`std::ffi::OsString::into_encoded_bytes`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.into_encoded_bytes) - [`std::ffi::OsStr::from_encoded_bytes_unchecked`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.from_encoded_bytes_unchecked) - [`std::ffi::OsStr::as_encoded_bytes`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.as_encoded_bytes) - [`std::io::Error::other`] (https://doc.rust-lang.org/stable/std/io/struct.Error.html#method.other) - [`impl TryFrom<char> for u16`] (https://doc.rust-lang.org/stable/std/primitive.u16.html#impl-TryFrom%3Cchar%3E-for-u16) - [`impl<T: Clone, const N: usize> From<&[T; N]> for Vec<T>`] (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E) - [`impl<T: Clone, const N: usize> From<&mut [T; N]> for Vec<T>`] (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26mut+%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E) - [`impl<T, const N: usize> From<[T; N]> for Arc<[T]>`] (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#impl-From%3C%5BT;+N%5D%3E-for-Arc%3C%5BT%5D,+Global%3E) - [`impl<T, const N: usize> From<[T; N]> for Rc<[T]>`] (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#impl-From%3C%5BT;+N%5D%3E-for-Rc%3C%5BT%5D,+Global%3E) These APIs are now stable in const contexts: - [`core::mem::transmute_copy`] (https://doc.rust-lang.org/beta/std/mem/fn.transmute_copy.html) - [`str::is_ascii`] (https://doc.rust-lang.org/beta/std/primitive.str.html#method.is_ascii) - [`[u8]::is_ascii`] (https://doc.rust-lang.org/beta/std/primitive.slice.html#method.is_ascii) Cargo ----- - [fix: Set MSRV for internal packages] (rust-lang/cargo#12381) - [config: merge lists in precedence order] (rust-lang/cargo#12515) - [fix(update): Clarify meaning of --aggressive as --recursive] (rust-lang/cargo#12544) - [fix(update): Make `-p` more convenient by being positional] (rust-lang/cargo#12545) - [feat(help): Add styling to help output ] (rust-lang/cargo#12578) - [feat(pkgid): Allow incomplete versions when unambigious] (rust-lang/cargo#12614) - [feat: stabilize credential-process and registry-auth] (rust-lang/cargo#12649) - [feat(cli): Add '-n' to dry-run] (rust-lang/cargo#12660) - [Add support for `target.'cfg(..)'.linker`] (rust-lang/cargo#12535) - [Stabilize `--keep-going`] (rust-lang/cargo#12568) - [feat: Stabilize lints] (rust-lang/cargo#12648) Rustdoc ------- - [Add warning block support in rustdoc] (rust-lang/rust#106561) - [Accept additional user-defined syntax classes in fenced code blocks] (rust-lang/rust#110800) - [rustdoc-search: add support for type parameters] (rust-lang/rust#112725) - [rustdoc: show inner enum and struct in type definition for concrete type] (rust-lang/rust#114855) Compatibility Notes ------------------- - [Raise minimum supported Apple OS versions] (rust-lang/rust#104385) - [make Cell::swap panic if the Cells partially overlap] (rust-lang/rust#114795) - [Reject invalid crate names in `--extern`] (rust-lang/rust#116001) - [Don't resolve generic impls that may be shadowed by dyn built-in impls] (rust-lang/rust#114941) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. None this cycle.

rust-highfive assigned kennytm Jun 30, 2022

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

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

rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 30, 2022

vthib force-pushed the impl-from-raw-for-childstd-structs branch 2 times, most recently from f0aad10 to 3ef02cd Compare July 6, 2022 14:03

joshtriplett added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jul 27, 2022

vthib force-pushed the impl-from-raw-for-childstd-structs branch from 3ef02cd to 2d9c1f9 Compare August 9, 2022 19:30

vthib changed the title ~~Implement FromRawFd/Handle for ChildStdin/out/err object~~ Implement From<OwnedFd/Handle> for ChildStdin/out/err object Aug 9, 2022

m-ou-se removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Aug 16, 2022

rust-highfive assigned joshtriplett and unassigned kennytm Aug 18, 2022

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 25, 2023

m-ou-se added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Jul 25, 2023

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Aug 1, 2023

apiraino removed the to-announce Announce this issue on triage meeting label Aug 18, 2023

dtolnay assigned dtolnay and unassigned m-ou-se Sep 28, 2023

Update stability attribute for child stream From impls

9bdf9e7

rustbot added O-unix Operating system: Unix-like O-windows Operating system: Windows labels Sep 28, 2023

dtolnay approved these changes Sep 28, 2023

View reviewed changes

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 28, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 28, 2023

bors merged commit e2d6aa7 into rust-lang:master Sep 28, 2023
11 checks passed

rustbot added this to the 1.74.0 milestone Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement From<OwnedFd/Handle> for ChildStdin/out/err object #98704

Implement From<OwnedFd/Handle> for ChildStdin/out/err object #98704

vthib commented Jun 30, 2022

rustbot commented Jun 30, 2022

rust-highfive commented Jun 30, 2022

vthib commented Jun 30, 2022

sunfishcode commented Jul 5, 2022

vthib commented Jul 6, 2022

the8472 commented Jul 27, 2022

yaahc commented Jul 27, 2022

joshtriplett commented Jul 27, 2022

joshtriplett commented Aug 9, 2022

ChrisDenton commented Aug 9, 2022

vthib commented Aug 9, 2022

joshtriplett commented Aug 10, 2022

ChrisDenton commented Aug 10, 2022

vthib commented Aug 18, 2022

vthib commented Sep 4, 2022

joshtriplett commented Sep 4, 2022 •

edited

Loading

ChrisDenton commented Sep 5, 2022

rfcbot commented Jul 25, 2023 •

edited by BurntSushi

Loading

rfcbot commented Aug 1, 2023

rfcbot commented Aug 11, 2023

dtolnay left a comment

dtolnay commented Sep 28, 2023

bors commented Sep 28, 2023

bors commented Sep 28, 2023

bors commented Sep 28, 2023

rust-timer commented Sep 28, 2023

Implement From<OwnedFd/Handle> for ChildStdin/out/err object #98704

Implement From<OwnedFd/Handle> for ChildStdin/out/err object #98704

Conversation

vthib commented Jun 30, 2022

Summary

Usecase

Questions

rustbot commented Jun 30, 2022

rust-highfive commented Jun 30, 2022

vthib commented Jun 30, 2022

sunfishcode commented Jul 5, 2022

vthib commented Jul 6, 2022

the8472 commented Jul 27, 2022

yaahc commented Jul 27, 2022

joshtriplett commented Jul 27, 2022

joshtriplett commented Aug 9, 2022

ChrisDenton commented Aug 9, 2022

vthib commented Aug 9, 2022

joshtriplett commented Aug 10, 2022

ChrisDenton commented Aug 10, 2022

vthib commented Aug 18, 2022

vthib commented Sep 4, 2022

joshtriplett commented Sep 4, 2022 • edited Loading

ChrisDenton commented Sep 5, 2022

rfcbot commented Jul 25, 2023 • edited by BurntSushi Loading

rfcbot commented Aug 1, 2023

rfcbot commented Aug 11, 2023

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay commented Sep 28, 2023

bors commented Sep 28, 2023

bors commented Sep 28, 2023

bors commented Sep 28, 2023

rust-timer commented Sep 28, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

joshtriplett commented Sep 4, 2022 •

edited

Loading

rfcbot commented Jul 25, 2023 •

edited by BurntSushi

Loading