DelSkayn / rquickjs

High level bindings to the quickjs javascript engine
MIT License
434 stars 59 forks source link

Crash due to memory leak & undefined behaviour in spawn poller when spawning nested tasks #224

Closed richarddd closed 8 months ago

richarddd commented 9 months ago

Upon further investigation of https://github.com/DelSkayn/rquickjs/issues/223 i discovered a memory leak in https://github.com/DelSkayn/rquickjs/blob/fb7d128d512d9f9a11024c8594e7486e3d350ae6/core/src/runtime/spawner.rs#L70 resulting in a crash:

Assertion failed: (list_empty(&rt->gc_obj_list)), function JS_FreeRuntime, file quickjs.c
[1]    39846 abort 

Consider the following code:

let mut did_complete = false;
self.0.futures.retain_mut(|f| {
    let ready = f.as_mut().poll(cx).is_ready();
    did_complete = did_complete || ready;
    !ready
});

If the f.as_mut().poll(cx) contains a new ctx.spawn it will modify the futures vec causing undefined behaviour.

That's why my original PR https://github.com/DelSkayn/rquickjs/pull/201/commits/982c4640a8a054704a8f98f5efe8975c75fd0d15 correcting the spawn ordering used a vec to collect the indexes and then remove them to avoid concurrent modification of the futures vec:

fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> Poll<Self::Output> {
  if self.0.futures.is_empty() {
      return Poll::Ready(false);
  }

  let mut completed_indices = Vec::new();

  // Iterate over the futures and check their completion status.
  for (i, f) in self.0.futures.iter_mut().enumerate() {
      if f.as_mut().poll(cx).is_ready() {
          completed_indices.push(i);
      }
  }

  // Remove the completed futures in reverse order to avoid index shifting.
  for &idx in completed_indices.iter().rev() {
      self.0.futures.remove(idx);
  }

  if !completed_indices.is_empty() {
      Poll::Ready(true)
  } else {
      Poll::Pending
  }
}