diff mbox series

[FFmpeg-devel,08/14] decode: plug leaks on error in update_frame_pool()

Message ID 20200327125747.13460-8-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,01/14] mpeg4videodec: do not copy a range of fields at once
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Anton Khirnov March 27, 2020, 12:57 p.m. UTC
---
 libavcodec/decode.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

James Almer March 27, 2020, 3:03 p.m. UTC | #1
On 3/27/2020 9:57 AM, Anton Khirnov wrote:
> ---
>  libavcodec/decode.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index 8925ce6edc..f43dc0dd5d 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -1595,7 +1595,7 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame)
>              // that linesize[0] == 2*linesize[1] in the MPEG-encoder for 4:2:2
>              ret = av_image_fill_linesizes(linesize, avctx->pix_fmt, w);
>              if (ret < 0)
> -                return ret;
> +                goto fail;
>              // increase alignment of w for next try (rhs gives the lowest bit set in w)
>              w += w & ~(w - 1);
>  
> @@ -1606,8 +1606,10 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame)
>  
>          tmpsize = av_image_fill_pointers(data, avctx->pix_fmt, h,
>                                           NULL, linesize);
> -        if (tmpsize < 0)
> -            return tmpsize;
> +        if (tmpsize < 0) {
> +            ret = tmpsize;
> +            goto fail;
> +        }
>  
>          for (i = 0; i < 3 && data[i + 1]; i++)
>              size[i] = data[i + 1] - data[i];

Nit: Not really a leak since the pool was not changed in any way if
either of these two checks fail, so new calls to get_buffer2() or
closing the context would free everything just fine.

LGTM with or without changing the commit message to say something about
cleaning on error rather than fixing a leak.
Anton Khirnov April 7, 2020, 1:43 p.m. UTC | #2
Quoting James Almer (2020-03-27 16:03:46)
> On 3/27/2020 9:57 AM, Anton Khirnov wrote:
> > ---
> >  libavcodec/decode.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index 8925ce6edc..f43dc0dd5d 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -1595,7 +1595,7 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame)
> >              // that linesize[0] == 2*linesize[1] in the MPEG-encoder for 4:2:2
> >              ret = av_image_fill_linesizes(linesize, avctx->pix_fmt, w);
> >              if (ret < 0)
> > -                return ret;
> > +                goto fail;
> >              // increase alignment of w for next try (rhs gives the lowest bit set in w)
> >              w += w & ~(w - 1);
> >  
> > @@ -1606,8 +1606,10 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame)
> >  
> >          tmpsize = av_image_fill_pointers(data, avctx->pix_fmt, h,
> >                                           NULL, linesize);
> > -        if (tmpsize < 0)
> > -            return tmpsize;
> > +        if (tmpsize < 0) {
> > +            ret = tmpsize;
> > +            goto fail;
> > +        }
> >  
> >          for (i = 0; i < 3 && data[i + 1]; i++)
> >              size[i] = data[i + 1] - data[i];
> 
> Nit: Not really a leak since the pool was not changed in any way if
> either of these two checks fail, so new calls to get_buffer2() or
> closing the context would free everything just fine.
> 
> LGTM with or without changing the commit message to say something about
> cleaning on error rather than fixing a leak.

Ah right, it actually becomes a leak with the previous patch. So maybe I
should just merge the two
diff mbox series

Patch

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 8925ce6edc..f43dc0dd5d 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -1595,7 +1595,7 @@  static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame)
             // that linesize[0] == 2*linesize[1] in the MPEG-encoder for 4:2:2
             ret = av_image_fill_linesizes(linesize, avctx->pix_fmt, w);
             if (ret < 0)
-                return ret;
+                goto fail;
             // increase alignment of w for next try (rhs gives the lowest bit set in w)
             w += w & ~(w - 1);
 
@@ -1606,8 +1606,10 @@  static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame)
 
         tmpsize = av_image_fill_pointers(data, avctx->pix_fmt, h,
                                          NULL, linesize);
-        if (tmpsize < 0)
-            return tmpsize;
+        if (tmpsize < 0) {
+            ret = tmpsize;
+            goto fail;
+        }
 
         for (i = 0; i < 3 && data[i + 1]; i++)
             size[i] = data[i + 1] - data[i];