Re: [Libvir] [patch 1/5] iptables: fix invalid free
On Wed, Mar 21, 2007 at 12:47:58PM +0000, Mark McLoughlin wrote:
In iptablesContextNew(), make sure we don't try and free an invalid pointer if one of the iptRulesNew() fails.
Signed-off-by: Mark McLoughlin <markmc@redhat.com>
Index: libvirt/qemud/iptables.c =================================================================== --- libvirt.orig/qemud/iptables.c +++ libvirt/qemud/iptables.c @@ -496,7 +496,7 @@ iptablesContextNew(void) { iptablesContext *ctx;
- if (!(ctx = (iptablesContext *) malloc(sizeof (iptablesContext)))) + if (!(ctx = (iptablesContext *) calloc(1, sizeof (iptablesContext)))) return NULL;
if (!(ctx->input_filter = iptRulesNew("filter", IPTABLES_PREFIX "INPUT")))
I usually prefer malloc + memset( , 0, ) , but this probably comes from libxml2 where I replaced malloc calls with specific wrappers (and I still have a TODO for this in libvirt though some part of libvirt are not linked to libxml2 I guess so that may make things a bit harder) What's the policy w.r.t. error reporting in qemud and libvirt related daemons in general ? I guess a failure to malloc or thisd kind of problems should be logged somewhere, right ?
@@ -518,9 +518,12 @@ iptablesContextNew(void) void iptablesContextFree(iptablesContext *ctx) { - iptRulesFree(ctx->input_filter); - iptRulesFree(ctx->forward_filter); - iptRulesFree(ctx->nat_postrouting); + if (ctx->input_filter) + iptRulesFree(ctx->input_filter); + if (ctx->forward_filter) + iptRulesFree(ctx->forward_filter); + if (ctx->nat_postrouting) + iptRulesFree(ctx->nat_postrouting); free(ctx); }
The patch does the right thing, sounds good to me :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, 2007-03-21 at 09:03 -0400, Daniel Veillard wrote:
On Wed, Mar 21, 2007 at 12:47:58PM +0000, Mark McLoughlin wrote:
In iptablesContextNew(), make sure we don't try and free an invalid pointer if one of the iptRulesNew() fails.
Signed-off-by: Mark McLoughlin <markmc@redhat.com>
Index: libvirt/qemud/iptables.c =================================================================== --- libvirt.orig/qemud/iptables.c +++ libvirt/qemud/iptables.c @@ -496,7 +496,7 @@ iptablesContextNew(void) { iptablesContext *ctx;
- if (!(ctx = (iptablesContext *) malloc(sizeof (iptablesContext)))) + if (!(ctx = (iptablesContext *) calloc(1, sizeof (iptablesContext)))) return NULL;
if (!(ctx->input_filter = iptRulesNew("filter", IPTABLES_PREFIX "INPUT")))
I usually prefer malloc + memset( , 0, ) , but this probably comes from libxml2 where I replaced malloc calls with specific wrappers (and I still have a TODO for this in libvirt though some part of libvirt are not linked to libxml2 I guess so that may make things a bit harder)
If libvirt was going to have it's own malloc() wrapper, I'd suggest that the wrapper should always zero out the allocated memory. Then we could just replace the calloc() with the wrapper. Don't know why we'd use the libxml2 wrappers instead of our own?
What's the policy w.r.t. error reporting in qemud and libvirt related daemons in general ? I guess a failure to malloc or thisd kind of problems should be logged somewhere, right ?
Yep. In this case it would be logged either to syslog if the daemon was autostarting the network or returned to the user in the case where the user is manually starting network. Thanks, Mark.
On Wed, Mar 21, 2007 at 01:30:00PM +0000, Mark McLoughlin wrote:
On Wed, 2007-03-21 at 09:03 -0400, Daniel Veillard wrote:
On Wed, Mar 21, 2007 at 12:47:58PM +0000, Mark McLoughlin wrote:
In iptablesContextNew(), make sure we don't try and free an invalid pointer if one of the iptRulesNew() fails.
Signed-off-by: Mark McLoughlin <markmc@redhat.com>
Index: libvirt/qemud/iptables.c =================================================================== --- libvirt.orig/qemud/iptables.c +++ libvirt/qemud/iptables.c @@ -496,7 +496,7 @@ iptablesContextNew(void) { iptablesContext *ctx;
- if (!(ctx = (iptablesContext *) malloc(sizeof (iptablesContext)))) + if (!(ctx = (iptablesContext *) calloc(1, sizeof (iptablesContext)))) return NULL;
if (!(ctx->input_filter = iptRulesNew("filter", IPTABLES_PREFIX "INPUT")))
I usually prefer malloc + memset( , 0, ) , but this probably comes from libxml2 where I replaced malloc calls with specific wrappers (and I still have a TODO for this in libvirt though some part of libvirt are not linked to libxml2 I guess so that may make things a bit harder)
If libvirt was going to have it's own malloc() wrapper, I'd suggest that the wrapper should always zero out the allocated memory. Then we could just replace the calloc() with the wrapper.
Well, one of the things I do in my wrapper is initilize to -1 all newly allocated bytes, allows to pinpoint relatively easilly when you assumed zeroed memory which was not, while not paying for the initialization when not needed. Granted libvirt CPU usage can be considered neglectible compared to otehr parts of the infrastructure.
Don't know why we'd use the libxml2 wrappers instead of our own?
Because tehy exist, that libvirt uses libxml2, and that would integrate the 2 memory reports
What's the policy w.r.t. error reporting in qemud and libvirt related daemons in general ? I guess a failure to malloc or thisd kind of problems should be logged somewhere, right ?
Yep. In this case it would be logged either to syslog if the daemon was autostarting the network or returned to the user in the case where the user is manually starting network.
okay, I assume those reporting layers are missing ATM, right ? Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, 2007-03-21 at 09:39 -0400, Daniel Veillard wrote:
On Wed, Mar 21, 2007 at 01:30:00PM +0000, Mark McLoughlin wrote:
On Wed, 2007-03-21 at 09:03 -0400, Daniel Veillard wrote:
On Wed, Mar 21, 2007 at 12:47:58PM +0000, Mark McLoughlin wrote:
In iptablesContextNew(), make sure we don't try and free an invalid pointer if one of the iptRulesNew() fails.
Signed-off-by: Mark McLoughlin <markmc@redhat.com>
Index: libvirt/qemud/iptables.c =================================================================== --- libvirt.orig/qemud/iptables.c +++ libvirt/qemud/iptables.c @@ -496,7 +496,7 @@ iptablesContextNew(void) { iptablesContext *ctx;
- if (!(ctx = (iptablesContext *) malloc(sizeof (iptablesContext)))) + if (!(ctx = (iptablesContext *) calloc(1, sizeof (iptablesContext)))) return NULL;
if (!(ctx->input_filter = iptRulesNew("filter", IPTABLES_PREFIX "INPUT")))
I usually prefer malloc + memset( , 0, ) , but this probably comes from libxml2 where I replaced malloc calls with specific wrappers (and I still have a TODO for this in libvirt though some part of libvirt are not linked to libxml2 I guess so that may make things a bit harder)
If libvirt was going to have it's own malloc() wrapper, I'd suggest that the wrapper should always zero out the allocated memory. Then we could just replace the calloc() with the wrapper.
Well, one of the things I do in my wrapper is initilize to -1 all newly allocated bytes, allows to pinpoint relatively easilly when you assumed zeroed memory which was not, while not paying for the initialization when not needed. Granted libvirt CPU usage can be considered neglectible compared to otehr parts of the infrastructure.
So: - I think the use case is a little different - generally in libvirt, we're only allocating very small chunks where the CPU hit for initialisation would be negligible and would never show up on a profile. I'd prefer to take the minor hit of zero-initialising most/all memory for programming ease. - If our wrappers always zero-initialise, we don't need the "initialise to -1 when debugging" thing. - If we rely on calloc() zero-initialising in our wrappers, we give opportunity for libc to optimise where it knows the memory is already initialised - e.g. where it's mmap()ing the memory from /dev/zero
Don't know why we'd use the libxml2 wrappers instead of our own?
Because tehy exist, that libvirt uses libxml2, and that would integrate the 2 memory reports
Ah, memory usage reports ... nice. Would such a report be less useful with the two mixed together, though? e.g. I personally would just like to see the libvirt memory usage, rather than libxml2, in such a report. Again, though, I think libvirt has slightly different needs from a malloc() wrapper.
What's the policy w.r.t. error reporting in qemud and libvirt related daemons in general ? I guess a failure to malloc or thisd kind of problems should be logged somewhere, right ?
Yep. In this case it would be logged either to syslog if the daemon was autostarting the network or returned to the user in the case where the user is manually starting network.
okay, I assume those reporting layers are missing ATM, right ?
Nope, it's all there. See, in qemudAddIptablesRules() we set VIR_ERR_NO_MEMORY if iptablesContextNew() returns NULL, qemudAutostartConfigs() calls qemudLog() if an error is set and qemudLog(), in turn, reports to syslog. Cheers, Mark.
On Wed, Mar 21, 2007 at 01:55:35PM +0000, Mark McLoughlin wrote:
- I think the use case is a little different - generally in libvirt, we're only allocating very small chunks where the CPU hit for initialisation would be negligible and would never show up on a profile. I'd prefer to take the minor hit of zero-initialising most/all memory for programming ease.
- If our wrappers always zero-initialise, we don't need the "initialise to -1 when debugging" thing.
- If we rely on calloc() zero-initialising in our wrappers, we give opportunity for libc to optimise where it knows the memory is already initialised - e.g. where it's mmap()ing the memory from /dev/zero
okay, okay, let's use calloc() for libvirt, but then there is a number of places where I probably used memset() for zeroing, they should all be cleaned up.
Ah, memory usage reports ... nice. Would such a report be less useful with the two mixed together, though? e.g. I personally would just like to see the libvirt memory usage, rather than libxml2, in such a report.
well memory allocated by libxml2 as strings my be kept for libvirt own usage
Again, though, I think libvirt has slightly different needs from a malloc() wrapper.
yes, agreed.
okay, I assume those reporting layers are missing ATM, right ?
Nope, it's all there.
See, in qemudAddIptablesRules() we set VIR_ERR_NO_MEMORY if iptablesContextNew() returns NULL, qemudAutostartConfigs() calls qemudLog() if an error is set and qemudLog(), in turn, reports to syslog.
then all calloc/malloc calls in qemud should get that report code (maybe factored to avoid repeated code ?) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard wrote:
On Wed, Mar 21, 2007 at 01:55:35PM +0000, Mark McLoughlin wrote:
- I think the use case is a little different - generally in libvirt, we're only allocating very small chunks where the CPU hit for initialisation would be negligible and would never show up on a profile. I'd prefer to take the minor hit of zero-initialising most/all memory for programming ease.
- If our wrappers always zero-initialise, we don't need the "initialise to -1 when debugging" thing.
- If we rely on calloc() zero-initialising in our wrappers, we give opportunity for libc to optimise where it knows the memory is already initialised - e.g. where it's mmap()ing the memory from /dev/zero
okay, okay, let's use calloc() for libvirt, but then there is a number of places where I probably used memset() for zeroing, they should all be cleaned up.
<pedant> Note that neither calloc nor memset really work on unusual architectures where null pointers aren't represented by all-bits-zero. So code like: struct { void *ptr; } *s; s = malloc (sizeof (*s)); memset (s, 0, sizeof (*s)); /* ... */ if (s->ptr == NULL) { do something } isn't portable. There's some really strange stuff here about this: http://www.ex-parrot.com/~chris/random/initialise.html </pedant> Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 "[Negative numbers] darken the very whole doctrines of the equations and make dark of the things which are in their nature excessively obvious and simple" (Francis Maseres FRS, mathematician, 1759)
On Thu, Mar 22, 2007 at 12:03:10PM +0000, Richard W.M. Jones wrote:
<pedant>
Note that neither calloc nor memset really work on unusual architectures where null pointers aren't represented by all-bits-zero. So code like:
struct { void *ptr; } *s; s = malloc (sizeof (*s)); memset (s, 0, sizeof (*s)); /* ... */ if (s->ptr == NULL) { do something }
isn't portable.
Understood, but I have yet to get feedback for this portability problems from such an architecture in real life. I'm not sure our compilers would be smart enough to remap to a single zeroing if we were to initialize each field individually, and in that case it's a matter of penalizing all existing architectures in the name of an unexistant one. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Mar 21, 2007 at 10:31:41AM -0400, Daniel Veillard wrote:
On Wed, Mar 21, 2007 at 01:55:35PM +0000, Mark McLoughlin wrote:
- I think the use case is a little different - generally in libvirt, we're only allocating very small chunks where the CPU hit for initialisation would be negligible and would never show up on a profile. I'd prefer to take the minor hit of zero-initialising most/all memory for programming ease.
- If our wrappers always zero-initialise, we don't need the "initialise to -1 when debugging" thing.
- If we rely on calloc() zero-initialising in our wrappers, we give opportunity for libc to optimise where it knows the memory is already initialised - e.g. where it's mmap()ing the memory from /dev/zero
okay, okay, let's use calloc() for libvirt, but then there is a number of places where I probably used memset() for zeroing, they should all be cleaned up.
Patch enclosed, it also fixes a couple of places in virsh.c where malloc() and calloc() were called directly instead of using the virsh checking functions. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Thu, 2007-03-22 at 13:01 -0400, Daniel Veillard wrote:
Index: qemud/iptables.c
Hmm, this stuff is already in one of the patches I sent.
- if (!(argv = (char **)malloc(sizeof(char *) * (n+1)))) + if (!(argv = (char **)calloc(1, sizeof(char *) * (n + 1))))
I'd do: if (!(argv = (char **)calloc(n + 1, sizeof(char *)))) Unless, of course, you're just being ironic about how stupid it is that we have calloc() rather than malloc0() ... in which case I thoroughly approve :-) Same thing elsewhere in the patch e.g.
- if (!(*argv = malloc(len * sizeof(char *)))) + if (!(*argv = calloc(1, len * sizeof(char *))))
should be: if (!(*argv = calloc(len, sizeof(char *)))) Cheers, Mark.
On Thu, Mar 22, 2007 at 05:13:35PM +0000, Mark McLoughlin wrote:
On Thu, 2007-03-22 at 13:01 -0400, Daniel Veillard wrote:
Index: qemud/iptables.c
Hmm, this stuff is already in one of the patches I sent.
- if (!(argv = (char **)malloc(sizeof(char *) * (n+1)))) + if (!(argv = (char **)calloc(1, sizeof(char *) * (n + 1))))
I'd do:
if (!(argv = (char **)calloc(n + 1, sizeof(char *))))
Unless, of course, you're just being ironic about how stupid it is that we have calloc() rather than malloc0() ... in which case I thoroughly approve :-)
no irony ... robotic, just s+malloc(+calloc(1, + in the right contexts
Same thing elsewhere in the patch e.g.
- if (!(*argv = malloc(len * sizeof(char *)))) + if (!(*argv = calloc(1, len * sizeof(char *))))
should be:
if (!(*argv = calloc(len, sizeof(char *))))
okay, will do, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Mark McLoughlin <markmc@redhat.com> wrote: ...
- If our wrappers always zero-initialise, we don't need the "initialise to -1 when debugging" thing.
You probably already know, but if your wrappers always initialize, that can mask used-uninitialized. So it's best if the initialization is only optional, so you can test (i.e. via valgrind) with it turned off.
On Wed, 2007-03-21 at 16:24 +0100, Jim Meyering wrote:
Mark McLoughlin <markmc@redhat.com> wrote: ...
- If our wrappers always zero-initialise, we don't need the "initialise to -1 when debugging" thing.
You probably already know, but if your wrappers always initialize, that can mask used-uninitialized. So it's best if the initialization is only optional, so you can test (i.e. via valgrind) with it turned off.
It doesn't mask used-uninitialised if it always initialises. If it always initialises, then how can it be even be used uninitialised? :-) Seriously, if[1] you had a wrapper e.g. libvirtMalloc0() then the intention of the function is to zero-initialise, so why would you want valgrind to be able to find out where there might have been a bug if you only used malloc() ? Cheers, Mark. [1] - I'm not suggesting we add this. If the only purpose of the wrapper is to zero-initialise, then we should just use calloc() instead of the wrapper
Mark McLoughlin <markmc@redhat.com> wrote:
On Wed, 2007-03-21 at 16:24 +0100, Jim Meyering wrote:
Mark McLoughlin <markmc@redhat.com> wrote: ...
- If our wrappers always zero-initialise, we don't need the "initialise to -1 when debugging" thing.
You probably already know, but if your wrappers always initialize, that can mask used-uninitialized. So it's best if the initialization is only optional, so you can test (i.e. via valgrind) with it turned off.
It doesn't mask used-uninitialised if it always initialises. If it always initialises, then how can it be even be used uninitialised? :-)
Hi Mark, I interpret "wrappers", above, to mean more than just a calloc-like wrapper. A malloc (not calloc, of course) wrapper that always initializes can mask what would have otherwise been a used-uninitialised error, and what would still be a logical U.I. error.
On Wed, Mar 21, 2007 at 04:38:00PM +0100, Jim Meyering wrote:
I interpret "wrappers", above, to mean more than just a calloc-like wrapper.
A malloc (not calloc, of course) wrapper that always initializes can mask what would have otherwise been a used-uninitialised error, and what would still be a logical U.I. error.
That seems silly. If the wrapper is defined as zero-initalising then it cannot be an error to assume that it zero-initalises. dme.
David Edmondson <dme@sun.com> wrote:
On Wed, Mar 21, 2007 at 04:38:00PM +0100, Jim Meyering wrote:
I interpret "wrappers", above, to mean more than just a calloc-like wrapper.
A malloc (not calloc, of course) wrapper that always initializes can mask what would have otherwise been a used-uninitialised error, and what would still be a logical U.I. error.
That seems silly. If the wrapper is defined as zero-initalising then it cannot be an error to assume that it zero-initalises.
What seems silly? A malloc() wrapper that initializes the memory it allocates? That's the case in which errors can be masked. A function intended to be used as a malloc or realloc replacement should not initialize its memory -- at least not by default. A calloc-wrapper _must_ do that. Not the others.
On Thu, 2007-03-22 at 12:22 +0100, Jim Meyering wrote:
David Edmondson <dme@sun.com> wrote:
On Wed, Mar 21, 2007 at 04:38:00PM +0100, Jim Meyering wrote:
I interpret "wrappers", above, to mean more than just a calloc-like wrapper.
A malloc (not calloc, of course) wrapper that always initializes can mask what would have otherwise been a used-uninitialised error, and what would still be a logical U.I. error.
That seems silly. If the wrapper is defined as zero-initalising then it cannot be an error to assume that it zero-initalises.
What seems silly? A malloc() wrapper that initializes the memory it allocates? That's the case in which errors can be masked. A function intended to be used as a malloc or realloc replacement should not initialize its memory -- at least not by default. A calloc-wrapper _must_ do that. Not the others.
Wow, this is a weird little side-track :-) I think the confusion is just around what the term "malloc() wrapper" means. I mean it purely as "a memory allocation function", you seem to interpret it as "a function which overrides the malloc() symbol and has the same semantics as malloc()". e.g. I would define g_malloc(), g_malloc0(), g_new() and g_new0() etc.[1] as malloc() wrappers ... but you wouldn't? They have different semantics from malloc() and some of them have the semantic "zero initialise the newly allocated memory". So, sure, a zero-initialising replacement for the malloc(3) symbol would be stupid. Cheers, Mark. [1] - http://developer.gnome.org/doc/API/2.0/glib/glib-Memory-Allocation.html
On Thu, Mar 22, 2007 at 12:22:13PM +0100, Jim Meyering wrote:
David Edmondson <dme@sun.com> wrote:
On Wed, Mar 21, 2007 at 04:38:00PM +0100, Jim Meyering wrote:
I interpret "wrappers", above, to mean more than just a calloc-like wrapper.
A malloc (not calloc, of course) wrapper that always initializes can mask what would have otherwise been a used-uninitialised error, and what would still be a logical U.I. error.
And did that only in special compilation mode.
That seems silly. If the wrapper is defined as zero-initalising then it cannot be an error to assume that it zero-initalises.
What seems silly? A malloc() wrapper that initializes the memory it allocates? That's the case in which errors can be masked. A function intended to be used as a malloc or realloc replacement should not initialize its memory -- at least not by default. A calloc-wrapper _must_ do that. Not the others.
Before jumping on this and creating a out of scope thread out of a passing remark, please check the source of what I was comparing to and why in context. The purpose has never been to have a memory wrapper initialize the memory all the time. References: http://xmlsoft.org/html/libxml-xmlmemory.html http://xmlsoft.org/xmlmem.html#Debugging If you want to debate this I rather suggest to do this on libxml2 mailing list since this was never the intent on libvirt. http://xmlsoft.org/bugs.html thanks, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Mar 21, 2007 at 09:39:15AM -0400, Daniel Veillard wrote:
Well, one of the things I do in my wrapper is initilize to -1 all newly allocated bytes, allows to pinpoint relatively easilly when you assumed zeroed memory which was not, while not paying for the initialization when not needed.
If you initialise the bytes to -1, don't you always pay the cost of initialisation? dme.
On Wed, Mar 21, 2007 at 01:57:09PM +0000, David Edmondson wrote:
On Wed, Mar 21, 2007 at 09:39:15AM -0400, Daniel Veillard wrote:
Well, one of the things I do in my wrapper is initilize to -1 all newly allocated bytes, allows to pinpoint relatively easilly when you assumed zeroed memory which was not, while not paying for the initialization when not needed.
If you initialise the bytes to -1, don't you always pay the cost of initialisation?
that's just when I use the debugging set of routines, off in the normal case. Somehow valgrind should now allow to spot those kind of errors, but valgrind ain't everywhere. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (5)
-
Daniel Veillard -
David Edmondson -
Jim Meyering -
Mark McLoughlin -
Richard W.M. Jones