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

[Nicolas George]

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?

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.

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

Otherwise, I see no reason not to do these changes.

But I am not mxf maintainer.

Regards,

-- 
  Nicolas George
-------------- 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/20120201/2957a574/attachment.asc>