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

[Peter Krempa]

On Wed, Jun 06, 2018 at 11:46:07 +0800, zhuweilun wrote:
> 
> 
> 在 2018/6/5 15:10, Peter Krempa 写道:
> > On Tue, Jun 05, 2018 at 10:14:39 +0800, Shannon Zhao wrote:
> >> From: Weilun Zhu <zhuweilun at huawei.com>
> >>
> >> 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)

Ah, okay I did not notice that these unlock the monitor.

That means that the proposed solution is not correct though.

I think a proper solution is to process the events in the same way
normal messages are processed, which is after the monitor is unlocked,
but that is a rather complex fix.

Other possibility is to re-acquire the 'msg' object after the call to
qemuMonitorJSONIOProcess returns. This requires adding a comment why is
it necessary

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180606/3db3de67/attachment-0001.sig>