[FFmpeg-devel] [PATCH] mxfdec: simplify code by using av_calloc()

[Tomas Härdin]

On Wed, 2012-02-01 at 19:40 +0100, Nicolas George wrote:
> Le tridi 13 pluviôse, an CCXX, Michael Niedermayer a écrit :
> > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > ---
> >  libavformat/mxfdec.c |   26 +++++++-------------------
> >  1 files changed, 7 insertions(+), 19 deletions(-)
> > 
> > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > index c927869..ea9bf6d 100644
> > --- a/libavformat/mxfdec.c
> > +++ b/libavformat/mxfdec.c
> > @@ -395,12 +395,10 @@ static int mxf_read_primer_pack(void *arg, AVIOContext *pb, int tag, int size, U
> >                                item_len);
> >          return AVERROR_PATCHWELCOME;
> >      }
> > -    if (item_num > UINT_MAX / item_len)
> > -        return AVERROR_INVALIDDATA;
> 
> calloc checks for INT_MAX, but could someday check for SIZE_MAX: could it be
> a problem somewhere else in the code, where something would assume that
> item_num has been validated against UINT_MAX precisely?

I don't think it's too much of a problem. The only normal case where any
array in the demuxer would approach such a huge size is the index
tables.
However, we could check item_num > 65536 here - there can be no more
than so many local tags per partition (16-bit key).

> Probably not a problem.
> 
> > -    mxf->local_tags_count = item_num;
> > -    mxf->local_tags = av_malloc(item_num*item_len);
> > +    mxf->local_tags = av_calloc(item_num, item_len);
> >      if (!mxf->local_tags)
> >          return AVERROR(ENOMEM);
> > +    mxf->local_tags_count = item_num;
> 
> I do not think you needed to move that last line.

This has the added benefit of local_tags and local_tags_count being
consistent though (NULL <==> 0).

> >      avio_read(pb, mxf->local_tags, item_num*item_len);
> >      return 0;
> >  }
> > @@ -564,9 +562,7 @@ static int mxf_read_content_storage(void *arg, AVIOContext *pb, int tag, int siz
> >      switch (tag) {
> >      case 0x1901:
> >          mxf->packages_count = avio_rb32(pb);
> > -        if (mxf->packages_count >= UINT_MAX / sizeof(UID))
> > -            return AVERROR_INVALIDDATA;
> > -        mxf->packages_refs = av_malloc(mxf->packages_count * sizeof(UID));
> > +        mxf->packages_refs = av_calloc(mxf->packages_count, sizeof(UID));
> >          if (!mxf->packages_refs)
> >              return AVERROR(ENOMEM);
> >          avio_skip(pb, 4); /* useless size of objects, always 16 according to specs */
> > @@ -604,9 +600,7 @@ static int mxf_read_material_package(void *arg, AVIOContext *pb, int tag, int si
> >      switch(tag) {
> >      case 0x4403:
> >          package->tracks_count = avio_rb32(pb);
> > -        if (package->tracks_count >= UINT_MAX / sizeof(UID))
> > -            return AVERROR_INVALIDDATA;
> > -        package->tracks_refs = av_malloc(package->tracks_count * sizeof(UID));
> > +        package->tracks_refs = av_calloc(package->tracks_count, sizeof(UID));
> >          if (!package->tracks_refs)
> >              return AVERROR(ENOMEM);
> >          avio_skip(pb, 4); /* useless size of objects, always 16 according to specs */
> > @@ -649,9 +643,7 @@ static int mxf_read_sequence(void *arg, AVIOContext *pb, int tag, int size, UID
> >          break;
> >      case 0x1001:
> >          sequence->structural_components_count = avio_rb32(pb);
> > -        if (sequence->structural_components_count >= UINT_MAX / sizeof(UID))
> > -            return AVERROR_INVALIDDATA;
> > -        sequence->structural_components_refs = av_malloc(sequence->structural_components_count * sizeof(UID));
> > +        sequence->structural_components_refs = av_calloc(sequence->structural_components_count, sizeof(UID));
> >          if (!sequence->structural_components_refs)
> >              return AVERROR(ENOMEM);
> >          avio_skip(pb, 4); /* useless size of objects, always 16 according to specs */
> > @@ -667,9 +659,7 @@ static int mxf_read_source_package(void *arg, AVIOContext *pb, int tag, int size
> >      switch(tag) {
> >      case 0x4403:
> >          package->tracks_count = avio_rb32(pb);
> > -        if (package->tracks_count >= UINT_MAX / sizeof(UID))
> > -            return AVERROR_INVALIDDATA;
> > -        package->tracks_refs = av_malloc(package->tracks_count * sizeof(UID));
> > +        package->tracks_refs = av_calloc(package->tracks_count, sizeof(UID));
> >          if (!package->tracks_refs)
> >              return AVERROR(ENOMEM);
> >          avio_skip(pb, 4); /* useless size of objects, always 16 according to specs */
> > @@ -771,9 +761,7 @@ static int mxf_read_generic_descriptor(void *arg, AVIOContext *pb, int tag, int
> >      switch(tag) {
> >      case 0x3F01:
> >          descriptor->sub_descriptors_count = avio_rb32(pb);
> > -        if (descriptor->sub_descriptors_count >= UINT_MAX / sizeof(UID))
> > -            return AVERROR_INVALIDDATA;
> > -        descriptor->sub_descriptors_refs = av_malloc(descriptor->sub_descriptors_count * sizeof(UID));
> > +        descriptor->sub_descriptors_refs = av_calloc(descriptor->sub_descriptors_count, sizeof(UID));
> >          if (!descriptor->sub_descriptors_refs)
> >              return AVERROR(ENOMEM);
> >          avio_skip(pb, 4); /* useless size of objects, always 16 according to specs */
> > -- 
> > 1.7.5.4
> 
> This will return ENOMEM instead of INVALIDDATA if the values are bogus: is
> it a problem?

That is a matter of interpretation. The file could indeed be valid, just
require too much memory to make sense of. So either is fine IMO.

Everything looks OK.

/Tomas