diff mbox series

[FFmpeg-devel] avformat/rmdec: Fix memleaks upon read_header failure

Message ID HE1PR0301MB2154AD8B8D7789D78FDE98F08F759@HE1PR0301MB2154.eurprd03.prod.outlook.com
State Accepted
Commit 9a471c5437d34cd1e63520b47f50a0fa605a5688
Headers show
Series [FFmpeg-devel] avformat/rmdec: Fix memleaks upon read_header failure
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt April 7, 2021, 11:06 a.m. UTC
For both the RealMedia as well as the IVR demuxer (which share the same
context) each AVStream's priv_data contains an AVPacket that might
contain data (even when reading the header) and therefore needs to be
unreferenced. Up until now, this has not always been done:

The RealMedia demuxer didn't do it when allocating a new stream's
priv_data failed although there might be other streams with packets to
unreference. (The reason for this was that until recently rm_read_close()
couldn't handle an AVStream without priv_data, so one had to choose
between a potential crash and a memleak.)

The IVR demuxer meanwhile never ever called read_close so that the data
already contained in packets leaks upon error.

This patch fixes both demuxers by adding the appropriate cleanup code.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/rmdec.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

Comments

Andreas Rheinhardt April 7, 2021, 11:57 p.m. UTC | #1
Andreas Rheinhardt:
> For both the RealMedia as well as the IVR demuxer (which share the same
> context) each AVStream's priv_data contains an AVPacket that might
> contain data (even when reading the header) and therefore needs to be
> unreferenced. Up until now, this has not always been done:
> 
> The RealMedia demuxer didn't do it when allocating a new stream's
> priv_data failed although there might be other streams with packets to
> unreference. (The reason for this was that until recently rm_read_close()
> couldn't handle an AVStream without priv_data, so one had to choose
> between a potential crash and a memleak.)
> 
> The IVR demuxer meanwhile never ever called read_close so that the data
> already contained in packets leaks upon error.
> 
> This patch fixes both demuxers by adding the appropriate cleanup code.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavformat/rmdec.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> index b6f42183e8..1dec70e95b 100644
> --- a/libavformat/rmdec.c
> +++ b/libavformat/rmdec.c
> @@ -614,8 +614,10 @@ static int rm_read_header(AVFormatContext *s)
>              get_str8(pb, mime, sizeof(mime)); /* mimetype */
>              st->codecpar->codec_type = AVMEDIA_TYPE_DATA;
>              st->priv_data = ff_rm_alloc_rmstream();
> -            if (!st->priv_data)
> -                return AVERROR(ENOMEM);
> +            if (!st->priv_data) {
> +                ret = AVERROR(ENOMEM);
> +                goto fail;
> +            }
>  
>              size = avio_rb32(pb);
>              codec_pos = avio_tell(pb);
> @@ -1249,20 +1251,19 @@ static int ivr_read_header(AVFormatContext *s)
>      }
>  
>      for (n = 0; n < nb_streams; n++) {
> -        st = avformat_new_stream(s, NULL);
> -        if (!st)
> -            return AVERROR(ENOMEM);
> -        st->priv_data = ff_rm_alloc_rmstream();
> -        if (!st->priv_data)
> -            return AVERROR(ENOMEM);
> +        if (!(st = avformat_new_stream(s, NULL)) ||
> +            !(st->priv_data = ff_rm_alloc_rmstream())) {
> +            ret = AVERROR(ENOMEM);
> +            goto fail;
> +        }
>  
>          if (avio_r8(pb) != 1)
> -            return AVERROR_INVALIDDATA;
> +            goto invalid_data;
>  
>          count = avio_rb32(pb);
>          for (i = 0; i < count; i++) {
>              if (avio_feof(pb))
> -                return AVERROR_INVALIDDATA;
> +                goto invalid_data;
>  
>              type = avio_r8(pb);
>              tlen  = avio_rb32(pb);
> @@ -1274,25 +1275,25 @@ static int ivr_read_header(AVFormatContext *s)
>              } else if (type == 4 && !strncmp(key, "OpaqueData", tlen)) {
>                  ret = ffio_ensure_seekback(pb, 4);
>                  if (ret < 0)
> -                    return ret;
> +                    goto fail;
>                  if (avio_rb32(pb) == MKBETAG('M', 'L', 'T', 'I')) {
>                      ret = rm_read_multi(s, pb, st, NULL);
>                  } else {
>                      if (avio_feof(pb))
> -                        return AVERROR_INVALIDDATA;
> +                        goto invalid_data;
>                      avio_seek(pb, -4, SEEK_CUR);
>                      ret = ff_rm_read_mdpr_codecdata(s, pb, st, st->priv_data, len, NULL);
>                  }
>  
>                  if (ret < 0)
> -                    return ret;
> +                    goto fail;
>              } else if (type == 4) {
>                  int j;
>  
>                  av_log(s, AV_LOG_DEBUG, "%s = '0x", key);
>                  for (j = 0; j < len; j++) {
>                      if (avio_feof(pb))
> -                        return AVERROR_INVALIDDATA;
> +                        goto invalid_data;
>                      av_log(s, AV_LOG_DEBUG, "%X", avio_r8(pb));
>                  }
>                  av_log(s, AV_LOG_DEBUG, "'\n");
> @@ -1309,14 +1310,19 @@ static int ivr_read_header(AVFormatContext *s)
>      }
>  
>      if (avio_r8(pb) != 6)
> -        return AVERROR_INVALIDDATA;
> +        goto invalid_data;
>      avio_skip(pb, 12);
>      avio_skip(pb, avio_rb64(pb) + pos - avio_tell(s->pb));
>      if (avio_r8(pb) != 8)
> -        return AVERROR_INVALIDDATA;
> +        goto invalid_data;
>      avio_skip(pb, 8);
>  
>      return 0;
> +invalid_data:
> +    ret = AVERROR_INVALIDDATA;
> +fail:
> +    rm_read_close(s);
> +    return ret;
>  }
>  
>  static int ivr_read_packet(AVFormatContext *s, AVPacket *pkt)
> 
Will apply this patchset.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
index b6f42183e8..1dec70e95b 100644
--- a/libavformat/rmdec.c
+++ b/libavformat/rmdec.c
@@ -614,8 +614,10 @@  static int rm_read_header(AVFormatContext *s)
             get_str8(pb, mime, sizeof(mime)); /* mimetype */
             st->codecpar->codec_type = AVMEDIA_TYPE_DATA;
             st->priv_data = ff_rm_alloc_rmstream();
-            if (!st->priv_data)
-                return AVERROR(ENOMEM);
+            if (!st->priv_data) {
+                ret = AVERROR(ENOMEM);
+                goto fail;
+            }
 
             size = avio_rb32(pb);
             codec_pos = avio_tell(pb);
@@ -1249,20 +1251,19 @@  static int ivr_read_header(AVFormatContext *s)
     }
 
     for (n = 0; n < nb_streams; n++) {
-        st = avformat_new_stream(s, NULL);
-        if (!st)
-            return AVERROR(ENOMEM);
-        st->priv_data = ff_rm_alloc_rmstream();
-        if (!st->priv_data)
-            return AVERROR(ENOMEM);
+        if (!(st = avformat_new_stream(s, NULL)) ||
+            !(st->priv_data = ff_rm_alloc_rmstream())) {
+            ret = AVERROR(ENOMEM);
+            goto fail;
+        }
 
         if (avio_r8(pb) != 1)
-            return AVERROR_INVALIDDATA;
+            goto invalid_data;
 
         count = avio_rb32(pb);
         for (i = 0; i < count; i++) {
             if (avio_feof(pb))
-                return AVERROR_INVALIDDATA;
+                goto invalid_data;
 
             type = avio_r8(pb);
             tlen  = avio_rb32(pb);
@@ -1274,25 +1275,25 @@  static int ivr_read_header(AVFormatContext *s)
             } else if (type == 4 && !strncmp(key, "OpaqueData", tlen)) {
                 ret = ffio_ensure_seekback(pb, 4);
                 if (ret < 0)
-                    return ret;
+                    goto fail;
                 if (avio_rb32(pb) == MKBETAG('M', 'L', 'T', 'I')) {
                     ret = rm_read_multi(s, pb, st, NULL);
                 } else {
                     if (avio_feof(pb))
-                        return AVERROR_INVALIDDATA;
+                        goto invalid_data;
                     avio_seek(pb, -4, SEEK_CUR);
                     ret = ff_rm_read_mdpr_codecdata(s, pb, st, st->priv_data, len, NULL);
                 }
 
                 if (ret < 0)
-                    return ret;
+                    goto fail;
             } else if (type == 4) {
                 int j;
 
                 av_log(s, AV_LOG_DEBUG, "%s = '0x", key);
                 for (j = 0; j < len; j++) {
                     if (avio_feof(pb))
-                        return AVERROR_INVALIDDATA;
+                        goto invalid_data;
                     av_log(s, AV_LOG_DEBUG, "%X", avio_r8(pb));
                 }
                 av_log(s, AV_LOG_DEBUG, "'\n");
@@ -1309,14 +1310,19 @@  static int ivr_read_header(AVFormatContext *s)
     }
 
     if (avio_r8(pb) != 6)
-        return AVERROR_INVALIDDATA;
+        goto invalid_data;
     avio_skip(pb, 12);
     avio_skip(pb, avio_rb64(pb) + pos - avio_tell(s->pb));
     if (avio_r8(pb) != 8)
-        return AVERROR_INVALIDDATA;
+        goto invalid_data;
     avio_skip(pb, 8);
 
     return 0;
+invalid_data:
+    ret = AVERROR_INVALIDDATA;
+fail:
+    rm_read_close(s);
+    return ret;
 }
 
 static int ivr_read_packet(AVFormatContext *s, AVPacket *pkt)