[libvirt] [PATCH] Set to NULL members that have been freed to prevent crashes

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org