diff mbox series

[FFmpeg-devel,v2] avformat/mxfdec: Remove this_partition

Message ID 20221224193621.4593-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,v2] avformat/mxfdec: Remove this_partition | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Michael Niedermayer Dec. 24, 2022, 7:36 p.m. UTC 
Suggested-by: Tomas Härdin <git@haerdin.se>

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/mxfdec.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

Comments

Tomas Härdin Dec. 26, 2022, 10:30 a.m. UTC | #1 
lör 2022-12-24 klockan 20:36 +0100 skrev Michael Niedermayer:
> @@ -725,10 +724,14 @@ 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;
>  
> +    if (klv_offset < mxf->run_in)
> +        return AVERROR_INVALIDDATA;

av_assert0() might be more appropriate since this should never happen
regardless of input

> @@ -771,7 +774,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);
> +        this_partition = klv_offset - mxf->run_in;

This could even be outside the if. It turns into a nop if the file is
already good. Not a huge issue either way.

> @@ -900,7 +909,7 @@ static uint64_t partition_score(MXFPartition *p)
>          score = 3;
>      else
>          score = 1;
> -    return (score << 60) | ((uint64_t)p->this_partition >> 4);
> +    return (score << 60) | ((uint64_t)p->pack_ofs >> 4);

Noting that while this changes the scores computed, it doesn't change
the intended behavior of the code. In fact it should make it more
correct in cases where ThisPartition are bogus values.

>  }
>  
>  static int mxf_add_metadata_set(MXFContext *mxf, MXFMetadataSet
> **metadata_set)
> @@ -3520,14 +3529,14 @@ static void
> mxf_compute_essence_containers(AVFormatContext *s)
>  
>              /* essence container spans to the next partition */
>              if (x < mxf->partitions_count - 1)
> -                p->essence_length = mxf-
> >partitions[x+1].this_partition - p->essence_offset;
> +                p->essence_length = mxf->partitions[x+1].pack_ofs -
> mxf->run_in - p->essence_offset;

I wonder if there's perhaps an underflow issue lurking here. Not that
this patch changes that

/Tomas
diff mbox series

Patch

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index e6118e141d..0553728253 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -100,7 +100,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;
@@ -725,10 +724,14 @@  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;
 
+    if (klv_offset < mxf->run_in)
+        return AVERROR_INVALIDDATA;
+
     tmp_part = av_realloc_array(mxf->partitions, mxf->partitions_count + 1, sizeof(*mxf->partitions));
     if (!tmp_part)
         return AVERROR(ENOMEM);
@@ -771,7 +774,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);
+        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);
@@ -791,8 +800,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) {
         av_log(mxf->fc, AV_LOG_ERROR,
                "PreviousPartition equal to ThisPartition %"PRIx64"\n",
                partition->previous_partition);
@@ -800,11 +809,11 @@  static int mxf_read_partition_pack(void *arg, AVIOContext *pb, int tag, int size
         if (!mxf->parsing_backward && mxf->last_forward_partition > 1) {
             MXFPartition *prev =
                 mxf->partitions + mxf->last_forward_partition - 2;
-            partition->previous_partition = prev->this_partition;
+            partition->previous_partition = prev->pack_ofs - mxf->run_in;
         }
         /* if no previous body partition are found point to the header
          * partition */
-        if (partition->previous_partition == partition->this_partition)
+        if (partition->previous_partition == this_partition)
             partition->previous_partition = 0;
         av_log(mxf->fc, AV_LOG_ERROR,
                "Overriding PreviousPartition with %"PRIx64"\n",
@@ -826,7 +835,7 @@  static int mxf_read_partition_pack(void *arg, AVIOContext *pb, int tag, int size
             "PartitionPack: ThisPartition = 0x%"PRIX64
             ", PreviousPartition = 0x%"PRIX64", "
             "FooterPartition = 0x%"PRIX64", IndexSID = %i, BodySID = %i\n",
-            partition->this_partition,
+            this_partition,
             partition->previous_partition, footer_partition,
             partition->index_sid, partition->body_sid);
 
@@ -900,7 +909,7 @@  static uint64_t partition_score(MXFPartition *p)
         score = 3;
     else
         score = 1;
-    return (score << 60) | ((uint64_t)p->this_partition >> 4);
+    return (score << 60) | ((uint64_t)p->pack_ofs >> 4);
 }
 
 static int mxf_add_metadata_set(MXFContext *mxf, MXFMetadataSet **metadata_set)
@@ -3520,14 +3529,14 @@  static void mxf_compute_essence_containers(AVFormatContext *s)
 
             /* essence container spans to the next partition */
             if (x < mxf->partitions_count - 1)
-                p->essence_length = mxf->partitions[x+1].this_partition - p->essence_offset;
+                p->essence_length = mxf->partitions[x+1].pack_ofs - mxf->run_in - p->essence_offset;
 
             if (p->essence_length < 0) {
                 /* next ThisPartition < essence_offset */
                 p->essence_length = 0;
                 av_log(mxf->fc, AV_LOG_ERROR,
                        "partition %i: bad ThisPartition = %"PRIX64"\n",
-                       x+1, mxf->partitions[x+1].this_partition);
+                       x+1, mxf->partitions[x+1].pack_ofs - mxf->run_in);
             }
         }
     }