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

[Michael Niedermayer]

On Wed, Feb 01, 2012 at 09:08:26PM +0100, Tomas Härdin wrote:
> 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).

ok, changed



> 
> > 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.

ok, applied

sorry for the delay

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct awnser.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120719/01cd904d/attachment.asc>