sync::mpsc: synchronize receiver disconnect with initialization

[petrosagg]

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:

1. 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.
2. 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.
3. 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.

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 #110001

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[ibraheemdev]

Thanks, I suspected this was the issue. @teskje can you verify your example no longer segfaults with this patch applied?

[teskje]

I tried to reproduce the segfault with this diff applied for ~1 hour and wasn't able to!

[the8472]

upstream fix: crossbeam-rs/crossbeam#972

[the8472]

Since the cause has been identified is it possible to write a test that triggers the flaw reliably?

[ibraheemdev]

Not really, it would require preemption at a specific point within send(). Possibly with a cfg(miri) yield() but I'm not sure that's something we want to do?

[m-ou-se]

@bors r+

[bors]

📌 Commit f0d487d has been approved by m-ou-se

It is now in the queue for this repository.