Fixed
Status Update
Comments
ky...@bytedance.com <ky...@bytedance.com> #2
This bug should go to the emulator team. However, I do not have permission to file bug against them yet.
rk...@google.com <rk...@google.com>
rk...@google.com <rk...@google.com> #3
Thank you for reporting this. I think we have a bad race condition in RecurrentTask
. When a callback fires, the QEMUTimer
is empty (see cb(opaque)
is unlock):
/* remove timer from the list before calling the callback */
timer_list->active_timers = ts->next;
ts->next = NULL;
ts->expire_time = -1;
cb = ts->cb;
opaque = ts->opaque;
/* run the callback (the timer list can be modified) */
qemu_mutex_unlock(&timer_list->active_timers_lock);
cb(opaque);
qemu_mutex_lock(&timer_list->active_timers_lock);
and timer_del_locked
simply removes the QEMUTimer
instance from the active_timers
list. I am not sure how to avoid race conditions here. Consider this:
1: void myCallback(void* arg) {
2:
3: someCall();
4:
5: }
- The control enters into
myCallback
and the thread is preempted before the function had a chance making any side effect (line2
here). ËœRecurrentTask
is called on a separate thread.- Since
myCallback
did not write anywhere that it is busyËœRecurrentTask
proceeds destroying the statemyCallback
is about to access. myCallback
wakes and proceeds accessing already destroyed data.
Either I am missing something or this API is incomplete.
Description
AutoLock
appears to be a custom implementation ofstd::unique_lock/lock_guard
. It does, however, a method namedisLocked()
. There is only one use of the method at the moment:I believe, that has room for improvement. Firstly, even if we checked
isLock()
in general, asisLock()
is not thread-safe, there is no guarantee (in general) that the same thread can then lock it. It is only useful when there is only one thread that locks/unlocks the givenAutoLock
object and the thread wants know whether thelock
is acquired or not by itself. I don't think that's useful. I think the method being public, it has a good chance to confuse external developers.My belief is that the
AutoLock
should be simply replaced withstd::unique_lock
and/orstd::lock_guard
, as many uses of the class do not calllock
orunlock
. And,isLocked()
should be removed. The only call site should be implemented without that.Aside from all mentioned above, I am trying to compile the code with
--enable-thread-safety-checks
, and the first code that blocked the compilation on Linux was the use ofisLocked()
.