8:03 a.m.
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(a)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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
8:30 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
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(a)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.
8:39 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
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(a)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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
8:55 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
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(a)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.
8:57 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
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.
9:19 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
9:31 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
10:24 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
Mark McLoughlin <markmc(a)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.
10:32 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
On Wed, 2007-03-21 at 16:24 +0100, Jim Meyering wrote:
Mark McLoughlin <markmc(a)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
10:38 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
Mark McLoughlin <markmc(a)redhat.com> wrote:
On Wed, 2007-03-21 at 16:24 +0100, Jim Meyering wrote:
> Mark McLoughlin <markmc(a)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.
6:16 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
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.
6:22 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
David Edmondson <dme(a)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.
6:40 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
On Thu, 2007-03-22 at 12:22 +0100, Jim Meyering wrote:
David Edmondson <dme(a)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
7:03 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
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)
7:32 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
7:39 a.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
On Thu, Mar 22, 2007 at 12:22:13PM +0100, Jim Meyering wrote:
David Edmondson <dme(a)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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
12:01 p.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
12:13 p.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
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.
1:20 p.m.
New subject: [Libvir] [patch 1/5] iptables: fix invalid free
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
6415
days inactive
6416
days old
18 comments
5 participants
participants (5)
-
Daniel Veillard -
David Edmondson -
Jim Meyering -
Mark McLoughlin -
Richard W.M. Jones