Race condition: `cancelCall()` sets `isCancelling` without holding lock

[grisutheguru]

Summary

CoreAV::cancelCall() (src/core/coreav.cpp:297-316) sets isCancelling = true before acquiring callsLock. Audio/video frame callbacks check isCancelling before acquiring the lock. This creates a TOCTOU race where callbacks can access the calls map while it is being mutated.

Details

// cancelCall() - line 299-310
isCancelling = true;              // no lock held
QWriteLocker locker{&callsLock};  // lock acquired after flag set
// ... erases from calls map ...
isCancelling = false;

// audioFrameCallback() - line 871-875
if (self->isCancelling)           // no lock held
    return;
const QReadLocker locker{&self->callsLock};  // lock acquired after check
auto it = self->calls.find(friendNum);       // map could be mutated between check and find

Between the callback's isCancelling check and its lock acquisition, cancelCall can erase the entry from the calls map. The callback then performs calls.find() on a map being mutated by another thread -- undefined behavior even though the find itself would return end().

Additionally, timeoutCall() (line 318-327) acquires a write lock then calls cancelCall() which acquires another write lock. While QReadWriteLock::Recursive allows this, it makes the locking protocol fragile.

Suggested Fix

Option A: Check isCancelling only under the lock:

// In callbacks:
const QReadLocker locker{&self->callsLock};
if (self->isCancelling) return;
auto it = self->calls.find(friendNum);

Option B (cleaner): Remove isCancelling entirely. Rely on calls.find() == calls.end() under the lock as the authoritative cancellation check.

For timeoutCall, refactor into a locking and non-locking variant to avoid recursive write locks.

Impact

Potential crash or undefined behavior during call teardown, especially when multiple calls are active.