[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [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 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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]