Skip to content

Commit c3a5941

Browse files
committed
fix(async): potential UB in the scheduler implementation
The way we accessed UnsafeCell in the posted event handler was oddly similar to the bad example in the reference[1]. It's not an exact match since we don't have a _shared_ reference to the cell, but I'd still prefer to use the documented way to access the underlying data. [1]: https://doc.rust-lang.org/stable/std/cell/struct.UnsafeCell.html#memory-layout
1 parent 1d76b83 commit c3a5941

File tree

1 file changed

+13
-10
lines changed

1 file changed

+13
-10
lines changed

src/async_/spawn.rs

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ unsafe impl Sync for Scheduler {}
2727

2828
impl Scheduler {
2929
const fn new() -> Self {
30-
Self(UnsafeCell::new(SchedulerInner::new()))
30+
Self(SchedulerInner::new())
3131
}
3232

3333
pub fn schedule(&self, runnable: Runnable) {
@@ -46,17 +46,17 @@ struct SchedulerInner {
4646
}
4747

4848
impl SchedulerInner {
49-
const fn new() -> Self {
49+
const fn new() -> UnsafeCell<Self> {
5050
let mut event: ngx_event_t = unsafe { mem::zeroed() };
5151
event.handler = Some(Self::scheduler_event_handler);
5252

53-
Self {
53+
UnsafeCell::new(Self {
5454
_ident: [
5555
0, 0, 0, 0x4153594e, // ASYN
5656
],
5757
event,
5858
queue: VecDeque::new(),
59-
}
59+
})
6060
}
6161

6262
pub fn send(&mut self, runnable: Runnable) {
@@ -82,12 +82,15 @@ impl SchedulerInner {
8282
let mut runnables = {
8383
// SAFETY:
8484
// This handler always receives a non-null pointer to an event embedded into a
85-
// SchedulerInner instance.
86-
// We modify the contents of `UnsafeCell`, but we ensured that the access is unique due
87-
// to being single-threaded and dropping the reference before we start processing queued
88-
// runnables.
89-
let this =
90-
unsafe { ngx_container_of!(NonNull::new_unchecked(ev), Self, event).as_mut() };
85+
// UnsafeCell<SchedulerInner> instance. We modify the contents of the `UnsafeCell`,
86+
// but we ensured that:
87+
// - we access the cell correctly, as documented in
88+
// https://doc.rust-lang.org/stable/std/cell/struct.UnsafeCell.html#memory-layout
89+
// - the access is unique due to being single-threaded
90+
// - the reference is dropped before we start processing queued runnables.
91+
let cell: NonNull<UnsafeCell<Self>> =
92+
unsafe { ngx_container_of!(NonNull::new_unchecked(ev), Self, event).cast() };
93+
let this = unsafe { &mut *UnsafeCell::raw_get(cell.as_ptr()) };
9194

9295
ngx_log_debug!(
9396
this.event.log,

0 commit comments

Comments
 (0)