From: "Liuji (Jeremy)" <jeremy.liu(a)huawei.com&gt; 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(a)huawei.com&gt; --- 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.

-----Original Message----- From: Osier Yang [mailto:jyang@redhat.com] Sent: Monday, September 09, 2013 10:46 AM To: Liuji (Jeremy) Cc: libvir-list(a)redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro On 06/09/13 18:30, Liuji (Jeremy) wrote: > The parameter of virBitmapFree function is just a pointer, not a pointer of pointer. > The second VIR_FREE on virBitmapFree only assign NULL to the formal parameter. > After calling the virBitmapFree function, the actual parameter are still not NULL. > There are many code segment don't assign NULL to the formal parameter after calling > the virBitmapFree function. This will bring potential risks. > > 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); > But after this call, the value of def->numatune.memory.nodemask is still not NULL. > This will generate an exception. > > > >From d2b69b130bca89df85005d395c6d45d8df0b74f1 Mon Sep 17 00:00:00 2001 > From: "Liuji (Jeremy)" <jeremy.liu(a)huawei.com&gt; > Date: Fri, 6 Sep 2013 04:49:30 -0400 > Subject: [PATCH] virBitmapFree: Change the function to a macro > > The parameter of virBitmapFree function is just a pointer, not a pointer of pointer. > The second VIR_FREE on virBitmapFree only assign NULL to the formal parameter. > After calling the virBitmapFree function, the actual parameter are still not NULL. > There are many code segment don't assign NULL to the formal parameter after calling > the virBitmapFree function. This will bring potential risks. Regardless of what the problem is...... > > Signed-off-by: Liuji (Jeremy) <jeremy.liu(a)huawei.com&gt; > --- > src/util/virbitmap.c | 21 --------------------- > src/util/virbitmap.h | 21 ++++++++++++++++----- > 2 files changed, 16 insertions(+), 26 deletions(-) > > diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c > index 7e1cd02..cde502f 100644 > --- a/src/util/virbitmap.c > +++ b/src/util/virbitmap.c > @@ -40,13 +40,6 @@ > > #define VIR_FROM_THIS VIR_FROM_NONE > > -struct _virBitmap { > - size_t max_bit; > - size_t map_len; > - unsigned long *map; > -}; > - > - > #define VIR_BITMAP_BITS_PER_UNIT ((int) sizeof(unsigned long) * CHAR_BIT) > #define VIR_BITMAP_UNIT_OFFSET(b) ((b) / VIR_BITMAP_BITS_PER_UNIT) > #define VIR_BITMAP_BIT_OFFSET(b) ((b) % VIR_BITMAP_BITS_PER_UNIT) > @@ -88,20 +81,6 @@ virBitmapPtr virBitmapNew(size_t size) > return bitmap; > } > > -/** > - * virBitmapFree: > - * @bitmap: previously allocated bitmap > - * > - * Free @bitmap previously allocated by virBitmapNew. > - */ > -void virBitmapFree(virBitmapPtr bitmap) > -{ > - if (bitmap) { > - VIR_FREE(bitmap->map); > - VIR_FREE(bitmap); > - } > -} > - > > int virBitmapCopy(virBitmapPtr dst, virBitmapPtr src) > { > diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h > index b682523..9b93b88 100644 > --- a/src/util/virbitmap.h > +++ b/src/util/virbitmap.h > @@ -28,6 +28,22 @@ > > # include <sys/types.h> > > +struct _virBitmap { > + size_t max_bit; > + size_t map_len; > + unsigned long *map; > +}; > + > +/* > + * Free previously allocated bitmap > + */ > +#define virBitmapFree(bitmap) \ > + do { \ > + if (bitmap) { \ > + VIR_FREE((bitmap)->map); \ > + VIR_FREE(bitmap); \ > + } \ > + } while (0); > ... What does this make difference? Unless I missed something, what you do is just changing the function into a macro. And I don't see any benifit from it. Osier

-----Original Message----- From: Osier Yang [mailto:jyang@redhat.com] Sent: Monday, September 09, 2013 6:15 PM To: Liuji (Jeremy) Cc: libvir-list(a)redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro [......] >> Regardless of what the problem is...... >> >>> Signed-off-by: Liuji (Jeremy) <jeremy.liu(a)huawei.com&gt; >>> --- >>> src/util/virbitmap.c | 21 --------------------- >>> src/util/virbitmap.h | 21 ++++++++++++++++----- >>> 2 files changed, 16 insertions(+), 26 deletions(-) >>> >>> diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c >>> index 7e1cd02..cde502f 100644 >>> --- a/src/util/virbitmap.c >>> +++ b/src/util/virbitmap.c >>> @@ -40,13 +40,6 @@ >>> >>> #define VIR_FROM_THIS VIR_FROM_NONE >>> >>> -struct _virBitmap { >>> - size_t max_bit; >>> - size_t map_len; >>> - unsigned long *map; >>> -}; >>> - >>> - >>> #define VIR_BITMAP_BITS_PER_UNIT ((int) sizeof(unsigned long) * >> CHAR_BIT) >>> #define VIR_BITMAP_UNIT_OFFSET(b) ((b) / >> VIR_BITMAP_BITS_PER_UNIT) >>> #define VIR_BITMAP_BIT_OFFSET(b) ((b) % >> VIR_BITMAP_BITS_PER_UNIT) >>> @@ -88,20 +81,6 @@ virBitmapPtr virBitmapNew(size_t size) >>> return bitmap; >>> } >>> >>> -/** >>> - * virBitmapFree: >>> - * @bitmap: previously allocated bitmap >>> - * >>> - * Free @bitmap previously allocated by virBitmapNew. >>> - */ >>> -void virBitmapFree(virBitmapPtr bitmap) >>> -{ >>> - if (bitmap) { >>> - VIR_FREE(bitmap->map); >>> - VIR_FREE(bitmap); >>> - } >>> -} >>> - >>> >>> int virBitmapCopy(virBitmapPtr dst, virBitmapPtr src) >>> { >>> diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h >>> index b682523..9b93b88 100644 >>> --- a/src/util/virbitmap.h >>> +++ b/src/util/virbitmap.h >>> @@ -28,6 +28,22 @@ >>> >>> # include <sys/types.h> >>> >>> +struct _virBitmap { >>> + size_t max_bit; >>> + size_t map_len; >>> + unsigned long *map; >>> +}; >>> + >>> +/* >>> + * Free previously allocated bitmap >>> + */ >>> +#define virBitmapFree(bitmap) \ >>> + do { \ >>> + if (bitmap) { \ >>> + VIR_FREE((bitmap)->map); \ >>> + VIR_FREE(bitmap); \ >>> + } \ >>> + } while (0); >>> >> ... What does this make difference? Unless I missed something, what you >> do is >> just changing the function into a macro. And I don't see any benifit >> from it. >> >> Osier > I thought I have already clearly explained the problem. > > The virBitmapFree is: > void virBitmapFree(virBitmapPtr bitmap) > { > if (bitmap) { > VIR_FREE(bitmap->map); > VIR_FREE(bitmap); > } > } > Bitmap is a parameter (formal parameter). The address which is pointed by Bitmap is the same as argument(actual parameter) pointed. > In " VIR_FREE(bitmap);", it frees the same address. But only assigns the NULL to the Bitmap, not to the argument(actual parameter). > After calling virBitmapFree, if there is not assigning NULL to the argument(actual parameter), and using this argument(actual parameter) to call > the virBitmapFree again, it will free an already freed address. It will generate a crash. I knew your meaning, what I was wondering was: > > There are 3 solutions: > 1> After call virBitmapFree function, add a assignment which is assign NULL to the argument(actual parameter) of virBitmapFree. > 2> Change the virBitmapFree to : > void virBitmapFree(virBitmapPtr *bitmap) > { > if (*bitmap) { > VIR_FREE((*bitmap)->map); > VIR_FREE(*bitmap); > } > } > And change all virBitmapFree calling from > virBitmapFree(abc) > to > virBitmapFree(&abc) > 3> change the virBitmapFree to a macro, like my first mail. > 1) Regardless of what the problem is, and whether to change the interface of virBitmapFree or introduce a macro, I didn't see you use it, I.E fixing the crash problems. Without the changes on the use of virBitmapFree, the patch is quite confused between the commit log and the patch itself. 2) And actually the macro still doesn't help if we don't pass a "virBitmapPtr *" argument for it. So the patch doesn't change things.

I prefer to change the interface of virBitmapFree instead, since it will need the "virBitmapPtr *" argument anyway. Osier