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

channel: Handle possible allocation failure in list::Block::new. #1147

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented Nov 8, 2024

std::alloc::alloc_zeroed can fail by returning nullptr, which is UB to pass to Box::from_raw.

This checks the returned pointer for null, and if so calls std::alloc::handle_alloc_error which diverges.

Also adds a should-never-fail assertion that layout is non-zero-sized, which is a precondition of alloc_zeroed.

cc @cuviper #1146

MIRI reproducer
use std::{
    alloc::{GlobalAlloc, System},
    sync::atomic::{AtomicUsize, Ordering},
};

struct FailNthAllocation {
    /// The allocation that decrements this from 1 to 0 should fail
    countdown: AtomicUsize,
}

impl FailNthAllocation {
    /// Makes a new `FailsNthAllocation` that will fail the `n`th (1-indexed) allocation.
    ///
    /// If `n == 0`, the returned allocator will not inject any failures.
    const fn new(n: usize) -> Self {
        Self {
            countdown: AtomicUsize::new(n),
        }
    }

    fn should_fail(&self) -> bool {
        self.countdown
            .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |count| {
                count.checked_sub(1)
            })
            .is_ok_and(|previous_value| previous_value == 1)
    }
}

unsafe impl GlobalAlloc for FailNthAllocation {
    unsafe fn alloc(&self, layout: std::alloc::Layout) -> *mut u8 {
        if self.should_fail() {
            return std::ptr::null_mut();
        }
        System.alloc(layout)
    }

    unsafe fn dealloc(&self, ptr: *mut u8, layout: std::alloc::Layout) {
        System.dealloc(ptr, layout)
    }
}

#[global_allocator]
static ALLOCATOR: FailNthAllocation = FailNthAllocation::new(2);

fn main() {
    let (s, _r) = crossbeam_channel::unbounded::<u32>();
    let _ = s.send(42);
}
$ cargo add crossbeam-channel --git https://github.com/crossbeam-rs/crossbeam
$ cargo +nightly miri run
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
     Running `/home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner /home/zachary/opt_mount/zachary/cargo-target/miri/x86_64-unknown-linux-gnu/debug/crossbeam-nullptr`
error: Undefined Behavior: constructing invalid value: encountered 0, but expected something greater or equal to 1
  --> /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/unique.rs:90:36
   |
90 |         unsafe { Unique { pointer: NonNull::new_unchecked(ptr), _marker: PhantomData } }
   |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered 0, but expected something greater or equal to 1
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
   = note: BACKTRACE:
   = note: inside `std::ptr::Unique::<crossbeam_channel::flavors::list::Block<u32>>::new_unchecked` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/unique.rs:90:36: 90:63
   = note: inside `std::boxed::Box::<crossbeam_channel::flavors::list::Block<u32>>::from_raw_in` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:1167:22: 1167:48
   = note: inside `std::boxed::Box::<crossbeam_channel::flavors::list::Block<u32>>::from_raw` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:1058:18: 1058:48
   = note: inside `crossbeam_channel::flavors::list::Block::<u32>::new` at /home/zachary/.cargo/git/checkouts/crossbeam-5d5b005504a37dac/d2c53ef/crossbeam-channel/src/flavors/list.rs:85:18: 85:60
   = note: inside `crossbeam_channel::flavors::list::Channel::<u32>::start_send` at /home/zachary/.cargo/git/checkouts/crossbeam-5d5b005504a37dac/d2c53ef/crossbeam-channel/src/flavors/list.rs:233:41: 233:58
   = note: inside `crossbeam_channel::flavors::list::Channel::<u32>::send` at /home/zachary/.cargo/git/checkouts/crossbeam-5d5b005504a37dac/d2c53ef/crossbeam-channel/src/flavors/list.rs:428:17: 428:39
   = note: inside `crossbeam_channel::Sender::<u32>::send` at /home/zachary/.cargo/git/checkouts/crossbeam-5d5b005504a37dac/d2c53ef/crossbeam-channel/src/channel.rs:444:41: 444:61
note: inside `main`
  --> src/main.rs:48:13
   |
48 |     let _ = s.send(42);
   |             ^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

With this patch, we get an (expected) abort instead of UB:

$ cargo add crossbeam-channel --git https://github.com/zachs18/crossbeam --branch handle_allocation_failure
$ cargo +nightly miri run
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
     Running `/home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner /home/zachary/opt_mount/zachary/cargo-target/miri/x86_64-unknown-linux-gnu/debug/crossbeam-nullptr`
memory allocation of 504 bytes failed
error: abnormal termination: the program aborted execution
   --> /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/pal/unix/mod.rs:373:14
    |
373 |     unsafe { libc::abort() }
    |              ^^^^^^^^^^^^^ the program aborted execution
    |
    = note: BACKTRACE:
    = note: inside `std::sys::pal::unix::abort_internal` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/pal/unix/mod.rs:373:14: 373:27
    = note: inside `std::process::abort` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/process.rs:2374:5: 2374:33
    = note: inside `std::alloc::rust_oom` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/alloc.rs:376:5: 376:28
    = note: inside `std::alloc::_::__rg_oom` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/alloc.rs:371:1: 371:37
    = note: inside `std::alloc::handle_alloc_error::rt_error` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:403:13: 403:70
    = note: inside `std::alloc::handle_alloc_error` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/alloc.rs:409:9: 409:75
    = note: inside `crossbeam_channel::flavors::list::Block::<u32>::new` at /home/zachary/.cargo/git/checkouts/crossbeam-e091cf7cf0ef731e/988e165/crossbeam-channel/src/flavors/list.rs:83:13: 83:39
    = note: inside `crossbeam_channel::flavors::list::Channel::<u32>::start_send` at /home/zachary/.cargo/git/checkouts/crossbeam-e091cf7cf0ef731e/988e165/crossbeam-channel/src/flavors/list.rs:240:41: 240:58
    = note: inside `crossbeam_channel::flavors::list::Channel::<u32>::send` at /home/zachary/.cargo/git/checkouts/crossbeam-e091cf7cf0ef731e/988e165/crossbeam-channel/src/flavors/list.rs:435:17: 435:39
    = note: inside `crossbeam_channel::Sender::<u32>::send` at /home/zachary/.cargo/git/checkouts/crossbeam-e091cf7cf0ef731e/988e165/crossbeam-channel/src/channel.rs:444:41: 444:61
note: inside `main`
   --> src/main.rs:48:13
    |
48  |     let _ = s.send(42);
    |             ^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

@cuviper
Copy link
Contributor

cuviper commented Nov 8, 2024

Ouch, thanks for catching this so quickly!

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

This is the second time this mistake happens with crossbeam repo (#690) and perhaps we should introduce a safe helper that returns Option<NonNull>...

@taiki-e taiki-e merged commit 46fe32b into crossbeam-rs:master Nov 8, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants