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

[Liuji (Jeremy)]

> -----Original Message-----
> From: Eric Blake [mailto:eblake at redhat.com]
> Sent: Tuesday, September 10, 2013 8:28 PM
> To: Liuji (Jeremy)
> Cc: Daniel P. Berrange; libvir-list at 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.
> 

Thank you very much for your detailed advice. 
The following is the latest patch.


>From 234d8616dba9fa645146def8a8da2562f6b4802b Mon Sep 17 00:00:00 2001
From: "Liuji (Jeremy)" <jeremy.liu at huawei.com>
Date: Tue, 10 Sep 2013 22:13:32 -0400
Subject: [PATCH] virDomainDefParseXML: set the argument of virBitmapFree to
 NULL after calling virBitmapFree

After freeing the bitmap pointer, it must set the pointer to NULL.
This will avoid any other use of the freed memory of the bitmap pointer.

Signed-off-by: Liuji (Jeremy) <jeremy.liu at huawei.com>
---
 src/conf/domain_conf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e3aec69..9d1e9fc 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11220,8 +11220,10 @@ virDomainDefParseXML(xmlDocPtr xml,
                     }

                     /* Ignore 'nodeset' if 'placement' is 'auto' finally */
-                    if (placement_mode == VIR_NUMA_TUNE_MEM_PLACEMENT_MODE_AUTO)
+                    if (placement_mode == VIR_NUMA_TUNE_MEM_PLACEMENT_MODE_AUTO) {
                         virBitmapFree(def->numatune.memory.nodemask);
+                        def->numatune.memory.nodemask = NULL;
+                    }

                     /* Copy 'placement' of <numatune> to <vcpu> if its 'placement'
                      * is not specified and 'placement' of <numatune> is specified.
-- 
1.8.1.4