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

[Tomas Härdin]

ons 2020-10-21 klockan 16:48 +0200 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > tis 2020-10-20 klockan 22:56 +0200 skrev 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 */
> > 
> > mxf_read_strong_ref_array() is called in more places than this. A
> > better solution would be to make freeing sub_descriptors_refs in
> > mxf_free_metadataset() not depends on type
> > 
> But this is the only place that sets the sub_descriptors_ref.
> Do the other tags that lead to certain properties being read (like
> frame_layouts) actually make sense for a MultipleDescriptor? If not, why
> do Descriptor and MultipleDescriptor share the same parsing function?

MultipleDescriptor (Annex D.5) can contain any FileDescriptor field
(Annex D.1), for example Codec (0x3005). At the moment none of those
fields are used, instead source_package->descriptor gets resolved by
mxf_resolve_multidescriptor() and the resolved descriptor's fields are
used instead.

The 0x3F01 case could be broken out into a separate function that calls
mxf_read_generic_descriptor() in the default case.

/Tomas