[FFmpeg-devel] [PATCH v3] avformat/mxfdec: Remove this_partition

[Tomas Härdin]

fre 2023-09-22 klockan 21:13 +0200 skrev Michael Niedermayer:
> Suggested-by: Tomas Härdin <git at haerdin.se>
> Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-
> 5130394286817280
> 
> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> ---
>  libavformat/mxfdec.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 4846c5d206a..1313f14fa03 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -102,7 +102,6 @@ typedef struct MXFPartition {
>      uint64_t previous_partition;
>      int index_sid;
>      int body_sid;
> -    int64_t this_partition;
>      int64_t essence_offset;         ///< absolute offset of essence
>      int64_t essence_length;
>      int32_t kag_size;
> @@ -727,10 +726,13 @@ static int mxf_read_partition_pack(void *arg,
> AVIOContext *pb, int tag, int size
>      UID op;
>      uint64_t footer_partition;
>      uint32_t nb_essence_containers;
> +    uint64_t this_partition;
>  
>      if (mxf->partitions_count >= INT_MAX / 2)
>          return AVERROR_INVALIDDATA;
>  
> +    av_assert0(klv_offset >= mxf->run_in);
> +
>      tmp_part = av_realloc_array(mxf->partitions, mxf-
> >partitions_count + 1, sizeof(*mxf->partitions));
>      if (!tmp_part)
>          return AVERROR(ENOMEM);
> @@ -773,7 +775,13 @@ static int mxf_read_partition_pack(void *arg,
> AVIOContext *pb, int tag, int size
>      partition->complete = uid[14] > 2;
>      avio_skip(pb, 4);
>      partition->kag_size = avio_rb32(pb);
> -    partition->this_partition = avio_rb64(pb);
> +    this_partition = avio_rb64(pb);
> +    if (this_partition != klv_offset - mxf->run_in) {
> +        av_log(mxf->fc, AV_LOG_WARNING,
> +               "this_partition %"PRId64" mismatches %"PRId64"\n",
> +               this_partition, klv_offset - mxf->run_in);

We might want to error out here, since this means offsets are likely to
be incorrect across the entire file. We have no files in FATE that
demonstrate this problem, and it pays to be strict when it comes to
MXF. This helps people writing new MXF muxers from writing broken ones.

> +    }
> +    this_partition = klv_offset - mxf->run_in;
>      partition->previous_partition = avio_rb64(pb);
>      footer_partition = avio_rb64(pb);
>      partition->header_byte_count = avio_rb64(pb);
> @@ -793,8 +801,8 @@ static int mxf_read_partition_pack(void *arg,
> AVIOContext *pb, int tag, int size
>          av_dict_set(&s->metadata, "operational_pattern_ul", str, 0);
>      }
>  
> -    if (partition->this_partition &&
> -        partition->previous_partition == partition->this_partition)
> {
> +    if (this_partition &&
> +        partition->previous_partition == this_partition) {

And just to add to the discussion, we know that there are files that
have this specific problem. Or more specifically, muxers that do.

I will also note that partition->previous_partition > this_partition is
checked further down.

/Tomas