From 4385992f5476588894c5495cf48970b2254aa89a Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Fri, 25 Aug 2023 22:52:29 +0800 Subject: [PATCH] fix: soundness issue in Somr --- y-octo/src/doc/common/somr.rs | 82 +++++++++++++++++++++++++++++------ y-octo/src/doc/document.rs | 4 +- y-octo/src/doc/store.rs | 12 ++--- 3 files changed, 76 insertions(+), 22 deletions(-) diff --git a/y-octo/src/doc/common/somr.rs b/y-octo/src/doc/common/somr.rs index a1c2889..32a21fb 100644 --- a/y-octo/src/doc/common/somr.rs +++ b/y-octo/src/doc/common/somr.rs @@ -1,8 +1,10 @@ use std::{ + cell::UnsafeCell, fmt::{self, Write}, hash::{Hash, Hasher}, marker::PhantomData, mem, + ops::{Deref, DerefMut}, ptr::NonNull, }; @@ -25,13 +27,32 @@ pub(crate) struct Owned(NonNull>); pub(crate) struct Ref(NonNull>); pub(crate) struct SomrInner { - data: Option, + data: Option>, /// increase the size when we really meet the the secenerio with refs more /// then u16::MAX(65535) times refs: AtomicU32, _marker: PhantomData>, } +pub(crate) struct InnerRefMut<'a, T> { + inner: NonNull, + _marker: PhantomData<&'a mut T>, +} + +impl<'a, T> Deref for InnerRefMut<'a, T> { + type Target = T; + + fn deref(&self) -> &Self::Target { + unsafe { &*self.inner.as_ptr() } + } +} + +impl<'a, T> DerefMut for InnerRefMut<'a, T> { + fn deref_mut(&mut self) -> &mut Self::Target { + unsafe { &mut *self.inner.as_ptr() } + } +} + unsafe impl Send for Somr {} unsafe impl Sync for Somr {} @@ -44,7 +65,7 @@ impl Default for Somr { impl Somr { pub fn new(data: T) -> Self { let inner = Box::new(SomrInner { - data: Some(data), + data: Some(UnsafeCell::new(data)), refs: AtomicU32::new(1), _marker: PhantomData, }); @@ -57,15 +78,28 @@ impl Somr { } } +impl SomrInner { + fn data_ref(&self) -> Option<&T> { + self.data.as_ref().map(|x| unsafe { &*x.get() }) + } + + fn data_mut(&self) -> Option> { + self.data.as_ref().map(|x| InnerRefMut { + inner: unsafe { NonNull::new_unchecked(x.get()) }, + _marker: PhantomData, + }) + } +} + impl Somr { #[inline] pub fn is_none(&self) -> bool { - self.dangling() || self.inner().data.is_none() + self.dangling() || self.inner().data_ref().is_none() } #[inline] pub fn is_some(&self) -> bool { - !self.dangling() && self.inner().data.is_some() + !self.dangling() && self.inner().data_ref().is_some() } pub fn get(&self) -> Option<&T> { @@ -73,7 +107,7 @@ impl Somr { return None; } - self.inner().data.as_ref() + self.inner().data_ref() } pub unsafe fn get_unchecked(&self) -> &T { @@ -81,7 +115,7 @@ impl Somr { panic!("Try to visit Somr data that has already been dropped.") } - match &self.inner().data { + match &self.inner().data_ref() { Some(data) => data, None => { panic!("Try to unwrap on None") @@ -89,23 +123,31 @@ impl Somr { } } - #[allow(dead_code)] - pub fn get_mut(&self) -> Option<&mut T> { + pub fn get_mut(&mut self) -> Option<&mut T> { if !self.is_owned() || self.dangling() { return None; } let inner = self.inner_mut(); - inner.data.as_mut() + inner.data.as_mut().map(|x| x.get_mut()) + } + + pub fn get_mut_from_ref(&self) -> Option> { + if !self.is_owned() || self.dangling() { + return None; + } + + let inner = self.inner_mut(); + inner.data_mut() } #[allow(clippy::mut_from_ref)] - pub unsafe fn get_mut_unchecked(&self) -> &mut T { + pub unsafe fn get_mut_unchecked(&self) -> InnerRefMut<'_, T> { if self.dangling() { panic!("Try to visit Somr data that has already been dropped.") } - match &mut self.inner_mut().data { + match self.inner_mut().data_mut() { Some(data) => data, None => { panic!("Try to unwrap on None") @@ -238,7 +280,7 @@ impl PartialEq for Somr { impl PartialEq for SomrInner { fn eq(&self, other: &Self) -> bool { - self.data == other.data + self.data_ref() == other.data_ref() } } @@ -366,7 +408,7 @@ mod tests { #[test] fn acquire_mut_ref() { loom_model!({ - let five = Somr::new(5); + let mut five = Somr::new(5); *five.get_mut().unwrap() += 1; assert_eq!(five.get(), Some(&6)); @@ -375,7 +417,7 @@ mod tests { // only owner can mut ref assert!(five_ref.get().is_some()); - assert!(five_ref.get_mut().is_none()); + assert!(five_ref.get_mut_from_ref().is_none()); drop(five); }); @@ -442,4 +484,16 @@ mod tests { assert!(!five.is_owned()); }); } + + // This is UB if we didn't use `UnsafeCell` in `Somr` + #[test] + fn test_inner_mut() { + let five = Somr::new(5); + fn add(a: &Somr, b: &Somr) { + a.get_mut_from_ref().map(|mut x| *x += *b.get().unwrap()).unwrap(); + } + + add(&five, &five); + assert_eq!(five.get().copied().unwrap(), 10); + } } diff --git a/y-octo/src/doc/document.rs b/y-octo/src/doc/document.rs index a6e0593..3c22900 100644 --- a/y-octo/src/doc/document.rs +++ b/y-octo/src/doc/document.rs @@ -101,8 +101,8 @@ impl Doc { for (mut s, offset) in update.iter(store.get_state_vector()) { if let Node::Item(item) = &mut s { debug_assert!(item.is_owned()); - let item = unsafe { item.get_mut_unchecked() }; - store.repair(item, self.store.clone())?; + let mut item = unsafe { item.get_mut_unchecked() }; + store.repair(&mut item, self.store.clone())?; } store.integrate(s, offset, None)?; } diff --git a/y-octo/src/doc/store.rs b/y-octo/src/doc/store.rs index 5ec3828..47cb410 100644 --- a/y-octo/src/doc/store.rs +++ b/y-octo/src/doc/store.rs @@ -186,15 +186,15 @@ impl DocStore { // we make sure store is the only entry of mutating an item, // and we already hold mutable reference of store, so it's safe to do so unsafe { - let left_item = raw_left_item.get_mut_unchecked(); - let right_item = right_item.get_mut_unchecked(); + let mut left_item = raw_left_item.get_mut_unchecked(); + let mut right_item = right_item.get_mut_unchecked(); left_item.content = new_left_item.get_unchecked().content.clone(); // we had the correct left/right content // now build the references let right_right_ref = ItemRef::from(&left_item.right); right_item.left = if right_right_ref.is_some() { - let right_right = right_right_ref.get_mut_unchecked(); + let mut right_right = right_right_ref.get_mut_unchecked(); right_right.left.replace(right_node.clone()) } else { Some(node.clone()) @@ -387,7 +387,7 @@ impl DocStore { // before we integrate struct into store, // the struct => Arc is owned reference actually, // no one else refer to such item yet, we can safely mutable refer to it now. - let this = unsafe { item.get_mut_unchecked() }; + let this = &mut *unsafe { item.get_mut_unchecked() }; if offset > 0 { this.id.clock += offset; @@ -485,7 +485,7 @@ impl DocStore { if left.is_some() { unsafe { // SAFETY: we get store write lock, no way the left get dropped by owner - let left = left.get_mut_unchecked(); + let mut left = left.get_mut_unchecked(); right = left.right.replace(Node::Item(item.clone())).into(); } this.left = Some(Node::Item(left)); @@ -508,7 +508,7 @@ impl DocStore { if right.is_some() { unsafe { // SAFETY: we get store write lock, no way the left get dropped by owner - let right = right.get_mut_unchecked(); + let mut right = right.get_mut_unchecked(); right.left = Some(Node::Item(item.clone())); } this.right = Some(Node::Item(right))