Skip to content
This repository has been archived by the owner on Feb 15, 2021. It is now read-only.

Add Sync bound to Send impl of LockWeak<T> #6

Merged
merged 1 commit into from
Feb 14, 2021
Merged

Add Sync bound to Send impl of LockWeak<T> #6

merged 1 commit into from
Feb 14, 2021

Conversation

JOE1994
Copy link
Contributor

@JOE1994 JOE1994 commented Nov 14, 2020

Hello! 🦀,
While scanning crates.io., we (Rust group @sslab-gatech) have noticed a soundness/memory safety issue in this crate which allows safe Rust code to trigger undefined behavior. This PR adds a small fix to resolve the issue.

Issue

It is possible to create data races by enclosing a
non-Sync type data to ParentArc and by accessing the enclosed data
from multiple threads via ChildArc.

This is possible since there is no bound on T of impl Send for LockWeak<T>.

unsafe impl<T> Send for LockWeak<T> {}

Proof of Concept

I prepared a minimal example that incurs undefined behavior while using the parc crate with safe Rust.
To observe undefined behavior, you need to run the below program in Debug mode.
Since Rc's internal strong_count is updated by multiple threads without synchronization,
the program will terminate in either one of the following states.

  • strong_count > 1
    • program panics at the assertion check at the end (memory leak)
  • strong_count == 0
    • Rc is dropped while references to it are still alive.
      When run on Ubuntu 18.04, program crashes with error: Illegal Instruction (Core Dumped)
  • strong_count == 1
    • Not impossible, but highly unlikely
#![forbid(unsafe_code)]

use parc::ParentArc;
use std::rc::Rc;

fn main() {
    // `Rc` neither implements `Send` nor `Sync`.
    let parent = ParentArc::new(Rc::new(0));

    let mut children = vec![];
    for _ in 0..5 {
        let weak = ParentArc::downgrade(&parent);
        let child_thr = std::thread::spawn(move || {
            loop {
                // `weak` is moved into child thread.
                let child = weak.upgrade();
                match child {
		    Some(rc) => {
                        for _ in 0..2000 {
                            // `strong_count` of `rc`
                            // is updated by multiple threads without synchronization.
                            let _ = Rc::clone(rc.as_ref());
                        }
                        break;
                    }
		    None => continue,
                }
            }
        });
        children.push(child_thr);
    }
    for child_thr in children {
        child_thr.join().expect("Failed to join with child thread");
    }
 
    let rc = parent.block_into_inner();

    // if (`strong_count` > 1): indicates a memory leak
    assert_eq!(1, Rc::strong_count(&rc));
}

Solution to the issue

The issue exists due to the fact that sending LockWeak to multiple threads can let multiple threads concurrently
access the underlying data. This PR addresses the issue by adding a Sync bound in the Send impl of LockWeak.

Please let me know if there are any concerns regarding this change, and
thank you for reviewing this PR 🐱

It is possible to create data races by enclosing a
non-Sync type data to `ParentArc` and by accessing the enclosed data
from multiple threads via `ChildArc`.

This PR fixes the issue by adding a Sync bound to the
`Send` impl of `LockWeak<T>`.
For a detailed explanation with a proof-of-concept,
please refer to the corresponding PR description.

Thank you for reviewing :)
@JOE1994 JOE1994 changed the title Add Sync bound to Send impl of LockWeak<T> Add Send + Sync bound to Send impl of LockWeak<T> Jan 23, 2021
@JOE1994 JOE1994 changed the title Add Send + Sync bound to Send impl of LockWeak<T> Add Sync bound to Send impl of LockWeak<T> Jan 23, 2021
@Shnatsel
Copy link

Heads up: this issue has been included in the RustSec advisory database. It will be surfaced by tools such as cargo-audit or cargo-deny from now on.

Once a fix is released to crates.io, please open a pull request to update the advisory with the patched version, or file an issue on the advisory database repository.

@hyyking hyyking merged commit 5e43ee8 into hyyking:master Feb 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants