diff mbox series

[FFmpeg-devel,1/4] avformat/asfdec_o: Don't segfault with lots of attached pics

Message ID 20201112173328.742491-1-andreas.rheinhardt@gmail.com
State Accepted
Commit e83f27a21a6d2f602b55e541ef66e365400e9827
Headers show
Series [FFmpeg-devel,1/4] avformat/asfdec_o: Don't segfault with lots of attached pics
Related show

Checks

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

Commit Message

Andreas Rheinhardt Nov. 12, 2020, 5:33 p.m. UTC
The ASF file format has a limit of 127 streams and the "asf_o" demuxer
(the ASF demuxer from Libav) has an array of pointers for a structure
called ASFStream that is allocated on demand for every stream. Attached
pictures are not streams in the sense of the ASF specification, yet the
demuxer created an ASFStream for them; and in one codepath it also
forgot to check whether the array of ASFStreams is already full. The
result is a write beyond the end of the array and a segfault lateron.

Fixing this is easy: Don't create ASFStreams for attached picture
streams.

(Other results of the current state of affairs are unnecessary allocations
(of ASFStreams structures), the misparsing of valid files (there might not
be enough ASFStreams left for the valid streams if attached pictures take
up too many); furthermore, the ASFStreams created for attached pictures all
have the stream number 0, an invalid stream number (the valid range is
1-127). This means that invalid data (packets for a stream with stream
number 0) won't get rejected lateron.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/asfdec_o.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

Comments

Andreas Rheinhardt Nov. 15, 2020, 5:51 a.m. UTC | #1
Andreas Rheinhardt:
> The ASF file format has a limit of 127 streams and the "asf_o" demuxer
> (the ASF demuxer from Libav) has an array of pointers for a structure
> called ASFStream that is allocated on demand for every stream. Attached
> pictures are not streams in the sense of the ASF specification, yet the
> demuxer created an ASFStream for them; and in one codepath it also
> forgot to check whether the array of ASFStreams is already full. The
> result is a write beyond the end of the array and a segfault lateron.
> 
> Fixing this is easy: Don't create ASFStreams for attached picture
> streams.
> 
> (Other results of the current state of affairs are unnecessary allocations
> (of ASFStreams structures), the misparsing of valid files (there might not
> be enough ASFStreams left for the valid streams if attached pictures take
> up too many); furthermore, the ASFStreams created for attached pictures all
> have the stream number 0, an invalid stream number (the valid range is
> 1-127). This means that invalid data (packets for a stream with stream
> number 0) won't get rejected lateron.)
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavformat/asfdec_o.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/libavformat/asfdec_o.c b/libavformat/asfdec_o.c
> index b142f83541..0a7e47d8cc 100644
> --- a/libavformat/asfdec_o.c
> +++ b/libavformat/asfdec_o.c
> @@ -357,7 +357,6 @@ static int asf_set_metadata(AVFormatContext *s, const uint8_t *name,
>   * but in reality this is only loosely similar */
>  static int asf_read_picture(AVFormatContext *s, int len)
>  {
> -    ASFContext *asf       = s->priv_data;
>      AVPacket pkt          = { 0 };
>      const CodecMime *mime = ff_id3v2_mime_tags;
>      enum  AVCodecID id    = AV_CODEC_ID_NONE;
> @@ -365,7 +364,6 @@ static int asf_read_picture(AVFormatContext *s, int len)
>      uint8_t  *desc = NULL;
>      AVStream   *st = NULL;
>      int ret, type, picsize, desc_len;
> -    ASFStream *asf_st;
>  
>      /* type + picsize + mime + desc */
>      if (len < 1 + 4 + 2 + 2) {
> @@ -422,22 +420,14 @@ static int asf_read_picture(AVFormatContext *s, int len)
>          ret = AVERROR(ENOMEM);
>          goto fail;
>      }
> -    asf->asf_st[asf->nb_streams] = av_mallocz(sizeof(*asf_st));
> -    asf_st = asf->asf_st[asf->nb_streams];
> -    if (!asf_st) {
> -        ret = AVERROR(ENOMEM);
> -        goto fail;
> -    }
>  
>      st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
> -    st->codecpar->codec_type      = asf_st->type = AVMEDIA_TYPE_VIDEO;
> +    st->codecpar->codec_type      = AVMEDIA_TYPE_VIDEO;
>      st->codecpar->codec_id        = id;
>      st->attached_pic              = pkt;
> -    st->attached_pic.stream_index = asf_st->index = st->index;
> +    st->attached_pic.stream_index = st->index;
>      st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
>  
> -    asf->nb_streams++;
> -
>      if (*desc) {
>          if (av_dict_set(&st->metadata, "title", desc, AV_DICT_DONT_STRDUP_VAL) < 0)
>              av_log(s, AV_LOG_WARNING, "av_dict_set failed.\n");
> 
Will apply this patchset later today unless there are objections.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/asfdec_o.c b/libavformat/asfdec_o.c
index b142f83541..0a7e47d8cc 100644
--- a/libavformat/asfdec_o.c
+++ b/libavformat/asfdec_o.c
@@ -357,7 +357,6 @@  static int asf_set_metadata(AVFormatContext *s, const uint8_t *name,
  * but in reality this is only loosely similar */
 static int asf_read_picture(AVFormatContext *s, int len)
 {
-    ASFContext *asf       = s->priv_data;
     AVPacket pkt          = { 0 };
     const CodecMime *mime = ff_id3v2_mime_tags;
     enum  AVCodecID id    = AV_CODEC_ID_NONE;
@@ -365,7 +364,6 @@  static int asf_read_picture(AVFormatContext *s, int len)
     uint8_t  *desc = NULL;
     AVStream   *st = NULL;
     int ret, type, picsize, desc_len;
-    ASFStream *asf_st;
 
     /* type + picsize + mime + desc */
     if (len < 1 + 4 + 2 + 2) {
@@ -422,22 +420,14 @@  static int asf_read_picture(AVFormatContext *s, int len)
         ret = AVERROR(ENOMEM);
         goto fail;
     }
-    asf->asf_st[asf->nb_streams] = av_mallocz(sizeof(*asf_st));
-    asf_st = asf->asf_st[asf->nb_streams];
-    if (!asf_st) {
-        ret = AVERROR(ENOMEM);
-        goto fail;
-    }
 
     st->disposition              |= AV_DISPOSITION_ATTACHED_PIC;
-    st->codecpar->codec_type      = asf_st->type = AVMEDIA_TYPE_VIDEO;
+    st->codecpar->codec_type      = AVMEDIA_TYPE_VIDEO;
     st->codecpar->codec_id        = id;
     st->attached_pic              = pkt;
-    st->attached_pic.stream_index = asf_st->index = st->index;
+    st->attached_pic.stream_index = st->index;
     st->attached_pic.flags       |= AV_PKT_FLAG_KEY;
 
-    asf->nb_streams++;
-
     if (*desc) {
         if (av_dict_set(&st->metadata, "title", desc, AV_DICT_DONT_STRDUP_VAL) < 0)
             av_log(s, AV_LOG_WARNING, "av_dict_set failed.\n");