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

[zhuweilun]

Hi Peter,
Thanks a lot for your review! I'm so sorry for the delay, please see my reply below.

在 2018/6/8 16:05, Peter Krempa 写道:
> On Thu, Jun 07, 2018 at 15:09:58 +0800, Weilun Zhu wrote:
>> 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
> 
> I'd write this as:
> 
> As qemuMonitorJSONIOProcess will call qemuMonitorJSONIOProcessEvent
> which unlocks the monitor mutex
> 

All right, it's much better.

>> 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 as cpu pressure
>> or some other reasons,, which means the qemu monitor is still unlocked.
>> 4. qemu send event 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 qemu event.
>> 7. qemuMonitorJSONIOProcess() unlock the qemu monitor in the macro
>> 'QEMU_MONITOR_CALLBACK', then qemuMonitorSend() thread get the mutex
>> and free the mon->msg, assign mon->msg to NULL.
> 
> This is okay
> 
>>
>> so the local viriable 'msg' of qemuMonitorIOProcess() is a wild pointer
>> now.
>>
>> AFAIK, it is not harmful to call again virCondBroadcast() while msg is a
>> wild pointer, but just in case,  we fix it in this patch.
> 
> These two paragraphs can be dropped.
> 
> 
> Also we now require that the authors of patches agree to the
> 'Developer's certificate of origin' ( https://developercertificate.org/
> ). You express your agreement by adding a 'Signed-off-by' line. Without
> that, your patch can't be pushed.
> 
> 

OK, I did not notice that, I will add it.

>> ---
>>  src/qemu/qemu_monitor.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
>> index 215135aa3e..a4b8572e24 100644
>> --- a/src/qemu/qemu_monitor.c
>> +++ b/src/qemu/qemu_monitor.c
>> @@ -463,6 +463,14 @@ qemuMonitorIOProcess(qemuMonitorPtr mon)
>>  #if DEBUG_IO
>>      VIR_DEBUG("Process done %d used %d", (int)mon->bufferOffset, len);
>>  #endif
>> +
>> +    /* As the monitor mutex was unlocked in qemuMonitorJSONIOProcess()
>> +     * while dealing with qemu event, mon->msg could be changed,
>> +     * thus we re-acquire the msg here */
>> +    msg = NULL;
> 
> This part is okay (except for the last line if you apply what I suggest
> bellow)
> 
>> +    if (mon->msg && mon->msg->txOffset == mon->msg->txLength) {
>> +        msg = mon->msg;
> 
> But this condition can be merged with the one below.
> 
>> +
>>      if (msg && msg->finished)
> 
> It should look like:
>     if (mon->msg && mon->msg->finished)
>         virCondBroadcast(&mon->notify);
> 
> The part with the txOffset is not necessary any more, since
> msg->finished will be set only when that was true. The main reasoning is
> that we don't really need to extract msg at this point any more.
> 
> 

OK, your suggest is better, and I will change the comment like following:
"
-    if (msg && msg->finished)
+
+    /* As the monitor mutex was unlocked in qemuMonitorJSONIOProcess()
+     * while dealing with qemu event, mon->msg could be changed which
+     * means the above 'msg' may be invalid, thus we use 'mon->msg' here */
+    if (mon->msg && mon->msg->finished)
"

>>          virCondBroadcast(&mon->notify);
>>      return len;
>> -- 
>> 2.18.0.rc1
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list