diff mbox

[FFmpeg-devel,v1,4/6] avcodec/magicyuv: fix for the memory leak if failed

Message ID 20191011061444.4988-4-lance.lmwang@gmail.com
State New
Headers show

Commit Message

Lance Wang Oct. 11, 2019, 6:14 a.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavcodec/magicyuv.c | 43 ++++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

Comments

Andreas Rheinhardt Oct. 11, 2019, 8 a.m. UTC | #1 
lance.lmwang@gmail.com:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavcodec/magicyuv.c | 43 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/libavcodec/magicyuv.c b/libavcodec/magicyuv.c
> index 0b1ac7345a..c905c2ca22 100644
> --- a/libavcodec/magicyuv.c
> +++ b/libavcodec/magicyuv.c
> @@ -661,12 +661,16 @@ static int magy_decode_frame(AVCodecContext *avctx, void *data,
>  
>      for (i = 0; i < s->planes; i++) {
>          av_fast_malloc(&s->slices[i], &s->slices_size[i], s->nb_slices * sizeof(Slice));
> -        if (!s->slices[i])
> -            return AVERROR(ENOMEM);
> +        if (!s->slices[i]) {
> +            ret = AVERROR(ENOMEM);
> +            goto fail;
> +        }
>  
>          offset = bytestream2_get_le32(&gbyte);
> -        if (offset >= avpkt->size - header_size)
> -            return AVERROR_INVALIDDATA;
> +        if (offset >= avpkt->size - header_size) {
> +            ret = AVERROR_INVALIDDATA;
> +            goto fail;
> +        }
>  
>          if (i == 0)
>              first_offset = offset;
> @@ -675,8 +679,10 @@ static int magy_decode_frame(AVCodecContext *avctx, void *data,
>              s->slices[i][j].start = offset + header_size;
>  
>              next_offset = bytestream2_get_le32(&gbyte);
> -            if (next_offset <= offset || next_offset >= avpkt->size - header_size)
> -                return AVERROR_INVALIDDATA;
> +            if (next_offset <= offset || next_offset >= avpkt->size - header_size) {
> +                ret = AVERROR_INVALIDDATA;
> +                goto fail;
> +            }
>  
>              s->slices[i][j].size = next_offset - offset;
>              offset = next_offset;
> @@ -686,28 +692,32 @@ static int magy_decode_frame(AVCodecContext *avctx, void *data,
>          s->slices[i][j].size  = avpkt->size - s->slices[i][j].start;
>      }
>  
> -    if (bytestream2_get_byte(&gbyte) != s->planes)
> -        return AVERROR_INVALIDDATA;
> +    if (bytestream2_get_byte(&gbyte) != s->planes) {
> +        ret = AVERROR_INVALIDDATA;
> +        goto fail;
> +    }
>  
>      bytestream2_skip(&gbyte, s->nb_slices * s->planes);
>  
>      table_size = header_size + first_offset - bytestream2_tell(&gbyte);
> -    if (table_size < 2)
> -        return AVERROR_INVALIDDATA;
> +    if (table_size < 2) {
> +        ret = AVERROR_INVALIDDATA;
> +        goto fail;
> +    }
>  
>      ret = init_get_bits8(&gbit, avpkt->data + bytestream2_tell(&gbyte), table_size);
>      if (ret < 0)
> -        return ret;
> +        goto fail;
>  
>      ret = build_huffman(avctx, &gbit, s->max);
>      if (ret < 0)
> -        return ret;
> +        goto fail;
>  
>      p->pict_type = AV_PICTURE_TYPE_I;
>      p->key_frame = 1;
>  
>      if ((ret = ff_thread_get_buffer(avctx, &frame, 0)) < 0)
> -        return ret;
> +        goto fail;
>  
>      s->buf = avpkt->data;
>      s->p = p;
> @@ -736,6 +746,13 @@ static int magy_decode_frame(AVCodecContext *avctx, void *data,
>      *got_frame = 1;
>  
>      return avpkt->size;
> +
> +fail:
> +    for (i = 0; i < FF_ARRAY_ELEMS(s->slices); i++) {
> +        av_freep(&s->slices[i]);
> +        s->slices_size[i] = 0;
> +    }
> +    return ret;
>  }
>  
>  #if HAVE_THREADS
> 
Why should there be a memleak? The slices buffer will be freed in
magy_decode_end (which is called from avcodec_close() (usually via
avcodec_free_context()) at the end of the decoding process). So how
did you detect this memleak?

- Andreas
Lance Wang Oct. 11, 2019, 9:20 a.m. UTC | #2 
On Fri, Oct 11, 2019 at 08:00:00AM +0000, Andreas Rheinhardt wrote:
> lance.lmwang@gmail.com:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavcodec/magicyuv.c | 43 ++++++++++++++++++++++++++++++-------------
> >  1 file changed, 30 insertions(+), 13 deletions(-)
> > 
> > diff --git a/libavcodec/magicyuv.c b/libavcodec/magicyuv.c
> > index 0b1ac7345a..c905c2ca22 100644
> > --- a/libavcodec/magicyuv.c
> > +++ b/libavcodec/magicyuv.c
> > @@ -661,12 +661,16 @@ static int magy_decode_frame(AVCodecContext *avctx, void *data,
> >  
> >      for (i = 0; i < s->planes; i++) {
> >          av_fast_malloc(&s->slices[i], &s->slices_size[i], s->nb_slices * sizeof(Slice));
> > -        if (!s->slices[i])
> > -            return AVERROR(ENOMEM);
> > +        if (!s->slices[i]) {
> > +            ret = AVERROR(ENOMEM);
> > +            goto fail;
> > +        }
> >  
> >          offset = bytestream2_get_le32(&gbyte);
> > -        if (offset >= avpkt->size - header_size)
> > -            return AVERROR_INVALIDDATA;
> > +        if (offset >= avpkt->size - header_size) {
> > +            ret = AVERROR_INVALIDDATA;
> > +            goto fail;
> > +        }
> >  
> >          if (i == 0)
> >              first_offset = offset;
> > @@ -675,8 +679,10 @@ static int magy_decode_frame(AVCodecContext *avctx, void *data,
> >              s->slices[i][j].start = offset + header_size;
> >  
> >              next_offset = bytestream2_get_le32(&gbyte);
> > -            if (next_offset <= offset || next_offset >= avpkt->size - header_size)
> > -                return AVERROR_INVALIDDATA;
> > +            if (next_offset <= offset || next_offset >= avpkt->size - header_size) {
> > +                ret = AVERROR_INVALIDDATA;
> > +                goto fail;
> > +            }
> >  
> >              s->slices[i][j].size = next_offset - offset;
> >              offset = next_offset;
> > @@ -686,28 +692,32 @@ static int magy_decode_frame(AVCodecContext *avctx, void *data,
> >          s->slices[i][j].size  = avpkt->size - s->slices[i][j].start;
> >      }
> >  
> > -    if (bytestream2_get_byte(&gbyte) != s->planes)
> > -        return AVERROR_INVALIDDATA;
> > +    if (bytestream2_get_byte(&gbyte) != s->planes) {
> > +        ret = AVERROR_INVALIDDATA;
> > +        goto fail;
> > +    }
> >  
> >      bytestream2_skip(&gbyte, s->nb_slices * s->planes);
> >  
> >      table_size = header_size + first_offset - bytestream2_tell(&gbyte);
> > -    if (table_size < 2)
> > -        return AVERROR_INVALIDDATA;
> > +    if (table_size < 2) {
> > +        ret = AVERROR_INVALIDDATA;
> > +        goto fail;
> > +    }
> >  
> >      ret = init_get_bits8(&gbit, avpkt->data + bytestream2_tell(&gbyte), table_size);
> >      if (ret < 0)
> > -        return ret;
> > +        goto fail;
> >  
> >      ret = build_huffman(avctx, &gbit, s->max);
> >      if (ret < 0)
> > -        return ret;
> > +        goto fail;
> >  
> >      p->pict_type = AV_PICTURE_TYPE_I;
> >      p->key_frame = 1;
> >  
> >      if ((ret = ff_thread_get_buffer(avctx, &frame, 0)) < 0)
> > -        return ret;
> > +        goto fail;
> >  
> >      s->buf = avpkt->data;
> >      s->p = p;
> > @@ -736,6 +746,13 @@ static int magy_decode_frame(AVCodecContext *avctx, void *data,
> >      *got_frame = 1;
> >  
> >      return avpkt->size;
> > +
> > +fail:
> > +    for (i = 0; i < FF_ARRAY_ELEMS(s->slices); i++) {
> > +        av_freep(&s->slices[i]);
> > +        s->slices_size[i] = 0;
> > +    }
> > +    return ret;
> >  }
> >  
> >  #if HAVE_THREADS
> > 
> Why should there be a memleak? The slices buffer will be freed in
> magy_decode_end (which is called from avcodec_close() (usually via
> avcodec_free_context()) at the end of the decoding process). So how
> did you detect this memleak?
> 
Have double check it's freed in magy_decode_end, it's my
misunderstanding. Please ignore it.

> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox

Patch

diff --git a/libavcodec/magicyuv.c b/libavcodec/magicyuv.c
index 0b1ac7345a..c905c2ca22 100644
--- a/libavcodec/magicyuv.c
+++ b/libavcodec/magicyuv.c
@@ -661,12 +661,16 @@  static int magy_decode_frame(AVCodecContext *avctx, void *data,
 
     for (i = 0; i < s->planes; i++) {
         av_fast_malloc(&s->slices[i], &s->slices_size[i], s->nb_slices * sizeof(Slice));
-        if (!s->slices[i])
-            return AVERROR(ENOMEM);
+        if (!s->slices[i]) {
+            ret = AVERROR(ENOMEM);
+            goto fail;
+        }
 
         offset = bytestream2_get_le32(&gbyte);
-        if (offset >= avpkt->size - header_size)
-            return AVERROR_INVALIDDATA;
+        if (offset >= avpkt->size - header_size) {
+            ret = AVERROR_INVALIDDATA;
+            goto fail;
+        }
 
         if (i == 0)
             first_offset = offset;
@@ -675,8 +679,10 @@  static int magy_decode_frame(AVCodecContext *avctx, void *data,
             s->slices[i][j].start = offset + header_size;
 
             next_offset = bytestream2_get_le32(&gbyte);
-            if (next_offset <= offset || next_offset >= avpkt->size - header_size)
-                return AVERROR_INVALIDDATA;
+            if (next_offset <= offset || next_offset >= avpkt->size - header_size) {
+                ret = AVERROR_INVALIDDATA;
+                goto fail;
+            }
 
             s->slices[i][j].size = next_offset - offset;
             offset = next_offset;
@@ -686,28 +692,32 @@  static int magy_decode_frame(AVCodecContext *avctx, void *data,
         s->slices[i][j].size  = avpkt->size - s->slices[i][j].start;
     }
 
-    if (bytestream2_get_byte(&gbyte) != s->planes)
-        return AVERROR_INVALIDDATA;
+    if (bytestream2_get_byte(&gbyte) != s->planes) {
+        ret = AVERROR_INVALIDDATA;
+        goto fail;
+    }
 
     bytestream2_skip(&gbyte, s->nb_slices * s->planes);
 
     table_size = header_size + first_offset - bytestream2_tell(&gbyte);
-    if (table_size < 2)
-        return AVERROR_INVALIDDATA;
+    if (table_size < 2) {
+        ret = AVERROR_INVALIDDATA;
+        goto fail;
+    }
 
     ret = init_get_bits8(&gbit, avpkt->data + bytestream2_tell(&gbyte), table_size);
     if (ret < 0)
-        return ret;
+        goto fail;
 
     ret = build_huffman(avctx, &gbit, s->max);
     if (ret < 0)
-        return ret;
+        goto fail;
 
     p->pict_type = AV_PICTURE_TYPE_I;
     p->key_frame = 1;
 
     if ((ret = ff_thread_get_buffer(avctx, &frame, 0)) < 0)
-        return ret;
+        goto fail;
 
     s->buf = avpkt->data;
     s->p = p;
@@ -736,6 +746,13 @@  static int magy_decode_frame(AVCodecContext *avctx, void *data,
     *got_frame = 1;
 
     return avpkt->size;
+
+fail:
+    for (i = 0; i < FF_ARRAY_ELEMS(s->slices); i++) {
+        av_freep(&s->slices[i]);
+        s->slices_size[i] = 0;
+    }
+    return ret;
 }
 
 #if HAVE_THREADS