[libvirt] [PATCH v2] qemu: fix msg could be a wild pointer in qemuMonitorIOProcess()
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 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. 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. --- 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; + if (mon->msg && mon->msg->txOffset == mon->msg->txLength) { + msg = mon->msg; + if (msg && msg->finished) virCondBroadcast(&mon->notify); return len; -- 2.18.0.rc1
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
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.
--- 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.
virCondBroadcast(&mon->notify); return len; -- 2.18.0.rc1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Peter Krempa -
Weilun Zhu -
zhuweilun