[FFmpeg-devel] [PATCH 03/11] avformat/mxfdec: Check type before setting sub_descriptors_refs

[Andreas Rheinhardt]

Michael Niedermayer:
> Fixes: memleak
> Fixes: 26352/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5201158714687488
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> ---
>  libavformat/mxfdec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index d16a7af0df..0313cf4a28 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1172,6 +1172,8 @@ static int mxf_read_generic_descriptor(void *arg, AVIOContext *pb, int tag, int
>  
>      switch(tag) {
>      case 0x3F01:
> +        if (descriptor->type != MultipleDescriptor)
> +            return AVERROR_INVALIDDATA;
>          return mxf_read_strong_ref_array(pb, &descriptor->sub_descriptors_refs,
>                                               &descriptor->sub_descriptors_count);
>      case 0x3002: /* ContainerDuration */
> 
I think this memleak has a twin: I don't see anything that makes sure
that a descriptor of type MultipleDescriptor can't take the default case
in that switch, leading to allocations that are never freed. All it
needs is the local_tags list. Presuming this to be true, one would
either add a corresponding check to the default case of the switch or
one would have to modify mxf_free_metadataset() or one would have to use
a dedicated function to read the MultipleDescriptors. The answer of
course depends upon the specification (it seems that the only thing used
from MultipleDescriptors are indeed the sub descriptors, so if the other
case labels are spec-incompliant anyway, then this would be my favourite
solution).

- Andreas