[libvirt] [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. 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@huawei.com> 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. Signed-off-by: Liuji (Jeremy) <jeremy.liu@huawei.com> --- 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); typedef struct _virBitmap virBitmap; typedef virBitmap *virBitmapPtr; @@ -38,11 +54,6 @@ typedef virBitmap *virBitmapPtr; virBitmapPtr virBitmapNew(size_t size) ATTRIBUTE_RETURN_CHECK; /* - * Free previously allocated bitmap - */ -void virBitmapFree(virBitmapPtr bitmap); - -/* * Copy all bits from @src to @dst. The bitmap sizes * must be the same */ -- 1.8.1.4
On Fri, Sep 06, 2013 at 10:30:56AM +0000, 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.
Have you got an actual crash happening today, or is this just a theoretical problem you're trying to address ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Friday, September 06, 2013 6:37 PM To: Liuji (Jeremy) Cc: libvir-list@redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro
The parameter of virBitmapFree function is just a pointer, not a pointer of
The second VIR_FREE on virBitmapFree only assign NULL to the formal
On Fri, Sep 06, 2013 at 10:30:56AM +0000, Liuji (Jeremy) wrote: pointer. 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.
Have you got an actual crash happening today, or is this just a theoretical problem you're trying to address ?
Yes，it's an actual crash problem. I found the problem in the above problem scenario in my first mail. But, my first mail has a mistake: The description of step 3 and 4 of "A problem scenario:" should be like this: 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); 4)Then, virsh destroy the VM. In the virDomainDefFree funtion, it also call the virBitmapFree funtion to free the nodemask: virBitmapFree(def->numatune.memory.nodemask); But before this call, the value of def->numatune.memory.nodemask is still not NULL, and the address which is pointed by the pointer has been freed. The VIR_FREE on virBitmapFree function will free the address again.
On Mon, Sep 09, 2013 at 01:39:42AM +0000, Liuji (Jeremy) wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Friday, September 06, 2013 6:37 PM To: Liuji (Jeremy) Cc: libvir-list@redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro
The parameter of virBitmapFree function is just a pointer, not a pointer of
The second VIR_FREE on virBitmapFree only assign NULL to the formal
On Fri, Sep 06, 2013 at 10:30:56AM +0000, Liuji (Jeremy) wrote: pointer. 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.
Have you got an actual crash happening today, or is this just a theoretical problem you're trying to address ?
Yes，it's an actual crash problem. I found the problem in the above problem scenario in my first mail.
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. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Monday, September 09, 2013 7:26 PM To: Liuji (Jeremy) Cc: libvir-list@redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Friday, September 06, 2013 6:37 PM To: Liuji (Jeremy) Cc: libvir-list@redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro
The parameter of virBitmapFree function is just a pointer, not a pointer of
The second VIR_FREE on virBitmapFree only assign NULL to the formal
On Fri, Sep 06, 2013 at 10:30:56AM +0000, Liuji (Jeremy) wrote: pointer. 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
On Mon, Sep 09, 2013 at 01:39:42AM +0000, Liuji (Jeremy) wrote: 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.
Have you got an actual crash happening today, or is this just a theoretical problem you're trying to address ?
Yes，it's an actual crash problem. I found the problem in the above problem scenario in my first mail.
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.html Jeremy
On Tue, Sep 10, 2013 at 01:29:40AM +0000, Liuji (Jeremy) wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Monday, September 09, 2013 7:26 PM To: Liuji (Jeremy) Cc: libvir-list@redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Friday, September 06, 2013 6:37 PM To: Liuji (Jeremy) Cc: libvir-list@redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro
The parameter of virBitmapFree function is just a pointer, not a pointer of
The second VIR_FREE on virBitmapFree only assign NULL to the formal
On Fri, Sep 06, 2013 at 10:30:56AM +0000, Liuji (Jeremy) wrote: pointer. 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
On Mon, Sep 09, 2013 at 01:39:42AM +0000, Liuji (Jeremy) wrote: 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.
Have you got an actual crash happening today, or is this just a theoretical problem you're trying to address ?
Yes，it's an actual crash problem. I found the problem in the above problem scenario in my first mail.
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.html
No, I want the root cause fixed. This change to the bitmap APIs is just papering over any root cause bug. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Tuesday, September 10, 2013 3:57 PM To: Liuji (Jeremy) Cc: libvir-list@redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Monday, September 09, 2013 7:26 PM To: Liuji (Jeremy) Cc: libvir-list@redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro
On Mon, Sep 09, 2013 at 01:39:42AM +0000, Liuji (Jeremy) wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Friday, September 06, 2013 6:37 PM To: Liuji (Jeremy) Cc: libvir-list@redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro
On Fri, Sep 06, 2013 at 10:30:56AM +0000, Liuji (Jeremy) wrote:
The parameter of virBitmapFree function is just a pointer, not a
The second VIR_FREE on virBitmapFree only assign NULL to the formal
pointer. 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
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.
Have you got an actual crash happening today, or is this just a
problem you're trying to address ?
Yes，it's an actual crash problem. I found the problem in the above
On Tue, Sep 10, 2013 at 01:29:40AM +0000, Liuji (Jeremy) wrote: pointer of parameter theoretical problem
scenario in my first mail.
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.html
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?
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.html
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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-----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@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@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@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
On 09/11/2013 04:27 AM, Liuji (Jeremy) wrote:
From: "Liuji (Jeremy)" <jeremy.liu@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@huawei.com> --- src/conf/domain_conf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
ACK, I've added a link to bugzilla and pushed it: https://bugzilla.redhat.com/show_bug.cgi?id=1006710 Jan
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.
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@huawei.com> 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@huawei.com> --- 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 10:46 AM To: Liuji (Jeremy) Cc: libvir-list@redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro
The parameter of virBitmapFree function is just a pointer, not a pointer of
The second VIR_FREE on virBitmapFree only assign NULL to the formal
On 06/09/13 18:30, Liuji (Jeremy) wrote: pointer. 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
This will generate an exception.
From d2b69b130bca89df85005d395c6d45d8df0b74f1 Mon Sep 17 00:00:00 2001 From: "Liuji (Jeremy)" <jeremy.liu@huawei.com> 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
The second VIR_FREE on virBitmapFree only assign NULL to the formal
NULL. pointer. 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@huawei.com> --- 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. 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. Because the 1 and 2 solutions will modify the code in many code segment. So, I prefer the 3 solution.
[......]
Regardless of what the problem is......
Signed-off-by: Liuji (Jeremy) <jeremy.liu@huawei.com> --- 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
-----Original Message----- From: Osier Yang [mailto:jyang@redhat.com] Sent: Monday, September 09, 2013 6:15 PM To: Liuji (Jeremy) Cc: libvir-list@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@huawei.com> --- 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.
In my patch, no need to modify the virBitmapFree called code, I.E NO need to change virBitmapFree(abc) to virBitmapFree(&abc) After the virBitmapFree was changed to a macro, there is no parameter(formal parameter). virBitmapFree directly operate the argument(actual parameter).
I prefer to change the interface of virBitmapFree instead, since it will need the "virBitmapPtr *" argument anyway.
Osier
On 09/09/2013 05:57 AM, Liuji (Jeremy) wrote:
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.
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.
Because the 1 and 2 solutions will modify the code in many code segment. So, I prefer the 3 solution.
Most of the code doesn't use the bitmap after it's freed, it seems it's just the one case of numatune nodemask that needs to be fixed. I vote for option (1). I think it's better to be consistent with what other *Free functions do. Jan
On 09/09/13 19:24, Ján Tomko wrote:
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.
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.
Because the 1 and 2 solutions will modify the code in many code segment. So, I prefer the 3 solution. Most of the code doesn't use the bitmap after it's freed, it seems it's just
On 09/09/2013 05:57 AM, Liuji (Jeremy) wrote: the one case of numatune nodemask that needs to be fixed. I vote for option (1). I think it's better to be consistent with what other *Free functions do.
Not really, a dangling pointer always has potential risks. It's never known whether if it will be used later, especially when the memory it pointed to is allocated for some other purpose. Causing crash is actually the better result than unexpected ones. Osier
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Ján Tomko -
Liuji (Jeremy) -
Osier Yang