[Libvir] [patch 1/5] iptables: fix invalid free

[Daniel Veillard]

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 at 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 at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/