diff mbox series

[FFmpeg-devel] avformat/matroskadec: Sanity check codec_id/track type

Message ID 20201207000614.20827-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel] avformat/matroskadec: Sanity check codec_id/track type
Related show

Checks

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

Commit Message

Michael Niedermayer Dec. 7, 2020, 12:06 a.m. UTC
Fixes: memleak
Fixes: 27766/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-5198300814508032

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/matroskadec.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Andreas Rheinhardt Dec. 7, 2020, 1:20 a.m. UTC | #1
Michael Niedermayer:
> Fixes: memleak
> Fixes: 27766/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-5198300814508032
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/matroskadec.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 44db2c8358..18fc2750a1 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -2318,6 +2318,18 @@ static int matroska_parse_tracks(AVFormatContext *s)
>          if (!track->codec_id)
>              continue;
>  
> +        if (track->type == MATROSKA_TRACK_TYPE_AUDIO && track->codec_id[0] == 'A') {
> +            ;
> +        } else if (track->type == MATROSKA_TRACK_TYPE_VIDEO && track->codec_id[0] == 'V') {
> +            ;
> +        } else if ((track->type == MATROSKA_TRACK_TYPE_SUBTITLE || track->type == MATROSKA_TRACK_TYPE_METADATA) &&
> +                   (track->codec_id[0] == 'S' || track->codec_id[0] == 'D')) {
> +            ;
> +        } else {
> +            av_log(matroska->ctx, AV_LOG_INFO, "Inconsistent track type\n");
> +            continue;
> +        }
> +
>          if (track->audio.samplerate < 0 || track->audio.samplerate > INT_MAX ||
>              isnan(track->audio.samplerate)) {
>              av_log(matroska->ctx, AV_LOG_WARNING,
> 
Let me guess: The audio buffer used for Real audio codecs leaks because
matroska_read_close() only frees it for audio tracks, whereas it is
possible that a codec gets one of these audio track codec ids without
being of MATROSKA_TRACK_TYPE_AUDIO (hence also not being of
AVMEDIA_TYPE_AUDIO, which is a bug in itself). So I agree with the aim
of these checks, but I think writing

if ((track->type == MATROSKA_TRACK_TYPE_AUDIO && track->codec_id[0] !=
'A') ||
    (track->type == MATROSKA_TRACK_TYPE_VIDEO && track->codec_id[0] !=
'V') ||
...

makes the intent clearer.

- Andreas
Michael Niedermayer Dec. 9, 2020, 8:35 p.m. UTC | #2
On Mon, Dec 07, 2020 at 02:20:50AM +0100, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Fixes: memleak
> > Fixes: 27766/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-5198300814508032
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/matroskadec.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > index 44db2c8358..18fc2750a1 100644
> > --- a/libavformat/matroskadec.c
> > +++ b/libavformat/matroskadec.c
> > @@ -2318,6 +2318,18 @@ static int matroska_parse_tracks(AVFormatContext *s)
> >          if (!track->codec_id)
> >              continue;
> >  
> > +        if (track->type == MATROSKA_TRACK_TYPE_AUDIO && track->codec_id[0] == 'A') {
> > +            ;
> > +        } else if (track->type == MATROSKA_TRACK_TYPE_VIDEO && track->codec_id[0] == 'V') {
> > +            ;
> > +        } else if ((track->type == MATROSKA_TRACK_TYPE_SUBTITLE || track->type == MATROSKA_TRACK_TYPE_METADATA) &&
> > +                   (track->codec_id[0] == 'S' || track->codec_id[0] == 'D')) {
> > +            ;
> > +        } else {
> > +            av_log(matroska->ctx, AV_LOG_INFO, "Inconsistent track type\n");
> > +            continue;
> > +        }
> > +
> >          if (track->audio.samplerate < 0 || track->audio.samplerate > INT_MAX ||
> >              isnan(track->audio.samplerate)) {
> >              av_log(matroska->ctx, AV_LOG_WARNING,
> > 
> Let me guess: The audio buffer used for Real audio codecs leaks because
> matroska_read_close() only frees it for audio tracks, whereas it is
> possible that a codec gets one of these audio track codec ids without
> being of MATROSKA_TRACK_TYPE_AUDIO (hence also not being of
> AVMEDIA_TYPE_AUDIO, which is a bug in itself). So I agree with the aim
> of these checks, but I think writing
> 
> if ((track->type == MATROSKA_TRACK_TYPE_AUDIO && track->codec_id[0] !=
> 'A') ||
>     (track->type == MATROSKA_TRACK_TYPE_VIDEO && track->codec_id[0] !=
> 'V') ||
> ...
> 
> makes the intent clearer.

will apply with this change

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 44db2c8358..18fc2750a1 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -2318,6 +2318,18 @@  static int matroska_parse_tracks(AVFormatContext *s)
         if (!track->codec_id)
             continue;
 
+        if (track->type == MATROSKA_TRACK_TYPE_AUDIO && track->codec_id[0] == 'A') {
+            ;
+        } else if (track->type == MATROSKA_TRACK_TYPE_VIDEO && track->codec_id[0] == 'V') {
+            ;
+        } else if ((track->type == MATROSKA_TRACK_TYPE_SUBTITLE || track->type == MATROSKA_TRACK_TYPE_METADATA) &&
+                   (track->codec_id[0] == 'S' || track->codec_id[0] == 'D')) {
+            ;
+        } else {
+            av_log(matroska->ctx, AV_LOG_INFO, "Inconsistent track type\n");
+            continue;
+        }
+
         if (track->audio.samplerate < 0 || track->audio.samplerate > INT_MAX ||
             isnan(track->audio.samplerate)) {
             av_log(matroska->ctx, AV_LOG_WARNING,