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

Fix soundness hole in FSA due to nested drops #146

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

jacob-hughes
Copy link
Collaborator

Imagine a Gc<T> where T has a finalizer. Up until now, a value v allocated locally inside T::drop could create a backdoor where non-finalizer-safe stuff can happen without FSA knowing about it. Consider the following example:

struct S { .. }

impl Drop for S {
    fn drop(&mut self) {
        unsound_external_fn_call(); // called from finalizer.
    }
}

struct T { .. }

impl Drop for T {
    fn drop(&mut self) {
        let s = S { .. };
    } // also calls 'S::drop' for the local 's'.
}

fn main() { Gc::new(T::new()) }

This program is unsound, but it would not be caught by FSA. By allowing the alloca of s, we have inadvertently run the unsound nested S::drop inside a finalizer without checking.

Note that my use of the term 'nested drops' here is distinct from drop glue. Drop glue (i.e. where the drop methods for fields of a type are automatically called too) works as intended.

This commit updates FSA to handle such nested drops. The approach is very similar to how we handle drop methods which contain functions calls. Using the same example from above, we know that the MIR for T::drop (the type being finalized) will contain a terminator to call S::drop, so after FSA has finished looking at T::drop, it will obtain and then check the (potentially monomorphized) MIR for S::drop.

This fix also works when S has drop glue, because that would mean that drop methods for any of S's fields would also be added as terminators in the MIR for T::drop.

If at any stage, the MIR for S::drop is unavailable, then we emit an FSA error.

Imagine a `Gc<T>` where `T` has a finalizer. Up until now, a value `v`
allocated locally inside `T::drop` could create a backdoor where
non-finalizer-safe stuff can happen without FSA knowing about it.
Consider the following example:

```rust

struct S { .. }
impl Drop for S {
    fn drop(&mut self) {
        ... // something unsound if called from finalizer.
    }
}

struct T { .. }

impl Drop for T {
    fn drop(&mut self) {
        let s = S { .. };
    } // also calls 'S::drop' for the local 's'.
}

fn main() { Gc::new(T::new()) }

```

This program is unsound, but it would *not* be caught by FSA. By
allowing the alloca of `s`, we have inadvertently run the unsound nested
`S::drop` inside a finalizer without checking.

Note that my use of the term 'nested drops' here is distinct from drop
glue. Drop glue (i.e. where the drop methods for fields of a type are
automatically called too) works as intended.

This commit updates FSA to handle such nested drops. The approach is
very similar to how we handle drop methods which contain functions
calls. Using the same example from above, we know that the MIR for
`T::drop` (the type being finalized) will contain a terminator to call
`S::drop`, so after FSA has finished looking at `T::drop`, it will
obtain and then check the (potentially monomorphized) MIR for `S::drop`.

This fix also works when `S` has drop glue, because that would mean that
drop methods for any of `S`'s fields would also be added as terminators
in the MIR for `T::drop`.

If at any stage, the MIR for S::drop (or any other drop methods in its
drop glue, or anything it calls) is unavailable, then we emit an FSA
error.
With inline assembly you can do just about anything, so all bets are
off. Deny!
@ltratt ltratt added this pull request to the merge queue Nov 12, 2024
Merged via the queue into softdevteam:master with commit bcc4292 Nov 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants