[vlc-devel] [vlc-commits] picture: fix empty allocation leak

[Thomas Guillem]

On Thu, Mar 1, 2018, at 17:05, Rémi Denis-Courmont wrote:
> Le torstaina 1. maaliskuuta 2018, 15.48.45 EET Thomas Guillem a écrit :
> > On Mon, Feb 26, 2018, at 23:25, Rémi Denis-Courmont wrote:
> > > vlc | branch: master | Rémi Denis-Courmont <remi at remlab.net> | Mon Feb
> > > 26 23:32:45 2018 +0200| [054bcfe4a97449d57d4f701ef642fdd01b3bcca9] |
> > > committer: Rémi Denis-Courmont
> > > 
> > > picture: fix empty allocation leak
> > 
> > Hello,
> > 
> > This commit break opaque decoder pool allocation when the display is
> > filtered.
> 
> No, it does not.

VDPAU is broken, and some other configuration on windows when the display is filtered (D3D9 on D3D11 and vice/versa).

Reverting just this patch fixes the issue.

> 
> > This is the case of VDPAU for example.
> 
> VDPAU is how I found and fixed the bug in the first place.
> 
> > > If the picture has zero planes, pic->p->p_pixels is set to NULL rather
> > > than the allocated (zero bytes) buffer, leading to a potential leak
> > > depending on aligned_alloc() implementation.
> > > 
> > > > http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=054bcfe4a97449d57
> > > > d4f701ef642fdd01b3bcca9> 
> > > ---
> > > 
> > >  src/misc/picture.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/misc/picture.c b/src/misc/picture.c
> > > index 8af4b250f9..69e7c92ad6 100644
> > > --- a/src/misc/picture.c
> > > +++ b/src/misc/picture.c
> > > @@ -250,7 +250,11 @@ picture_t *picture_NewFromFormat(const
> > > video_format_t *restrict fmt)
> > > 
> > >      if (unlikely(priv == NULL))
> > >      
> > >          return NULL;
> > > 
> > > +    priv->gc.destroy = picture_Destroy;
> > > +
> > > 
> > >      picture_t *pic = &priv->picture;
> > > 
> > > +    if (pic->i_planes == 0)
> > > +        return NULL;
> > 
> > I would remove this check
> > 
> > >      /* Calculate how big the new image should be */
> > >      size_t plane_sizes[PICTURE_PLANE_MAX];
> > > 
> > > @@ -269,7 +273,7 @@ picture_t *picture_NewFromFormat(const
> > > video_format_t *restrict fmt)
> > > 
> > >          goto error;
> > >      
> > >      uint8_t *buf = aligned_alloc(16, pic_size);
> > 
> > And do the aligned_alloc only if pic_size > 0.
> 
> And that will change exactly nothing. If you perform constant propagation with 
> zero planes, the loops go away and the code can be reduced to:
> 
>     /* if pic->i_planes == 0 */
> 
>     size_t plane_sizes[PICTURE_PLANE_MAX];
>     
>     uint8_t *buf = aligned_alloc(16, 0);
>     if (unlikely(buf == NULL))
>         goto error;
> 
>     return pic;
> 
> So if you skip the allocation and error check, it becomes equivalent to:
> 
>     return pic;
> 
> ...which is exactly what is already there.

Ah ? For me it returns NULL, not pic.

    if (pic->i_planes == 0)
       return NULL;

> 

> -- 
> 雷米‧德尼-库尔蒙
> https://www.remlab.net/
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel