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

Use Box<[T]> instead of Vec<T> to initialize and drop ArrayQueue #533

Merged
1 commit merged into from
Aug 4, 2020

Conversation

caelunshun
Copy link
Contributor

@caelunshun caelunshun commented Jun 26, 2020

Previously, the queue buffer was initialized using Vec::from_iter through collect. Note that collect does not guarantee a precise capacity; the layout of the allocated memory could have a greater size.

ArrayVec::drop assumed the buffer has a size equal to the queue's capacity. This is undefined behavior when the buffer actually has a different size thanks to Vec.

Using Box<[T]> instead guarantees an exact capacity, resolving the UB.

(The same fix should probably be applied to concurrent-queue as it suffers from the same bug.)

Previously, the queue buffer was initialized using Vec::with_capacity.
Note the with_capacity does not guarantee that precise capacity;
the actual layout of the allocated memory could have a greater size.

The drop code assumed the buffer has a size
equal to the queue's capacity. This is undefined
behavior when the buffer actually has a greater size.

Using Box<[T]> instead guarantees an exact capacity, resolving the UB.
@caelunshun caelunshun force-pushed the no-vec-with-capacity branch from 769c195 to be327d5 Compare June 26, 2020 04:26
@Gilnaa
Copy link

Gilnaa commented Jul 2, 2020

The FromIterator impl that collect uses for Box<[T]> is

impl<A> FromIterator<A> for Box<[A]> {
    fn from_iter<T: IntoIterator<Item = A>>(iter: T) -> Self {
        iter.into_iter().collect::<Vec<_>>().into_boxed_slice()
    }
}

I might be wrong, but wouldn't this also result in bigger than needed allocation?

@caelunshun
Copy link
Contributor Author

caelunshun commented Jul 2, 2020

The FromIterator impl that collect uses for Box<[T]> is

impl<A> FromIterator<A> for Box<[A]> {
    fn from_iter<T: IntoIterator<Item = A>>(iter: T) -> Self {
        iter.into_iter().collect::<Vec<_>>().into_boxed_slice()
    }
}

I might be wrong, but wouldn't this also result in bigger than needed allocation?

into_boxed_slice will shrink the allocation's size to the length of the slice:

unsafe {
    self.shrink_to_fit();
    let me = ManuallyDrop::new(self);
    let buf = ptr::read(&me.buf);
    let len = me.len();
    buf.into_box(len).assume_init()
}

Notice the call to shrink_to_fit.

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 772b4f3 into crossbeam-rs:master Aug 4, 2020
bors bot added a commit to nervosnetwork/ckb that referenced this pull request Sep 1, 2020
2244: refactor: re-export crossbeam-channel r=doitian,yangby-cryptape a=zhangsoledad

## Description

re-export crossbeam-channel from facade wrapper, unify version specify.
use tilde requirements specify for crossbeam-channel, prevent automate dependency updates.

crossbeam-channel 0.4.3 has UB, may lead to jemalloc deadlock.

```
    frame #0: 0x00007fb34acd5620 libpthread.so.0`__lll_lock_wait + 48
    frame #1: 0x00007fb34accddf3 libpthread.so.0`__pthread_mutex_lock + 227
    frame #2: 0x000056423ada57fc ckb`_rjem_je_malloc_mutex_lock_slow at mutex.h:141:2
    frame #3: 0x000056423ada57f4 ckb`_rjem_je_malloc_mutex_lock_slow at mutex.c:84
    frame #4: 0x000056423ad6e1a8 ckb`_rjem_je_arena_tcache_fill_small at mutex.h:205:4
    frame #5: 0x000056423ad6e1a0 ckb`_rjem_je_arena_tcache_fill_small at arena.c:1261
    frame #6: 0x000056423adc1494 ckb`_rjem_je_tcache_alloc_small_hard at tcache.c:93:2
    frame #7: 0x000056423ad68618 ckb`mallocx at tcache_inlines.h:60:9
    frame #8: 0x000056423ad685e0 ckb`mallocx at jemalloc.c:1709
    frame #9: 0x000056423ad685e0 ckb`mallocx at jemalloc.c:1905
    frame #10: 0x000056423ad685e0 ckb`mallocx at jemalloc.c:2005
    frame #11: 0x000056423ad68324 ckb`mallocx at jemalloc.c:2588
    frame #12: 0x000056423b16a56c ckb`alloc::raw_vec::RawVec$LT$T$C$A$GT$::reserve::h10a313223974a7d1 + 188
    frame #13: 0x000056423b1a25f9 ckb`crossbeam_channel::flavors::array::Channel$LT$T$GT$::with_capacity::had6f123b9bc217e1 + 121
```
crossbeam-rs/crossbeam#533
#2235

also crossbeam-channel is not optimize for oneshot case. replace it if it is convenient.


Co-authored-by: zhangsoledad <[email protected]>
@taiki-e
Copy link
Member

taiki-e commented Oct 1, 2020

@rocallahan crossbeam-channel 0.4.4 is like removing the commit that introduced the UB fixed by this PR from 0.4.3.
So "incorrect layout on deallocation" UB shouldn't exist in that version. (Could you check the output of cargo tree | grep 'crossbeam-channel'?)

@rocallahan
Copy link

Yes, I just figured that out and deleted my comment, sorry.

I was confused because I couldn't find the commit from this PR under the crossbeam-channel-0.4.4 tag. I also looked at the 0.4.4 crates.io source and misunderstood the code there. Thanks!

@rocallahan
Copy link

rocallahan commented Oct 1, 2020

I do think it would be good if fixes for critical bugs like this can make it into a release more quickly. It was more than two months from this bug being discovered and the fix submitted (late June) to 0.4.4 being released and 0.4.3 being yanked (early September). We updated to 0.4.3 in the meantime and I guess a lot of other projects did too.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 7, 2020
It's used by both webrender and fog, and it contains a subtle soundness
issue which may affect us, see:

 * crossbeam-rs/crossbeam#533
 * https://twitter.com/khuey_/status/1311641831201857537

Quoting for posterity:

> There is a 0.4.4 on a branch and it contains a reversion for the UB
> mentioned in crossbeam-rs/crossbeam#533.
>
> This was causing corruption of jemalloc structures (and ultimately a
> deadlock) for us.

Update the crate resolving the issue.

Differential Revision: https://phabricator.services.mozilla.com/D92046
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 7, 2020
It's used by both webrender and fog, and it contains a subtle soundness
issue which may affect us, see:

 * crossbeam-rs/crossbeam#533
 * https://twitter.com/khuey_/status/1311641831201857537

Quoting for posterity:

> There is a 0.4.4 on a branch and it contains a reversion for the UB
> mentioned in crossbeam-rs/crossbeam#533.
>
> This was causing corruption of jemalloc structures (and ultimately a
> deadlock) for us.

Update the crate resolving the issue.

Differential Revision: https://phabricator.services.mozilla.com/D92046
@tomrittervg
Copy link

Would it be possible to request a CVE from Github for this issue?

@taiki-e
Copy link
Member

taiki-e commented Oct 11, 2020

@tomrittervg I've filed advisory in rustsec/advisory-db#425. (And I'll request a CVE from Github.)

sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this pull request Oct 13, 2020
It's used by both webrender and fog, and it contains a subtle soundness
issue which may affect us, see:

 * crossbeam-rs/crossbeam#533
 * https://twitter.com/khuey_/status/1311641831201857537

Quoting for posterity:

> There is a 0.4.4 on a branch and it contains a reversion for the UB
> mentioned in crossbeam-rs/crossbeam#533.
>
> This was causing corruption of jemalloc structures (and ultimately a
> deadlock) for us.

Update the crate resolving the issue.

Differential Revision: https://phabricator.services.mozilla.com/D92046
@taiki-e
Copy link
Member

taiki-e commented Oct 14, 2020

Security advisory is now published as RUSTSEC-2020-0052 / GHSA-v5m7-53cv-f3hx / CVE-2020-15254.

@jdm jdm mentioned this pull request Nov 21, 2020
3 tasks
bors-servo added a commit to servo/servo that referenced this pull request Nov 21, 2020
Update past yanked crate.

crossbeam-channel 0.4.3 was yanked in October and 0.4.4 includes crossbeam-rs/crossbeam#533.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes
bors-servo added a commit to servo/servo that referenced this pull request Nov 22, 2020
Update past yanked crate.

crossbeam-channel 0.4.3 was yanked in October and 0.4.4 includes crossbeam-rs/crossbeam#533.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] There are tests for these changes
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 <[email protected]>
CjS77 added a commit to tari-project/broadcast_channel that referenced this pull request Jun 7, 2022
Impact
The affected version of this crate's the bounded channel incorrectly assumes that Vec::from_iter has allocated capacity that same as the number of iterator elements. Vec::from_iter does not actually guarantee that and may allocate extra memory. The destructor of the bounded channel reconstructs Vec from the raw pointer based on the incorrect assumes described above. This is unsound and causing deallocation with the incorrect capacity when Vec::from_iter has allocated different sizes with the number of iterator elements.

Patches
This has been fixed in crossbeam-channel 0.4.4.

We recommend users to upgrade to 0.4.4.

References
See crossbeam-rs/crossbeam#533, crossbeam-rs/crossbeam#539, and rustsec/advisory-db#425 for more details.
CjS77 added a commit to tari-project/broadcast_channel that referenced this pull request Jun 7, 2022
Impact
The affected version of this crate's the bounded channel incorrectly assumes that Vec::from_iter has allocated capacity that same as the number of iterator elements. Vec::from_iter does not actually guarantee that and may allocate extra memory. The destructor of the bounded channel reconstructs Vec from the raw pointer based on the incorrect assumes described above. This is unsound and causing deallocation with the incorrect capacity when Vec::from_iter has allocated different sizes with the number of iterator elements.

Patches
This has been fixed in crossbeam-channel 0.4.4.

We recommend users to upgrade to 0.4.4.

References
See crossbeam-rs/crossbeam#533, crossbeam-rs/crossbeam#539, and rustsec/advisory-db#425 for more details.
CjS77 added a commit to tari-project/broadcast_channel that referenced this pull request Jun 7, 2022
Impact
The affected version of this crate's the bounded channel incorrectly assumes that Vec::from_iter has allocated capacity that same as the number of iterator elements. Vec::from_iter does not actually guarantee that and may allocate extra memory. The destructor of the bounded channel reconstructs Vec from the raw pointer based on the incorrect assumes described above. This is unsound and causing deallocation with the incorrect capacity when Vec::from_iter has allocated different sizes with the number of iterator elements.

Patches
This has been fixed in crossbeam-channel 0.4.4.

We recommend users to upgrade to 0.4.4.

References
See crossbeam-rs/crossbeam#533, crossbeam-rs/crossbeam#539, and rustsec/advisory-db#425 for more details.
CjS77 added a commit to tari-project/broadcast_channel that referenced this pull request Jun 7, 2022
Impact
The affected version of this crate's the bounded channel incorrectly assumes that Vec::from_iter has allocated capacity that same as the number of iterator elements. Vec::from_iter does not actually guarantee that and may allocate extra memory. The destructor of the bounded channel reconstructs Vec from the raw pointer based on the incorrect assumes described above. This is unsound and causing deallocation with the incorrect capacity when Vec::from_iter has allocated different sizes with the number of iterator elements.

Patches
This has been fixed in crossbeam-channel 0.4.4.

We recommend users to upgrade to 0.4.4.

References
See crossbeam-rs/crossbeam#533, crossbeam-rs/crossbeam#539, and rustsec/advisory-db#425 for more details.
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.

5 participants