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

[Rémi Denis-Courmont]

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.

> 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.

-- 
雷米‧德尼-库尔蒙
https://www.remlab.net/