channel: unbounded: synchronize receiver disconnect with initialization #972

petrosagg · 2023-04-08T17:50:24Z

Receiver disconnection relies on the incorrect assumption that head.index != tail.index implies that the channel is initialized (i.e head.block and tail.block point to allocated blocks). However, it can happen that head.index != tail.index and head.block == null at the same time which leads to a segfault when a channel is dropped in that state.

This can happen because initialization is performed in two steps. First, the tail block is allocated and the tail.block is set. If that is successful head.block is set to the same pointer. Importantly, initialization is skipped if tail.block is not null.

Therefore we can have the following situation:

Thread A starts to send the first value of the channel, observes that tail.block is null and begins initialization. It sets tail.block to point to a newly allocated block and then gets preempted. head.block is still null at this point.
Thread B starts to send the second value of the channel, observes that tail.block is not null and proceeds with writing its value in the allocated tail block and sets tail.index to 1.
Thread B drops the receiver of the channel which observes that head.index != tail.index (0 and 1 respectively), therefore there must be messages to drop. It starts traversing the linked list from head.block which is still a null pointer, leading to a segfault.

The first commit of this PR includes a reproduction of this scenario.

This PR fixes this problem by waiting for initialization to complete when head.index != tail.index and the head.block is still null. A similar check exists in start_recv for similar reasons.

Fixes #971

Signed-off-by: Petros Angelatos <[email protected]>

We must take into account the case where the channel has messages in the block pointed to by `tail` but the head is still pointing to a null pointer. This can happen with two concurrent senders if one gets preempted during initialization. Signed-off-by: Petros Angelatos <[email protected]>

Signed-off-by: Petros Angelatos <[email protected]>

taiki-e

Thanks!

bors r+

bors · 2023-04-09T11:42:18Z

Build succeeded:

973: v0.8: Prepare for the next release r=taiki-e a=taiki-e - crossbeam-channel 0.5.7 -> 0.5.8 - Fix race condition in unbounded channel. (#972) Also, yanking `>= 0.5.1, <= 0.5.7` that affected by the bug fixed in this release. Co-authored-by: Petros Angelatos <[email protected]> Co-authored-by: Taiki Endo <[email protected]>

Update to `v0.5.8` which includes crossbeam-rs/crossbeam#972 Signed-off-by: Petros Angelatos <[email protected]>

update iana-time-zone-haiku to drop bumch of cxx* deps cargo update -p iana-time-zone-haiku Updating crates.io index Updating cc v1.0.77 -> v1.0.79 Removing codespan-reporting v0.11.1 Removing cxx v1.0.94 Removing cxx-build v1.0.94 Removing cxxbridge-flags v1.0.94 Removing cxxbridge-macro v1.0.94 Updating iana-time-zone-haiku v0.1.1 -> v0.1.2 Removing link-cplusplus v1.0.8 Removing scratch v1.0.5 fixes known issue crossbeam-rs/crossbeam#972 cargo update -p crossbeam-channel Updating crates.io index Updating crossbeam-channel v0.5.6 -> v0.5.8 dedupes memoffset versions cargo update -p crossbeam-epoch Updating crates.io index Updating crossbeam-epoch v0.9.13 -> v0.9.14 Removing memoffset v0.7.1 dedupes bstr versions cargo update -p ignore -p opener Updating crates.io index Removing bstr v0.2.17 Updating globset v0.4.9 -> v0.4.10 Updating ignore v0.4.18 -> v0.4.20 Updating opener v0.5.0 -> v0.5.2

…lacrum deps: bump crates Updates few deps: drops a lot of cxx* crates: ```console $ cargo update -p iana-time-zone-haiku Updating crates.io index Updating cc v1.0.77 -> v1.0.79 Removing codespan-reporting v0.11.1 Removing cxx v1.0.94 Removing cxx-build v1.0.94 Removing cxxbridge-flags v1.0.94 Removing cxxbridge-macro v1.0.94 Updating iana-time-zone-haiku v0.1.1 -> v0.1.2 Removing link-cplusplus v1.0.8 Removing scratch v1.0.5 ``` cc: https://github.com/rust-lang/cc-rs/releases/tag/1.0.78, https://github.com/rust-lang/cc-rs/releases/tag/1.0.79 iana-time-zone-haiku: https://github.com/strawlab/iana-time-zone/releases/tag/haiku%2Fv0.1.2 fixed crossbeam-rs/crossbeam#972 (similar fixed in rust repo rust-lang#110089) ```console $ cargo update -p crossbeam-channel Updating crates.io index Updating crossbeam-channel v0.5.6 -> v0.5.8 ``` https://github.com/crossbeam-rs/crossbeam/blob/master/crossbeam-channel/CHANGELOG.md#version-058 dedupes memoffset versions: ```console $ cargo update -p crossbeam-epoch Updating crates.io index Updating crossbeam-epoch v0.9.13 -> v0.9.14 Removing memoffset v0.7.1 ``` https://github.com/crossbeam-rs/crossbeam/blob/master/crossbeam-epoch/CHANGELOG.md#version-0914 Gilnaa/memoffset@v0.6.5...v0.8.0 rust-lang#108638 (comment) dedupes bstr versions ```console $ cargo update -p ignore -p opener Updating crates.io index Removing bstr v0.2.17 Updating globset v0.4.9 -> v0.4.10 Updating ignore v0.4.18 -> v0.4.20 Updating opener v0.5.0 -> v0.5.2 ``` globset BurntSushi/ripgrep@ac8fecb ignore https://github.com/BurntSushi/ripgrep/commits/master/crates/ignore hard to track, but drop dep on crossbeam-utils (BurntSushi/ripgrep@e95254a), don't stat git if require_git is false (BurntSushi/ripgrep@009dda1) and added bunch of formats to ignore list opener Seeker14491/opener@v0.5.0...v0.5.2 nothing interesting

deps: bump crates Updates few deps: drops a lot of cxx* crates: ```console $ cargo update -p iana-time-zone-haiku Updating crates.io index Updating cc v1.0.77 -> v1.0.79 Removing codespan-reporting v0.11.1 Removing cxx v1.0.94 Removing cxx-build v1.0.94 Removing cxxbridge-flags v1.0.94 Removing cxxbridge-macro v1.0.94 Updating iana-time-zone-haiku v0.1.1 -> v0.1.2 Removing link-cplusplus v1.0.8 Removing scratch v1.0.5 ``` cc: https://github.com/rust-lang/cc-rs/releases/tag/1.0.78, https://github.com/rust-lang/cc-rs/releases/tag/1.0.79 iana-time-zone-haiku: https://github.com/strawlab/iana-time-zone/releases/tag/haiku%2Fv0.1.2 fixed crossbeam-rs/crossbeam#972 (similar fixed in rust repo rust-lang/rust#110089) ```console $ cargo update -p crossbeam-channel Updating crates.io index Updating crossbeam-channel v0.5.6 -> v0.5.8 ``` https://github.com/crossbeam-rs/crossbeam/blob/master/crossbeam-channel/CHANGELOG.md#version-058 dedupes memoffset versions: ```console $ cargo update -p crossbeam-epoch Updating crates.io index Updating crossbeam-epoch v0.9.13 -> v0.9.14 Removing memoffset v0.7.1 ``` https://github.com/crossbeam-rs/crossbeam/blob/master/crossbeam-epoch/CHANGELOG.md#version-0914 Gilnaa/memoffset@v0.6.5...v0.8.0 rust-lang/rust#108638 (comment) dedupes bstr versions ```console $ cargo update -p ignore -p opener Updating crates.io index Removing bstr v0.2.17 Updating globset v0.4.9 -> v0.4.10 Updating ignore v0.4.18 -> v0.4.20 Updating opener v0.5.0 -> v0.5.2 ``` globset BurntSushi/ripgrep@ac8fecb ignore https://github.com/BurntSushi/ripgrep/commits/master/crates/ignore hard to track, but drop dep on crossbeam-utils (BurntSushi/ripgrep@e95254a), don't stat git if require_git is false (BurntSushi/ripgrep@009dda1) and added bunch of formats to ignore list opener Seeker14491/opener@v0.5.0...v0.5.2 nothing interesting

Versions 0.5.1 - 0.5.7 were yanked due to a race condition. See [changelog](https://github.com/crossbeam-rs/crossbeam/blob/master/crossbeam-channel/CHANGELOG.md#version-058) and [crates.io](https://crates.io/crates/crossbeam-channel/versions) and [PR with the fix](crossbeam-rs/crossbeam#972).

deps: bump crates Updates few deps: drops a lot of cxx* crates: ```console $ cargo update -p iana-time-zone-haiku Updating crates.io index Updating cc v1.0.77 -> v1.0.79 Removing codespan-reporting v0.11.1 Removing cxx v1.0.94 Removing cxx-build v1.0.94 Removing cxxbridge-flags v1.0.94 Removing cxxbridge-macro v1.0.94 Updating iana-time-zone-haiku v0.1.1 -> v0.1.2 Removing link-cplusplus v1.0.8 Removing scratch v1.0.5 ``` cc: https://github.com/rust-lang/cc-rs/releases/tag/1.0.78, https://github.com/rust-lang/cc-rs/releases/tag/1.0.79 iana-time-zone-haiku: https://github.com/strawlab/iana-time-zone/releases/tag/haiku%2Fv0.1.2 fixed crossbeam-rs/crossbeam#972 (similar fixed in rust repo rust-lang/rust#110089) ```console $ cargo update -p crossbeam-channel Updating crates.io index Updating crossbeam-channel v0.5.6 -> v0.5.8 ``` https://github.com/crossbeam-rs/crossbeam/blob/master/crossbeam-channel/CHANGELOG.md#version-058 dedupes memoffset versions: ```console $ cargo update -p crossbeam-epoch Updating crates.io index Updating crossbeam-epoch v0.9.13 -> v0.9.14 Removing memoffset v0.7.1 ``` https://github.com/crossbeam-rs/crossbeam/blob/master/crossbeam-epoch/CHANGELOG.md#version-0914 Gilnaa/memoffset@v0.6.5...v0.8.0 rust-lang/rust#108638 (comment) dedupes bstr versions ```console $ cargo update -p ignore -p opener Updating crates.io index Removing bstr v0.2.17 Updating globset v0.4.9 -> v0.4.10 Updating ignore v0.4.18 -> v0.4.20 Updating opener v0.5.0 -> v0.5.2 ``` globset BurntSushi/ripgrep@ac8fecb ignore https://github.com/BurntSushi/ripgrep/commits/master/crates/ignore hard to track, but drop dep on crossbeam-utils (BurntSushi/ripgrep@e95254a), don't stat git if require_git is false (BurntSushi/ripgrep@009dda1) and added bunch of formats to ignore list opener Seeker14491/opener@v0.5.0...v0.5.2 nothing interesting

This PR is fixing a regression introduced by crossbeam-rs#1084 that can lead to a double free when dropping the channel. The method `Channel::discard_all_messages` has the property that if it observes `head.block` pointing to a non-null pointer it will attempt to free it. The same property holds for the `Channel::drop` method and so it is critical that whenever `head.block` is freed it must also be set to a null pointer so that it is freed exactly once. Before crossbeam-rs#1084 the behavior of `discard_all_messages` ensured `head.block` was `null` after its execution due to the atomic store right before exiting [1]. After crossbeam-rs#1084 `discard_all_messages` atomically swaps the current value of `head.block` with a null pointer at the moment the value is read instead of waiting for the end of the function. The problem lies in the fact that `dicard_all_messages` contained two paths that could lead to `head.block` being read but only one of them would swap the value. This meant that `dicard_all_messages` could end up observing a non-null block pointer (and therefore attempting to free it) without setting `head.block` to null. This would then lead to `Channel::drop` making a second attempt at dropping the same pointer. The bug is similar to the one previously fixed by crossbeam-rs#972 and the double free can be reproduced by reverting the reproduction commit from that PR [2]. As with crossbeam-rs#972 it is quite difficult to trigger this bug without introducing artificial sleeps in critical points so this PR does not include a test. [1] https://github.com/crossbeam-rs/crossbeam/blob/crossbeam-channel-0.5.11/crossbeam-channel/src/flavors/list.rs#L625 [2] crossbeam-rs@2d22628 Signed-off-by: Petros Angelatos <[email protected]>

This PR is fixing a regression introduced by #1084 that can lead to a double free when dropping the channel. The method `Channel::discard_all_messages` has the property that if it observes `head.block` pointing to a non-null pointer it will attempt to free it. The same property holds for the `Channel::drop` method and so it is critical that whenever `head.block` is freed it must also be set to a null pointer so that it is freed exactly once. Before #1084 the behavior of `discard_all_messages` ensured `head.block` was `null` after its execution due to the atomic store right before exiting [1]. After #1084 `discard_all_messages` atomically swaps the current value of `head.block` with a null pointer at the moment the value is read instead of waiting for the end of the function. The problem lies in the fact that `dicard_all_messages` contained two paths that could lead to `head.block` being read but only one of them would swap the value. This meant that `dicard_all_messages` could end up observing a non-null block pointer (and therefore attempting to free it) without setting `head.block` to null. This would then lead to `Channel::drop` making a second attempt at dropping the same pointer. The bug is similar to the one previously fixed by #972 and the double free can be reproduced by reverting the reproduction commit from that PR [2]. As with #972 it is quite difficult to trigger this bug without introducing artificial sleeps in critical points so this PR does not include a test. [1] https://github.com/crossbeam-rs/crossbeam/blob/crossbeam-channel-0.5.11/crossbeam-channel/src/flavors/list.rs#L625 [2] 2d22628 Signed-off-by: Petros Angelatos <[email protected]>

petrosagg added 3 commits April 8, 2023 19:11

channel: unbounded: minimal testcase for null pointer dereference

00dcea6

Signed-off-by: Petros Angelatos <[email protected]>

channel: unbounded: remove artificial preemption and reproduction

2d22628

Signed-off-by: Petros Angelatos <[email protected]>

taiki-e approved these changes Apr 9, 2023

View reviewed changes

bors bot merged commit 5b7499b into crossbeam-rs:master Apr 9, 2023

taiki-e mentioned this pull request Apr 9, 2023

v0.8: Prepare for the next release #973

Merged

petrosagg added a commit to petrosagg/materialize that referenced this pull request Apr 10, 2023

deps: pick up crossbeam soundness fix

176a2a6

Update to `v0.5.8` which includes crossbeam-rs/crossbeam#972 Signed-off-by: Petros Angelatos <[email protected]>

petrosagg mentioned this pull request Apr 10, 2023

deps: pick up crossbeam soundness fix MaterializeInc/materialize#18689

Merged

5 tasks

petrosagg added a commit to petrosagg/materialize that referenced this pull request Apr 10, 2023

deps: pick up crossbeam soundness fix

d729bae

Update to `v0.5.8` which includes crossbeam-rs/crossbeam#972 Signed-off-by: Petros Angelatos <[email protected]>

petrosagg added a commit to petrosagg/materialize that referenced this pull request Apr 10, 2023

deps: pick up crossbeam soundness fix

02b642b

Update to `v0.5.8` which includes crossbeam-rs/crossbeam#972 Signed-off-by: Petros Angelatos <[email protected]>

the8472 mentioned this pull request Apr 11, 2023

sync::mpsc: synchronize receiver disconnect with initialization rust-lang/rust#110089

Merged

sploiselle pushed a commit to sploiselle/materialize that referenced this pull request Apr 14, 2023

deps: pick up crossbeam soundness fix

b945d0a

Update to `v0.5.8` which includes crossbeam-rs/crossbeam#972 Signed-off-by: Petros Angelatos <[email protected]>

klensy mentioned this pull request May 26, 2023

deps: bump crates rust-lang/rust#111989

Merged

jakmeier mentioned this pull request Sep 26, 2023

chore: bump crossbeam-channel to 0.5.8 near/nearcore#9592

Merged

petrosagg mentioned this pull request Apr 8, 2025

crossbeam-channel: prevent double free on Drop #1187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

channel: unbounded: synchronize receiver disconnect with initialization #972

channel: unbounded: synchronize receiver disconnect with initialization #972

Uh oh!

petrosagg commented Apr 8, 2023

Uh oh!

taiki-e left a comment

Uh oh!

bors bot commented Apr 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

channel: unbounded: synchronize receiver disconnect with initialization #972

channel: unbounded: synchronize receiver disconnect with initialization #972

Uh oh!

Conversation

petrosagg commented Apr 8, 2023

Uh oh!

taiki-e left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Apr 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants