dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.08k stars 1.56k forks source link

Potential deadlock in dart:ffi test #45382

Open frostant opened 3 years ago

frostant commented 3 years ago

https://github.com/dart-lang/sdk/blob/master/runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc

intptr_t MyCallbackBlocking(intptr_t a) {
  std::mutex mutex;
  std::unique_lock<std::mutex> lock(mutex);
  intptr_t result;
  auto callback = my_callback_blocking_fp_;  // Define storage duration.
  std::condition_variable cv;
  bool notified = false;
  const Work work = [a, &result, callback, &cv, &notified]() {
    result = callback(a);
    printf("C Da:     Notify result ready.\n");
    notified = true;
    cv.notify_one();
  };
  const Work* work_ptr = new Work(work);  // Copy to heap.
  NotifyDart(my_callback_blocking_send_port_, work_ptr);
  printf("C   :  Waiting for result.\n");
  while (!notified) {
    cv.wait(lock);
  }
  printf("C   :  Received result.\n");
  return result;
}
 // 424 row

After a lot of testing, this place will deadlock. if after while(!notified) then the switch threads to the C++, the program will deadlock. Because the cv.notify_one() will execute before cv.wait(lock). So the condition_variable will lock forever. So, we should prevent notify_one before wait.

void CountUpDownLatch::Wait() {
    if (0 == count_) {
        return;
    }
    std::unique_lock<std::mutex> locker(mutex_);
    while (count_ > 0) {
        cv_.wait(locker);
    }
}

void CountUpDownLatch::CountDown() {
    if (0 == count_) {
        return;
    }
    if (--count_ == 0) {
        std::lock_guard<std::mutex> locker(mutex_);
        cv_.notify_all();
    }
}

In my view, this we can safely prevent the Deadlock

mraleph commented 3 years ago

Indeed. The simplest fix is just to acquire the same mutex before notifying this would ensure the proper ordering.

/cc @dcharkes