Skip to content

Commit

Permalink
fix potential deadlock in Lazy
Browse files Browse the repository at this point in the history
  • Loading branch information
matsadler committed May 9, 2024
1 parent 458b1e8 commit fb83309
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 18 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@
- `Exception::backtrace`.

### Fixed
- Potential deadlock in `Lazy` when accessed for the first time from
multiple threads simultaneously.

### Security

Expand Down
42 changes: 24 additions & 18 deletions src/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -454,12 +454,8 @@ unsafe impl<T: ReprValue> Sync for Opaque<T> {}
pub struct Lazy<T: ReprValue> {
init: Once,
mark: bool,
inner: UnsafeCell<LazyInner<T>>,
}

union LazyInner<T: ReprValue> {
func: fn(&Ruby) -> T,
value: T,
value: UnsafeCell<Value>,
}

impl<T> Lazy<T>
Expand All @@ -469,7 +465,9 @@ where
/// Create a new `Lazy<T>`.
///
/// This function can be called in a `const` context. `func` is evaluated
/// once when the `Lazy<T>` is first accessed (see [`Ruby::get_inner`]).
/// when the `Lazy<T>` is first accessed (see [`Ruby::get_inner`]). If
/// multiple threads attempt first access at the same time `func` may be
/// called more than once, but all threads will recieve the same value.
///
/// This function assumes the `Lazy<T>` will be assinged to a `static`, so
/// marks the inner Ruby value with Ruby's garbage collector to never be
Expand All @@ -491,7 +489,8 @@ where
Self {
init: Once::new(),
mark: true,
inner: UnsafeCell::new(LazyInner { func }),
func,
value: UnsafeCell::new(QNIL.0.get()),
}
}

Expand All @@ -506,7 +505,8 @@ where
Self {
init: Once::new(),
mark: false,
inner: UnsafeCell::new(LazyInner { func }),
func,
value: UnsafeCell::new(QNIL.0.get()),
}
}

Expand Down Expand Up @@ -562,7 +562,7 @@ where
unsafe {
this.init
.is_completed()
.then(|| (*this.inner.get()).value.into())
.then(|| T::from_value_unchecked(*this.value.get()).into())
}
}
}
Expand All @@ -589,15 +589,16 @@ where

fn get_inner_ref_with<'a>(&'a self, ruby: &Ruby) -> &'a Self::Value {
unsafe {
self.init.call_once(|| {
let inner = self.inner.get();
let value = ((*inner).func)(ruby);
if self.mark {
gc::register_mark_object(value);
}
(*inner).value = value;
});
&(*self.inner.get()).value
if !self.init.is_completed() {
let value = (self.func)(ruby);
self.init.call_once(|| {
if self.mark {
gc::register_mark_object(value);
}
*self.value.get() = value.as_value();
});
}
T::ref_from_ref_value_unchecked(&*self.value.get())
}
}
}
Expand Down Expand Up @@ -626,6 +627,11 @@ pub(crate) mod private {
*(&val as *const Value as *const Self)
}

#[inline]
unsafe fn ref_from_ref_value_unchecked(val: &Value) -> &Self {
&*(val as *const Value as *const Self)
}

#[inline]
fn copy_as_value(self) -> Value {
// This trait is only ever implemented for things with the same
Expand Down

0 comments on commit fb83309

Please sign in to comment.