[Spice-devel] [usbredir v1] usbredirhost: fix leak on error

[Pavel Grunt]

On Thu, 2016-08-04 at 13:38 +0200, Victor Toso wrote:
> Hi,
> 
> On Thu, Aug 04, 2016 at 08:37:02AM +0200, Pavel Grunt wrote:
> > 
> > Hey
> > 
> > On Wed, 2016-08-03 at 18:11 +0200, Victor Toso wrote:
> > > 
> > > Pointed by coverity:
> > >  17. usbredir-0.7.1/usbredirhost/usbredirhost.c:2306: leaked_storage:
> > >  Returning without freeing "data" leaks the storage that it points to.
> > >  #  2304|
> > >  #  usbredirhost_bulk_packet_complete(transfer->transfer);
> > >  #  2305|       }
> > >  #  2306|-> }
> > >  #  2307|
> > >  #  2308|   static void usbredirhost_iso_packet(void *priv, uint64_t
> > >  #  id,
> > > ---
> > >  usbredirhost/usbredirhost.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/usbredirhost/usbredirhost.c b/usbredirhost/usbredirhost.c
> > > index 3e80677..2b3ee74 100644
> > > --- a/usbredirhost/usbredirhost.c
> > > +++ b/usbredirhost/usbredirhost.c
> > > @@ -2280,6 +2280,7 @@ static void usbredirhost_bulk_packet(void *priv,
> > > uint64_t id,
> > >                                           transfer, BULK_TIMEOUT);
> > >  #else
> > >          r = LIBUSB_ERROR_INVALID_PARAM;
> > > +        free(buffer);
> > buffer is not declared in that function
> 
> But the malloc is ours [0] and we free it in situation of error when
> usbredirhost_alloc_transfer() fails.

yeah, but the variable name is 'data' not 'buffer'
> 
> [0] https://cgit.freedesktop.org/spice/usbredir/tree/usbredirhost/usbredirhost
> .c#n2256
> 
> Not sure about the code design but it seems we should free the data if
> we don't pass this buffer to libusb_fill_bulk_transfer()
> 
> Cheers,
>   toso
> 
> > 
> > 
> > Pavel
> > > 
> > >          goto error;
> > >  #endif
> > >      } else {
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel