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 potential undefined behavior in ArrayQueue initialization #500

Merged
1 commit merged into from
May 17, 2020

Conversation

caelunshun
Copy link
Contributor

@caelunshun caelunshun commented May 13, 2020

As far as I can tell, ArrayQueue::new() would previously create temporary references
to uninitialized data, which is undefined behavior. The stamps for the Slot buffer were initialized like so:

// Initialize stamps in the slots.
for i in 0..cap {
    unsafe {
        // Set the stamp to `{ lap: 0, index: i }`.
        let slot = buffer.add(i);
        ptr::write(&mut (*slot).stamp, AtomicUsize::new(i));
    }
}

As I see it, the code above creates a temporary reference to the uninitialized Slots on the call to ptr::write(), equivalent to the following:

for i in 0..cap {
    unsafe {
        // Undefined behavior: reference to uninitialized memory
        let slot: &mut Slot<T> = &mut *buffer.add(i);
        // ... 
    }
}

The problem was fixed by creating the slots vector using Iterator::collect(). This has the extra benefit of removing an unnecessary unsafe block.

The same issue was present in the bounded flavor for crossbeam-channel, to which I applied the same change.

For reference, see the nomicon:

For a struct, however, in general we do not know how it is laid out, and we also cannot use &mut base_ptr.field as that would be creating a reference. Thus, it is currently not possible to create a raw pointer to a field of a partially initialized struct, and also not possible to initialize a single field of a partially initialized struct. (A solution to this problem is being worked on.)

`ArrayQueue::new()` would previously create temporary references
to uninitialized data, which is undefined behavior.
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thank you!

@ghost ghost merged commit 010ced5 into crossbeam-rs:master May 17, 2020
@caelunshun caelunshun deleted the ub-fix branch May 17, 2020 19:16
bors bot added a commit that referenced this pull request Jan 22, 2022
774: Reduce unsafe code in array queue/bounded channel r=taiki-e a=taiki-e

Before #458, since destructors could be called on elements, the correct way was to convert the Vec to a pointer, save it, and then [deallocate it as a Vec with zero length](https://github.com/crossbeam-rs/crossbeam/blob/28ad2b7e015832b47db7e389dd9ebce3e94b3adb/crossbeam-channel/src/flavors/array.rs#L549-L552). However, after #458, destructors of the elements wrapped by MaybeUninit are not called, so there is no need to save the Vec/boxed slice as a pointer. This removes the unsafe code that has caused several (potential) unsoundnesses in the past (#500, #533, #763).


Co-authored-by: Taiki Endo <te316e89@gmail.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant