If qemu quited unexpectedly when we call qemuMonitorJSONHMP(), libvirt will crash. Steps to reproduce this bug: 1. use gdb to attach libvirtd, and set a breakpoint in the function qemuMonitorSetCapabilities() 2. start a vm 3. let the libvirtd to run until qemuMonitorJSONSetCapabilities() returns. 4. kill the qemu process 5. continue running libvirtd Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> --- src/qemu/qemu_monitor.c | 9 ++++++++- src/qemu/qemu_monitor_json.c | 9 ++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index eed83f4..647e2bb 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -906,7 +906,14 @@ int qemuMonitorSetCapabilities(qemuMonitorPtr mon) if (mon->json) { ret = qemuMonitorJSONSetCapabilities(mon); - mon->json_hmp = qemuMonitorJSONCheckHMP(mon); + if (ret == 0) { + mon->json_hmp = qemuMonitorJSONCheckHMP(mon); + if (mon->json_hmp < 0) { + /* qemu may quited unexpectedly when we call + * qemuMonitorJSONCheckHMP() */ + ret = -1; + } + } } else { ret = 0; } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 6bd03d6..20a78e1 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -746,10 +746,14 @@ qemuMonitorJSONSetCapabilities(qemuMonitorPtr mon) } +/* + * Returns: 0 if human-monitor-command is not supported, +1 if + * human-monitor-command worked or -1 on failure + */ int qemuMonitorJSONCheckHMP(qemuMonitorPtr mon) { - int ret = 0; + int ret = -1; virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-commands", NULL); virJSONValuePtr reply = NULL; virJSONValuePtr data; @@ -781,6 +785,9 @@ qemuMonitorJSONCheckHMP(qemuMonitorPtr mon) } } + /* human-monitor-command is not supported */ + ret = 0; + cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); -- 1.7.1

On Tue, Mar 29, 2011 at 03:22:40PM +0800, Wen Congyang wrote:

If the monitor met a error, and we will call qemuProcessHandleMonitorEOF(). But we may try to send monitor command after qemuProcessHandleMonitorEOF() returned. Then libvirtd will be blocked in qemuMonitorSend().

Steps to reproduce this bug: 1. use gdb to attach libvirtd, and set a breakpoint in the function qemuConnectMonitor() 2. start a vm 3. let the libvirtd to run until qemuMonitorOpen() returns. 4. kill the qemu process 5. continue running libvirtd

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

--- src/qemu/qemu_monitor.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 800f744..eed83f4 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -572,6 +572,13 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) { mon->msg->lastErrno = EIO; virCondSignal(&mon->notify); } + /* If qemu quited unexpectedly, and we may try to send monitor + * command later. But we have no chance to wake up it. So set + * mon->lastErrno to EIO, and check it before sending monitor + * command. + */ + if (!mon->lastErrno) + mon->lastErrno = EIO; quit = 1; } else if (events) { VIR_ERROR(_("unhandled fd event %d for monitor fd %d"),

I'm wondering if we should just move this safety check a little further down in this method, because the 'else if (events)' branch likely wants this check too. eg I'd put it in the bit where we run the EOF callback something like this: /* We have to unlock to avoid deadlock against command thread, * but is this safe ? I think it is, because the callback * will try to acquire the virDomainObjPtr mutex next */ if (failed || quit) { void (*eofNotify)(qemuMonitorPtr, virDomainObjPtr, int) = mon->cb->eofNotify; virDomainObjPtr vm = mon->vm; + /* If qemu quited unexpectedly, and we may try to send monitor + * command later. But we have no chance to wake up it. So set + * mon->lastErrno to EIO, and check it before sending monitor + * command. + */ + if (!mon->lastErrno) + mon->lastErrno = EIO; /* Make sure anyone waiting wakes up now */ virCondSignal(&mon->notify); if (qemuMonitorUnref(mon) > 0) qemuMonitorUnlock(mon); VIR_DEBUG("Triggering EOF callback error? %d", failed); (eofNotify)(mon, vm, failed);

@@ -725,6 +732,12 @@ int qemuMonitorSend(qemuMonitorPtr mon, { int ret = -1;

+ /* Check whether qemu quited unexpectedly */ + if (mon->lastErrno) { + msg->lastErrno = mon->lastErrno; + return -1; + } + mon->msg = msg; qemuMonitorUpdateWatch(mon);

Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|