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

RcCell panics on drop? #550

Open
Medowhill opened this issue May 28, 2021 · 2 comments
Open

RcCell panics on drop? #550

Medowhill opened this issue May 28, 2021 · 2 comments
Assignees
Labels

Comments

@Medowhill
Copy link
Collaborator

현재 RcCell은 rc가 0보다 큰 상황에서 드롭되면 패닉을 발생시킵니다. rv6에서는 RcCell을 스택에 배치하는 경우가 없고 한 스레드라도 패닉하면 결국 커널 전체가 패닉하므로 이 설계가 큰 문제가 아닐 수 있지만, 일반적인 경우를 생각해 보면 현재의 RcCell 설계는 메모리 안전성을 깰 수 있어 보입니다.

아래 코드에서 RcCell을 소유한 스레드가 종료되면서 RcCell이 드롭되면 rc가 0이 아니기 때문에 패닉이 발생합니다. 하지만 메인 스레드의 실행에는 영향을 주지 않기 때문에, 이미 드롭된 RcCell을 가리키는 Ref를 dereference할 수 있으며 이는 UB입니다(실제 실행해 보면 segmentaion fault가 발생하며 Miri로도 UB가 탐지됨).

https://doc.rust-lang.org/std/ops/trait.Drop.html#panics에 따르면 drop안에서 패닉이 발생하더라도 그 값은 드롭된 것으로 간주되는 것 같습니다.

Note that even if this panics, the value is considered to be dropped;

use std::{ops::Deref, pin::Pin, sync::{atomic::{AtomicUsize, Ordering}, mpsc::channel}, thread};

fn main() {
    let (tx, rx) = channel();

    let handler = thread::spawn(move || {
        let rc = Rc::new(0u32);
        let rc = Pin::new(&rc);
        let r = rc.borrow();
        tx.send(r).unwrap();
    });

    let _ = handler.join();
    let r = rx.recv().unwrap();
    let _: &u32 = &r;
}

struct Rc<T> {
    data: T,
    rc: AtomicUsize,
}

impl<T> Rc<T> {
    const fn new(data: T) -> Self {
        Self {
            data,
            rc: AtomicUsize::new(0),
        }
    }

    fn borrow(self: Pin<&Self>) -> Ref<T> {
        self.rc.fetch_add(1, Ordering::SeqCst);
        Ref(self.get_ref())
    }
}

impl<T> Drop for Rc<T> {
    fn drop(&mut self) {
        if self.rc.load(Ordering::SeqCst) > 0 {
            panic!();
        }
    }
}

struct Ref<T>(*const Rc<T>);

unsafe impl<T: Send> Send for Ref<T> {}
unsafe impl<T: Send> Sync for Ref<T> {}

impl<T> Deref for Ref<T> {
    type Target = T;

    fn deref(&self) -> &Self::Target {
        unsafe { &(*self.0).data }
    }
}

impl<T> Drop for Ref<T> {
    fn drop(&mut self) {
        unsafe { &(*self.0).rc }.fetch_sub(1, Ordering::SeqCst);
    }
}
@Medowhill
Copy link
Collaborator Author

Alternative design 1:

impl<T> Drop for Rc<T> {
    fn drop(&mut self) {
        if self.rc.load(Ordering::SeqCst) > 0 {
            loop {}
        }
    }
}

Alternative design 2:

impl<T> Drop for Rc<T> {
    fn drop(&mut self) {
        while self.rc.load(Ordering::SeqCst) > 0 {}
    }
}

@jeehoonkang
Copy link
Member

지금 논문에 쓴 ArcCellStaticArc 디자인이 좀 다른 것 같습니다. 일반적으로 우리가 디자인한 모든 것에 대해 논문에 쓴대로 kernel code도 수정해야할 것 같습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants