Mark `-1` as an available niche for file descriptors by notriddle · Pull Request #74699 · rust-lang/rust

notriddle · 2020-07-23T22:15:18Z

Based on discussion from https://internals.rust-lang.org/t/can-the-standard-library-shrink-option-file/12768, the file descriptor -1 is chosen based on the POSIX API designs that use it as a sentinel to report errors. A bigger niche could've been chosen, particularly on Linux, but would not necessarily be portable.

This PR also adds a test case to ensure that the -1 niche (which is kind of hacky and has no obvious test case) works correctly. It requires the "upper" bound, which is actually -1, to be expressed in two's complement.

rust-highfive · 2020-07-23T22:15:21Z

r? @kennytm

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

src/libstd/sys/unix/fd.rs

src/test/ui/print_type_sizes/niche-filling.rs

joshtriplett · 2020-07-23T22:36:15Z

I posted a few comments; otherwise this looks good to me.

joshtriplett · 2020-07-23T22:37:42Z

Draft release notes text:

On Unix platforms, the `std::fs::File` type now has a "niche" of `-1`, which cannot be a valid file descriptor.
This means `Option<File>` now takes up no more space than `File`.

notriddle · 2020-07-23T22:58:18Z

This should probably get a crater run, too. The FromRawFd doc comment ought to ban invalid file descriptors, but ... you know, any externally visible behaviour might as well be part of the API contract.

jstarks · 2020-07-24T00:44:15Z

Is it worth doing this for Windows (but with 0/NULL) in the same change?

retep998 · 2020-07-24T03:25:55Z

Windows generally uses INVALID_HANDLE_VALUE to represent invalid handles, which should be fine for file handles. For other types of handles it sometimes has special meaning beyond just being an invalid handle. Like for process handles GetCurrentProcess returns a pseudo handle of the same value as INVALID_HANDLE_VALUE and functions which take a process handle will interpret that as the current process.

jstarks · 2020-07-24T04:49:41Z

The Win32 APIs conventionally return INVALID_HANDLE_VALUE (-1) for file handles and only file handles. This is just a convention, and as you point out there are some collisions with pseudo handles used for other purposes. This was done for compatibility with 16-bit Windows code.

Non-file APIs such as CreateEvent return NULL on failure. INVALID_HANDLE_VALUE is probably used in a few other esoteric Win32 APIs by people who didn't understand the history.

But I'd say let's keep it simple. NULL/0 is never a valid handle in any context, file or otherwise. It's the conventional "no handle" value in Windows kernel code. I think it's a better niche for this purpose than -1.

src/libstd/sys/unix/fd.rs

RalfJung · 2020-07-24T10:49:04Z

Is it worth doing this for Windows (but with 0/NULL) in the same change?

It is an entirely separate discussion (requiring knowledge about an independent set of APIs) so I propose this should be done in a separate PR.

the8472 · 2020-07-24T14:37:04Z

Note that some APIs might use -1 as a "no file" placeholder. At least io_uring does. Of course this could be hanlded with a separate type, but then RawFd would not be all that raw anymore.

Edit: nvm, I mistook FileDesc for RawFd

src/libstd/sys/unix/fd.rs

oli-obk · 2020-07-25T14:31:32Z

For the crater, this would need a full test run crater, right? Not just a check, since successful compilation is not the issue here.

programmerjake · 2020-07-26T17:09:57Z

It seems worthwhile to explicitly add a test or a comment in whatever pre-existing test that asserts c_int is i32, that way, if some platform has a different c_int they'd be less likely to miss changing the bounds for File.

birkenfeld · 2020-07-26T18:23:56Z

Alternatively, make it possible in rustc to specify the bound as -2 which should keep the correct semantics even for a 64 bit c_int.

bors · 2020-07-28T03:15:54Z

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

notriddle · 2020-07-28T04:26:58Z

Manually rebased.

oli-obk · 2020-07-28T05:37:07Z

@bors try

bors · 2020-07-28T05:37:18Z

⌛ Trying commit e918808c2c27141008ab89875137727603cdc305 with merge 68da184b2b3962d69cccd2f8a627dd35a0c2a1e7...

Dylan-DPC-zz · 2020-12-08T12:31:43Z

@Amanieu @KodrAus @withoutboats this is waiting on your votes/concerns

rfcbot · 2020-12-09T01:38:09Z

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

vi · 2020-12-10T15:17:16Z

library/std/src/sys/unix/fd.rs

Will there be unsafe fn new_unchecked, to avoid including panicking code in a program that otherwise avoid it?

If this is a private type, not directly available in public API, will panicking and string formatting code suddenly appear in places where it was absent before, breaking things like no-panic?

Based on discussion from https://internals.rust-lang.org/t/can-the-standard-library-shrink-option-file/12768, the file descriptor -1 is chosen based on the POSIX API designs that use it as a sentinel to report errors. A bigger niche could've been chosen, particularly on Linux, but would not necessarily be portable. This PR also adds a test case to ensure that the -1 niche (which is kind of hacky and has no obvious test case) works correctly. It requires the "upper" bound, which is actually -1, to be expressed in two's complement.

Co-authored-by: Elichai Turkel <elichai.turkel@gmail.com>

pickfire · 2020-12-17T01:59:11Z

File::from_raw_fd(-1) will panic but it is possible to have the compiler emit an error during compilation if the compiler knew it was -1? Like compile time out of bounds check that we have now.

rfcbot · 2020-12-19T01:43:54Z

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.

The RFC will be merged soon.

m-ou-se · 2020-12-20T13:13:43Z

@bors r+

bors · 2020-12-20T13:13:44Z

📌 Commit 094b1da has been approved by m-ou-se

bors · 2020-12-20T16:36:29Z

⌛ Testing commit 094b1da with merge b0e5c7d...

bors · 2020-12-20T19:32:51Z

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing b0e5c7d to master...

Pkgsrc changes: * Adjust patches, re-compute line offsets, fix capitalization. * Remove i686/FreeBSD support, no longer provided upstream. * Bump bootstraps to 1.49.0. * Change USE_TOOLS from bsdtar to gtar. * Reduce diffs to pkgsrc-wip package patches. * Allow rust.BUILD_TARGET to override automatic choice of target. * Add an i586/NetBSD (pentium) bootstrap variant (needs testing), not yet added as bootstrap since 1.49 doesn't have that variant. Upstream changes: Version 1.50.0 (2021-02-11) ============================ Language ----------------------- - [You can now use `const` values for `x` in `[x; N]` array expressions.][79270] This has been technically possible since 1.38.0, as it was unintentionally stabilized. - [Assignments to `ManuallyDrop<T>` union fields are now considered safe.][78068] Compiler ----------------------- - [Added tier 3\* support for the `armv5te-unknown-linux-uclibceabi` target.][78142] - [Added tier 3 support for the `aarch64-apple-ios-macabi` target.][77484] - [The `x86_64-unknown-freebsd` is now built with the full toolset.][79484] \* Refer to Rust's [platform support page][forge-platform-support] for more information on Rust's tiered platform support. Libraries ----------------------- - [`proc_macro::Punct` now implements `PartialEq<char>`.][78636] - [`ops::{Index, IndexMut}` are now implemented for fixed sized arrays of any length.][74989] - [On Unix platforms, the `std::fs::File` type now has a "niche" of `-1`.][74699] This value cannot be a valid file descriptor, and now means `Option<File>` takes up the same amount of space as `File`. Stabilized APIs --------------- - [`bool::then`] - [`btree_map::Entry::or_insert_with_key`] - [`f32::clamp`] - [`f64::clamp`] - [`hash_map::Entry::or_insert_with_key`] - [`Ord::clamp`] - [`RefCell::take`] - [`slice::fill`] - [`UnsafeCell::get_mut`] The following previously stable methods are now `const`. - [`IpAddr::is_ipv4`] - [`IpAddr::is_ipv6`] - [`Layout::size`] - [`Layout::align`] - [`Layout::from_size_align`] - `pow` for all integer types. - `checked_pow` for all integer types. - `saturating_pow` for all integer types. - `wrapping_pow` for all integer types. - `next_power_of_two` for all unsigned integer types. - `checked_power_of_two` for all unsigned integer types. Cargo ----------------------- - [Added the `[build.rustc-workspace-wrapper]` option.][cargo/8976] This option sets a wrapper to execute instead of `rustc`, for workspace members only. - [`cargo:rerun-if-changed` will now, if provided a directory, scan the entire contents of that directory for changes.][cargo/8973] - [Added the `--workspace` flag to the `cargo update` command.][cargo/8725] Misc ---- - [The search results tab and the help button are focusable with keyboard in rustdoc.][79896] - [Running tests will now print the total time taken to execute.][75752] Compatibility Notes ------------------- - [The `compare_and_swap` method on atomics has been deprecated.][79261] It's recommended to use the `compare_exchange` and `compare_exchange_weak` methods instead. - [Changes in how `TokenStream`s are checked have fixed some cases where you could write unhygenic `macro_rules!` macros.][79472] - [`#![test]` as an inner attribute is now considered unstable like other inner macro attributes, and reports an error by default through the `soft_unstable` lint.][79003] - [Overriding a `forbid` lint at the same level that it was set is now a hard error.][78864] - [Dropped support for all cloudabi targets.][78439] - [You can no longer intercept `panic!` calls by supplying your own macro.][78343] It's recommended to use the `#[panic_handler]` attribute to provide your own implementation. - [Semi-colons after item statements (e.g. `struct Foo {};`) now produce a warning.][78296] [74989]: rust-lang/rust#74989 [79261]: rust-lang/rust#79261 [79896]: rust-lang/rust#79896 [79484]: rust-lang/rust#79484 [79472]: rust-lang/rust#79472 [79270]: rust-lang/rust#79270 [79003]: rust-lang/rust#79003 [78864]: rust-lang/rust#78864 [78636]: rust-lang/rust#78636 [78439]: rust-lang/rust#78439 [78343]: rust-lang/rust#78343 [78296]: rust-lang/rust#78296 [78068]: rust-lang/rust#78068 [75752]: rust-lang/rust#75752 [74699]: rust-lang/rust#74699 [78142]: rust-lang/rust#78142 [77484]: rust-lang/rust#77484 [cargo/8976]: rust-lang/cargo#8976 [cargo/8973]: rust-lang/cargo#8973 [cargo/8725]: rust-lang/cargo#8725 [`IpAddr::is_ipv4`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv4 [`IpAddr::is_ipv6`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv6 [`Layout::align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.align [`Layout::from_size_align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.from_size_align [`Layout::size`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.size [`Ord::clamp`]: https://doc.rust-lang.org/stable/std/cmp/trait.Ord.html#method.clamp [`RefCell::take`]: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.take [`UnsafeCell::get_mut`]: https://doc.rust-lang.org/stable/std/cell/struct.UnsafeCell.html#method.get_mut [`bool::then`]: https://doc.rust-lang.org/stable/std/primitive.bool.html#method.then [`btree_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/btree_map/enum.Entry.html#method.or_insert_with_key [`f32::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f32.html#method.clamp [`f64::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f64.html#method.clamp [`hash_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/hash_map/enum.Entry.html#method.or_insert_with_key [`slice::fill`]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.fill

rust-highfive assigned kennytm Jul 23, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 23, 2020

notriddle force-pushed the fd-non-negative branch 2 times, most recently from 33e3333 to 7590613 Compare July 23, 2020 22:22

joshtriplett reviewed Jul 23, 2020

View reviewed changes

src/libstd/sys/unix/fd.rs Outdated Show resolved Hide resolved

joshtriplett reviewed Jul 23, 2020

View reviewed changes

src/libstd/sys/unix/fd.rs Outdated Show resolved Hide resolved

joshtriplett reviewed Jul 23, 2020

View reviewed changes

src/test/ui/print_type_sizes/niche-filling.rs Outdated Show resolved Hide resolved

joshtriplett reviewed Jul 23, 2020

View reviewed changes

src/test/ui/print_type_sizes/niche-filling.rs Outdated Show resolved Hide resolved

joshtriplett added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 23, 2020

RalfJung reviewed Jul 24, 2020

View reviewed changes

src/libstd/sys/unix/fd.rs Outdated Show resolved Hide resolved

sfackler mentioned this pull request Jul 24, 2020

Document FromRawFd unsafety #72175

Closed

birkenfeld reviewed Jul 24, 2020

View reviewed changes

src/libstd/sys/unix/fd.rs Outdated Show resolved Hide resolved

oli-obk approved these changes Jul 25, 2020

View reviewed changes

notriddle force-pushed the fd-non-negative branch from 9d86aac to e918808 Compare July 28, 2020 04:26

m-ou-se added the S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. label Dec 7, 2020

rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Dec 9, 2020

rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Dec 9, 2020

vi reviewed Dec 10, 2020

View reviewed changes

notriddle and others added 3 commits December 10, 2020 13:31

Add safety note to library/std/src/sys/unix/fd.rs

a50811a

Co-authored-by: Elichai Turkel <elichai.turkel@gmail.com>

Fix fd test case

08b70ed

notriddle force-pushed the fd-non-negative branch from 8b124ec to 08b70ed Compare December 10, 2020 22:05

Check that c_int is i32 in FileDesc::new.

094b1da

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 20, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 20, 2020

bors merged commit b0e5c7d into rust-lang:master Dec 20, 2020

rustbot added this to the 1.50.0 milestone Dec 20, 2020

notriddle deleted the fd-non-negative branch December 20, 2020 21:45

spastorino removed the to-announce Announce this issue on triage meeting label Dec 31, 2020

tmandry mentioned this pull request Jan 21, 2021

Fix file descriptor ranges for fuchsia #81226

Closed

RalfJung mentioned this pull request May 30, 2021

RFC: I/O Safety rust-lang/rfcs#3128

Merged

sunfishcode mentioned this pull request Jun 1, 2021

Docs don't match type niche sunfishcode/io-lifetimes#1

Closed

Uh oh!

Conversation

notriddle commented Jul 23, 2020

Uh oh!

rust-highfive commented Jul 23, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joshtriplett commented Jul 23, 2020

Uh oh!

joshtriplett commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notriddle commented Jul 23, 2020

Uh oh!

jstarks commented Jul 24, 2020

Uh oh!

retep998 commented Jul 24, 2020

Uh oh!

jstarks commented Jul 24, 2020

Uh oh!

Uh oh!

RalfJung commented Jul 24, 2020

Uh oh!

the8472 commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

oli-obk commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

programmerjake commented Jul 26, 2020

Uh oh!

birkenfeld commented Jul 26, 2020

Uh oh!

bors commented Jul 28, 2020

Uh oh!

notriddle commented Jul 28, 2020

Uh oh!

oli-obk commented Jul 28, 2020

Uh oh!

bors commented Jul 28, 2020

Uh oh!

Dylan-DPC-zz commented Dec 8, 2020

Uh oh!

rfcbot commented Dec 9, 2020

Uh oh!

vi Dec 10, 2020 • 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!

pickfire commented Dec 17, 2020

Uh oh!

rfcbot commented Dec 19, 2020

Uh oh!

m-ou-se commented Dec 20, 2020

Uh oh!

bors commented Dec 20, 2020

Uh oh!

bors commented Dec 20, 2020

Uh oh!

bors commented Dec 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

joshtriplett commented Jul 23, 2020 •

edited

Loading

the8472 commented Jul 24, 2020 •

edited

Loading

oli-obk commented Jul 25, 2020 •

edited

Loading

vi Dec 10, 2020 •

edited

Loading