Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro

-----Original Message----- From: Eric Blake [mailto:eblake@redhat.com] Sent: Tuesday, September 10, 2013 8:28 PM To: Liuji (Jeremy) Cc: Daniel P. Berrange; libvir-list(a)redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro On 09/10/2013 02:29 AM, Liuji (Jeremy) wrote: >>>>> Yes，it's an actual crash problem. I found the problem in the above >> problem >>>> scenario in my first mail. >> >> A problem scenario: >> 1) The XML of VM contain the below segment: >> <numatune> >> <memory mode='preferred' placement='auto' nodeset='0'/> >> </numatune> >> 2)virsh create the VM >> 3)In the virDomainDefParseXML funtion: >> /* Ignore 'nodeset' if 'placement' is 'auto' finally */ >> if (placement_mode == > VIR_NUMA_TUNE_MEM_PLACEMENT_MODE_AUTO) { >> > virBitmapFree(def->numatune.memory.nodemask); >> def->numatune.memory.nodemask = NULL; >> } >> 4)Then, virsh destroy the VM. In the virDomainDefFree funtion, it >> also call the virBitmapFree function to free the nodemask: >> virBitmapFree(def->numatune.memory.nodemask); >>>> >>>> Then can you send a patch which explicitly fixes that problem on >>>> its own, without doing this major refactoring, which obscures what >>>> is being fixed. >>> >>> I had sent a patch in my first mail. You can find the mail at the following links: >>> >> https://www.redhat.com/archives/libvir-list/2013-September/msg00337.h >> tml >> >> No, I want the root cause fixed. This change to the bitmap APIs is >> just papering over any root cause bug. >> > > This is a simple problem about an address is released twice in some scenario. > I think that released twice is the root cause. I'm not sure what you mean about root cause. > Did you mean that why the address will be released two times? What we want is a patch against virDomainDefParseXML - if it frees the bitmap, it must set the bitmap member to NULL (ie. the root cause is step 3 of your trace above). That will then avoid the second free in virDomainDefFree (step 4 of your trace). The root cause is insidious because leaving the bitmap pointer non-NULL after freeing it means that ANY other use of the bitmap (not just the free in virDomainDefFree) is accessing freed memory. Creating a macro to avoid the double free is overkill; compared to fixing the real bug of freeing but not marking the memory freed.

From 234d8616dba9fa645146def8a8da2562f6b4802b Mon Sep 17 00:00:00 2001