Re: [PATCH v2] libxc: fix leak of t_info in xc_tbuf_get_size()

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2016-02-11 at 11:03 +0100, Dario Faggioli wrote:
> On Thu, 2016-02-11 at 09:52 +0000, Ian Campbell wrote:
> > On Thu, 2016-02-11 at 14:02 +0530, Harmandeep Kaur wrote:
> > >  
> > > diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
> > > index 695939a..d96cc67 100644
> > > --- a/tools/libxc/xc_tbuf.c
> > > +++ b/tools/libxc/xc_tbuf.c
> > > @@ -70,11 +70,13 @@ int xc_tbuf_get_size(xc_interface *xch,
> > > unsigned long
> > > *size)
> > >                      sysctl.u.tbuf_op.buffer_mfn);
> > >  
> > >      if ( t_info == NULL || t_info->tbuf_size == 0 )
> > > -        return -1;
> > > +        rc = -1;
> > > +    else
> > > +	*size = t_info->tbuf_size;
> > >  
> > > -    *size = t_info->tbuf_size;
> > > +    xenforeignmemory_unmap(xch->fmem, t_info, *size);
> > 
> > *size could be uninitialised here (in the error path) and even in the
> > success case I don't think t_info->tbus_size is the right argument
> > here, it
> > needs to be the size which was passed to the map function, i.e.
> > sysctl.u.tbuf_op.size.
> > 
> And I think both are issues with the current code,

I don't think so, the xenforeignmemory_unmap using *size as an argument
(where it is either uninitialised or the wrong value) is added by this
patch.

>  and, more important,
> not what Coverity is complaining about in the referenced CID?

> To be clear, I'm not arguing that they're not issues we should fix (I
> don't know about the tbuf_size vs. tbuf_op.size, but I can check). I'm
> genuinely asking whether we should do that here, as compared to in a
> pre or follow up patch, and let this one be the one that placates
> Coverity on that particular issue.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel