[libvirt] [PATCH] Avoid invoking the qemu monitor destroy callback if the constructor fails
Some, but not all, codepaths in the qemuMonitorOpen() method would trigger the destroy callback. The caller does not expect this to be invoked if construction fails, only during normal release of the monitor. This resulted in a possible double-unref of the virDomainObjPtr, because the caller explicitly unrefs the virDomainObjPtr if qemuMonitorOpen() fails * src/qemu/qemu_monitor.c: Don't invoke destroy callback from qemuMonitorOpen() failure paths --- src/qemu/qemu_monitor.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f428665..ff613a0 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -671,6 +671,12 @@ qemuMonitorOpen(virDomainObjPtr vm, return mon; cleanup: + /* We don't want the 'destroy' callback invoked during + * cleanup from construction failure, because that can + * give a double-unref on virDomainObjPtr in the caller, + * so kill the callbacks now. + */ + mon->cb = NULL; qemuMonitorUnlock(mon); qemuMonitorClose(mon); return NULL; -- 1.6.6.1
On 06/29/2010 05:02 AM, Daniel P. Berrange wrote:
Some, but not all, codepaths in the qemuMonitorOpen() method would trigger the destroy callback. The caller does not expect this to be invoked if construction fails, only during normal release of the monitor. This resulted in a possible double-unref of the virDomainObjPtr, because the caller explicitly unrefs the virDomainObjPtr if qemuMonitorOpen() fails
* src/qemu/qemu_monitor.c: Don't invoke destroy callback from qemuMonitorOpen() failure paths
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f428665..ff613a0 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -671,6 +671,12 @@ qemuMonitorOpen(virDomainObjPtr vm, return mon;
cleanup: + /* We don't want the 'destroy' callback invoked during + * cleanup from construction failure, because that can + * give a double-unref on virDomainObjPtr in the caller, + * so kill the callbacks now. + */ + mon->cb = NULL; qemuMonitorUnlock(mon); qemuMonitorClose(mon); return NULL;
Unfortunately, this patch causes segfaults since qemuMonitorFree is not ready to see mon->cb == NULL. On the other hand, we are lucky that this patch didn't make it into the repository yet, so we can squash the following patch into it before pushing: diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f428665..9b050a0 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -198,7 +198,7 @@ void qemuMonitorUnlock(qemuMonitorPtr mon) static void qemuMonitorFree(qemuMonitorPtr mon) { VIR_DEBUG("mon=%p", mon); - if (mon->cb->destroy) + if (mon->cb && mon->cb->destroy) (mon->cb->destroy)(mon, mon->vm); if (virCondDestroy(&mon->notify) < 0) {} Jirka
On Wed, Jun 30, 2010 at 02:40:58PM +0200, Jiri Denemark wrote:
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f428665..ff613a0 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -671,6 +671,12 @@ qemuMonitorOpen(virDomainObjPtr vm, return mon;
cleanup: + /* We don't want the 'destroy' callback invoked during + * cleanup from construction failure, because that can + * give a double-unref on virDomainObjPtr in the caller, + * so kill the callbacks now. + */ + mon->cb = NULL; qemuMonitorUnlock(mon); qemuMonitorClose(mon); return NULL;
Unfortunately, this patch causes segfaults since qemuMonitorFree is not ready to see mon->cb == NULL. On the other hand, we are lucky that this patch didn't make it into the repository yet, so we can squash the following patch into it before pushing:
ACK, I've included this in the patch & pushed the result
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f428665..9b050a0 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -198,7 +198,7 @@ void qemuMonitorUnlock(qemuMonitorPtr mon) static void qemuMonitorFree(qemuMonitorPtr mon) { VIR_DEBUG("mon=%p", mon); - if (mon->cb->destroy) + if (mon->cb && mon->cb->destroy) (mon->cb->destroy)(mon, mon->vm); if (virCondDestroy(&mon->notify) < 0) {}
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Jiri Denemark