[libvirt] [PATCH] Set to NULL members that have been freed to prevent crashes
Do not crash if virStreamFinish is called after error. ==11000== Invalid read of size 4 ==11000== at 0x373A8099A0: pthread_mutex_lock (pthread_mutex_lock.c:51) ==11000== by 0x4C7CADE: virMutexLock (threads-pthread.c:85) ==11000== by 0x4D57C31: virNetClientStreamRaiseError (virnetclientstream.c:203) ==11000== by 0x4D385E4: remoteStreamFinish (remote_driver.c:3541) ==11000== by 0x4D182F9: virStreamFinish (libvirt.c:14157) ==11000== by 0x40FDC4: cmdScreenshot (virsh.c:3075) ==11000== by 0x42BA40: vshCommandRun (virsh.c:14922) ==11000== by 0x42ECCA: main (virsh.c:16381) ==11000== Address 0x59b86c0 is 16 bytes inside a block of size 216 free'd ==11000== at 0x4A06928: free (vg_replace_malloc.c:427) ==11000== by 0x4C69E2B: virFree (memory.c:310) ==11000== by 0x4D57B56: virNetClientStreamFree (virnetclientstream.c:184) ==11000== by 0x4D3DB7A: remoteDomainScreenshot (remote_client_bodies.h:1812) ==11000== by 0x4CFD245: virDomainScreenshot (libvirt.c:2903) ==11000== by 0x40FB73: cmdScreenshot (virsh.c:3029) ==11000== by 0x42BA40: vshCommandRun (virsh.c:14922) ==11000== by 0x42ECCA: main (virsh.c:16381) --- src/rpc/gendispatch.pl | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 039d785..b7ac3c8 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -1480,6 +1480,8 @@ elsif ($opt_k) { if ($call->{streamflag} ne "none") { print " virNetClientRemoveStream(priv->client, netst);\n"; print " virNetClientStreamFree(netst);\n"; + print " st->driver = NULL;\n"; + print " st->privateData = NULL;\n"; } print " goto done;\n"; -- 1.7.6.2
On 09/30/2011 07:39 PM, Marc-André Lureau wrote:
Do not crash if virStreamFinish is called after error.
==11000== Invalid read of size 4 ==11000== at 0x373A8099A0: pthread_mutex_lock (pthread_mutex_lock.c:51) ==11000== by 0x4C7CADE: virMutexLock (threads-pthread.c:85) ==11000== by 0x4D57C31: virNetClientStreamRaiseError (virnetclientstream.c:203) ==11000== by 0x4D385E4: remoteStreamFinish (remote_driver.c:3541) ==11000== by 0x4D182F9: virStreamFinish (libvirt.c:14157) ==11000== by 0x40FDC4: cmdScreenshot (virsh.c:3075) ==11000== by 0x42BA40: vshCommandRun (virsh.c:14922) ==11000== by 0x42ECCA: main (virsh.c:16381) ==11000== Address 0x59b86c0 is 16 bytes inside a block of size 216 free'd ==11000== at 0x4A06928: free (vg_replace_malloc.c:427) ==11000== by 0x4C69E2B: virFree (memory.c:310) ==11000== by 0x4D57B56: virNetClientStreamFree (virnetclientstream.c:184) ==11000== by 0x4D3DB7A: remoteDomainScreenshot (remote_client_bodies.h:1812) ==11000== by 0x4CFD245: virDomainScreenshot (libvirt.c:2903) ==11000== by 0x40FB73: cmdScreenshot (virsh.c:3029) ==11000== by 0x42BA40: vshCommandRun (virsh.c:14922) ==11000== by 0x42ECCA: main (virsh.c:16381) --- src/rpc/gendispatch.pl | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
Took me a while to read the code, including the generated src/remote/remote_client_bodies.h before and after the patch, to convince myself, but this does indeed look like the correct fix. cmdScreenshot definitely triggers this, but several other commands were fixed in the process, such as peer2peer migration, storage volume download, remote console support, and so on.
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 039d785..b7ac3c8 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -1480,6 +1480,8 @@ elsif ($opt_k) { if ($call->{streamflag} ne "none") { print " virNetClientRemoveStream(priv->client, netst);\n"; print " virNetClientStreamFree(netst);\n"; + print " st->driver = NULL;\n"; + print " st->privateData = NULL;\n";
The second line is not strictly necessary; the first is sufficient to prevent the crash. But it also doesn't hurt, and mirrors the fact that both members of st were just set away from NULL earlier on in each of the same functions that now have the new cleanup. ACK and pushed as-is. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Marc-André Lureau