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.

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

Abort a process when FD ownership is violated #124210

Merged
merged 3 commits into from
Apr 28, 2024

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Apr 20, 2024

When an owned FD has already been closed before it's dropped that means something else touched an FD in ways it is not allowed to. At that point things can already be arbitrarily bad, e.g. clobbered mmaps. Recovery is not possible.
All we can do is hasten the fire.

Unlike the previous attempt in #124130 this shouldn't suffer from the possibility that FUSE filesystems can return arbitrary errors.

@rustbot
Copy link
Collaborator

rustbot commented Apr 20, 2024

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 20, 2024
@rust-log-analyzer

This comment has been minimized.

When an EBADF happens then something else already touched an FD in ways it is not allowed to.
At that point things can already be arbitrarily bad, e.g. clobbered mmaps.
Recovery is not possible.
All we can do is hasten the fire.
@jhpratt
Copy link
Member

jhpratt commented Apr 20, 2024

r? libs

@rustbot rustbot assigned Mark-Simulacrum and unassigned jhpratt Apr 20, 2024
@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Apr 22, 2024

This is only applied in debug builds of std since assert_unsafe_precondition! isn't available outside core.

That comment seems outdated now? At least, this is using the ub_checks macro now, so it will honor -Zub-checks just like assert_unsafe_precondition!

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 27, 2024

📌 Commit 072c32d has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@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 Apr 27, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 27, 2024
… r=Mark-Simulacrum

Abort a process when FD ownership is violated

When an owned FD has already been closed before it's dropped that means something else touched an FD in ways it is not allowed to. At that point things can already be arbitrarily bad, e.g. clobbered mmaps. Recovery is not possible.
All we can do is hasten the fire.

Unlike the previous attempt in rust-lang#124130 this shouldn't suffer from the possibility that FUSE filesystems can return arbitrary errors.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 27, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#123942 (`x vendor`)
 - rust-lang#124165 (add test for incremental ICE: slice-pattern-const.rs rust-lang#83085)
 - rust-lang#124210 (Abort a process when FD ownership is violated)
 - rust-lang#124242 (bootstrap: Describe build_steps modules)
 - rust-lang#124406 (Remove unused `[patch]` for clippy_lints)
 - rust-lang#124429 (bootstrap: Document `struct Builder` and its fields)
 - rust-lang#124447 (Unconditionally call `really_init` on GNU/Linux)

r? `@ghost`
`@rustbot` modify labels: rollup
@workingjubilee
Copy link
Member

Died in rollup: #124452 (comment)

@bors r-

@bors 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 Apr 27, 2024
uses the same machinery as assert_unsafe_precondition
@tamird
Copy link
Contributor

tamird commented May 2, 2024

Happy to. Should I do that in the miri repo?

Depends on whether you need something changed in Miri or the standard library.

I suppose it would be in the standard library.

But if a Miri magic function to "create null FD" would be helpful, then a Miri issue sounds good. I don't quite understand how it helps or why the problem only arises under Miri, but adding such a function should be trivial so maybe I don't have to understand everything. ;)

The problem only arises under miri because otherwise we're using a standard library compiled without debug assertions.

After thinking about this for a while our use case is sufficiently niche that doing anything about it in the standard library or Miri doesn't make sense. Thanks for humoring me!

@saethlin
Copy link
Member

saethlin commented May 2, 2024

because otherwise we're using a standard library compiled without debug assertions.

This check is based on intrinsics::ub_checks, so that shouldn't matter.

@tamird
Copy link
Contributor

tamird commented May 2, 2024

because otherwise we're using a standard library compiled without debug assertions.

This check is based on intrinsics::ub_checks, so that shouldn't matter.

huh?

if core::ub_checks::check_library_ub() {
->
pub use intrinsics::ub_checks as check_library_ub;
->
pub const fn ub_checks() -> bool {
cfg!(debug_assertions)
}

what am I missing?

@RalfJung
Copy link
Member

RalfJung commented May 2, 2024

You're missing the line you cut off here:

#[rustc_intrinsic]
pub const fn ub_checks() -> bool {
cfg!(debug_assertions)
}

This is an intrinsic, the body is just the fallback when codegen backends don't overwrite it.

Our backends do overwrite it, using the value of -Cdebug-assertions at codegen time.

@tamird
Copy link
Contributor

tamird commented May 2, 2024

Oh, this probably is not specific to miri - it's just that we use nightly to run miri and stable otherwise, and this change hasn't yet hit stable. Does that sound right to you?

tamird added a commit to tamird/aya that referenced this pull request May 2, 2024
The need for this type isn't specific to Miri; it is necessary on
toolchains containing rust-lang/rust#124210 - it
just so happens that today this is nightly only, and so is Miri.
tamird added a commit to tamird/aya that referenced this pull request May 2, 2024
The need for this type isn't specific to Miri; it is necessary on
toolchains containing rust-lang/rust#124210 - it
just so happens that today this is nightly only, and so is Miri.
@saethlin
Copy link
Member

saethlin commented May 2, 2024

The first work on ub_checks just hit stable today, actually. Perfect timing.

This PR of course won't be on stable until 1.80 releases. Each PR is automatically attached to a GitHub milestone so you can see when it will hit stable.

tamird added a commit to tamird/aya that referenced this pull request May 2, 2024
The need for this type isn't specific to Miri; it is necessary on
toolchains containing rust-lang/rust#124210 - it
just so happens that today this is nightly only, and so is Miri.
tamird added a commit to tamird/aya that referenced this pull request May 2, 2024
The need for this type isn't specific to Miri; it is necessary on
toolchains containing rust-lang/rust#124210 - it
just so happens that today this is nightly only, and so is Miri.
tamird added a commit to tamird/aya that referenced this pull request May 2, 2024
The need for this type isn't specific to Miri; it is necessary on
toolchains containing rust-lang/rust#124210 - it
just so happens that today this is nightly only, and so is Miri.
tamird added a commit to tamird/aya that referenced this pull request May 3, 2024
The need for this type isn't specific to Miri; it is necessary on
toolchains containing rust-lang/rust#124210 - it
just so happens that today this is nightly only, and so is Miri.
tamird added a commit to tamird/aya that referenced this pull request May 3, 2024
The need for this type isn't specific to Miri; it is necessary on
toolchains containing rust-lang/rust#124210 - it
just so happens that today this is nightly only, and so is Miri.
tamird added a commit to tamird/aya that referenced this pull request May 3, 2024
The need for this type isn't specific to Miri; it is necessary on
toolchains containing rust-lang/rust#124210 - it
just so happens that today this is nightly only, and so is Miri.
tamird added a commit to aya-rs/aya that referenced this pull request May 3, 2024
tamird added a commit to aya-rs/aya that referenced this pull request May 3, 2024
The need for this type isn't specific to Miri; it is necessary on
toolchains containing rust-lang/rust#124210 - it
just so happens that today this is nightly only, and so is Miri.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 3, 2024
Fix HorizonOS build broken by rust-lang#124210

HorizonOS (for the Tier-3 target `armv6k-nintendo-3ds`) does not support `dirfd()`, as many other similar targets.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#124461 (handle the targets that are missing in stage0)
 - rust-lang#124492 (Generalize `adjust_from_tcx` for `Allocation`)
 - rust-lang#124588 (Use `ObligationCtxt` in favor of `TraitEngine` in many more places)
 - rust-lang#124612 (Add support for inputing via stdin with run-make-support)
 - rust-lang#124613 (Allow fmt to run on rmake.rs test files)
 - rust-lang#124649 (Fix HorizonOS build broken by rust-lang#124210)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2024
Rollup merge of rust-lang#124649 - Meziu:master, r=ChrisDenton

Fix HorizonOS build broken by rust-lang#124210

HorizonOS (for the Tier-3 target `armv6k-nintendo-3ds`) does not support `dirfd()`, as many other similar targets.
@the8472 the8472 added the relnotes Marks issues that should be documented in the release notes of the next release. label May 11, 2024
tamird added a commit to tamird/aya that referenced this pull request Jul 26, 2024
Rust 1.80 contains rust-lang/rust#124210,
causing tests which we skip under miri to segfault.
tamird added a commit to tamird/aya that referenced this pull request Jul 26, 2024
Rust 1.80 contains rust-lang/rust#124210,
causing tests which we skip under miri to segfault.
tamird added a commit to tamird/aya that referenced this pull request Jul 26, 2024
Rust 1.80 contains rust-lang/rust#124210,
causing tests which we skip under miri to segfault.
tamird added a commit to tamird/aya that referenced this pull request Jul 26, 2024
Rust 1.80 contains rust-lang/rust#124210,
causing tests which we skip under miri to segfault.
vadorovsky pushed a commit to aya-rs/aya that referenced this pull request Jul 29, 2024
Rust 1.80 contains rust-lang/rust#124210,
causing tests which we skip under miri to segfault.
abezukor added a commit to MaticianInc/bluest that referenced this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. O-unix Operating system: Unix-like perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. relnotes Marks issues that should be documented in the release notes of the next release. 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
Development

Successfully merging this pull request may close these issues.