Re: [libvirt] [PATCH] qemu: fix msg could be a wild pointer in qemuMonitorIOProcess()

在 2018/6/5 15:10, Peter Krempa 写道: > On Tue, Jun 05, 2018 at 10:14:39 +0800, Shannon Zhao wrote: >> From: Weilun Zhu <zhuweilun(a)huawei.com&gt; >> >> As qemuMonitorJSONIOProcess() will unlock the qemu monitor, there is >> some extreme situation, eg qemu send message to monitor twice in a short >> time, where the local viriable 'msg' of qemuMonitorIOProcess() could be >> a wild point: >> >> 1. qemuMonitorSend() assign mon->msg to parameter 'msg', which is alse a >> local variable of its caller qemuMonitorJSONCommandWithFd(), cause >> eventloop to send message to monitor, then wait condition. >> 2. qemu send message to monitor for the first time immediately. >> 3. qemuMonitorIOProcess() is called, then wake up the qemuMonitorSend() >> thread, but the qemuMonitorSend() thread stuck for a while, which means > > If you wake up a tread via signalling a condition it _must_ have the > mutex locked prior to executing .... > > Note that after waking up from virCondWait you have the mutex which was > passed to it locked. > Yes, it _must_ have the mutex locked prior to executing, but there is still a chance that the qemuMonitorIO() hold the mutex faster. I mean virCondWait wants to wake up after qemuMonitorIOProcess() broadcast, but it stuck for a while as cpu pressure or scheduleed out or some other reasons. And in such short time, before virCondWait try to hold the mutex, qemu has sent message again, qemuMonitorIO() has been called again, and hold the mutex successfully. So virCondWait will still wait the mutex even qemuMonitorIOProcess() has broadcast. >> the qemu monitor is still unlocked. >> 4. qemu send message to monitor for the second time, such as RTC_CHANGE >> event >> 5. qemuMonitorIOProcess() is called, the local viriable 'msg' is >> assigned to mon->msg. >> 6. qemuMonitorIOProcess() call qemuMonitorJSONIOProcess() to deal with >> the message >> 7. qemuMonitorJSONIOProcess() unlock the qemu monitor, qemuMonitorSend() >> thread get the lock and free the mon->msg, assign mon->msg to NULL. > > The monitor is unlocked in qemuMonitorIO() after finishing processing > from the event loop. There is no point where qemuMonitorIOProcess() > would not hold the mutex locked. > qemuMonitorJSONIOProcess() is called by qemuMonitorIOProcess() to deal with the message, qemuMonitorJSONIOProcess()->qemuMonitorJSONIOProcessLine()-> qemuMonitorJSONIOProcessEvent->qemuMonitorEmitEvent()->QEMU_MONITOR_CALLBACK #define QEMU_MONITOR_CALLBACK(mon, ret, callback, ...) \ do { \ virObjectRef(mon); \ virObjectUnlock(mon); \ if ((mon)->cb && (mon)->cb->callback) \ (ret) = (mon)->cb->callback(mon, __VA_ARGS__, \ (mon)->callbackOpaque); \ virObjectLock(mon); \ virObjectUnref(mon); \ } while (0)