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

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

[Michael Niedermayer]

Suggested-by: Tomas Härdin <git@haerdin.se>
Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-5130394286817280

Signed-off-by: Michael Niedermayer <michael@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);
+    }
+    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) {
         av_log(mxf->fc, AV_LOG_ERROR,
                "PreviousPartition equal to ThisPartition %"PRIx64"\n",
                partition->previous_partition);
@@ -802,11 +810,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",
@@ -828,7 +836,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);
 
@@ -902,7 +910,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)
@@ -3539,14 +3547,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);
             }
         }
     }
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [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@haerdin.se>
> Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-
> 5130394286817280
> 
> Signed-off-by: Michael Niedermayer <michael@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2707 bytes --]

Hi

On Wed, Sep 27, 2023 at 01:37:40PM +0200, Tomas Härdin wrote:
> fre 2023-09-22 klockan 21:13 +0200 skrev Michael Niedermayer:
> > Suggested-by: Tomas Härdin <git@haerdin.se>
> > Fixes: 51896/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-
> > 5130394286817280
> > 
> > Signed-off-by: Michael Niedermayer <michael@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.

ok, should i ask for a sample here (so we maybe get a sample)
or just error out with this message at AV_LOG_ERROR ?

thx

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

No great genius has ever existed without some touch of madness. -- Aristotle

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Tomas Härdin]

ons 2023-09-27 klockan 23:12 +0200 skrev Michael Niedermayer:
> Hi
> 
> On Wed, Sep 27, 2023 at 01:37:40PM +0200, Tomas Härdin wrote:
> > fre 2023-09-22 klockan 21:13 +0200 skrev Michael Niedermayer:
> > > Suggested-by: Tomas Härdin <git@haerdin.se>
> > > Fixes: 51896/clusterfuzz-testcase-minimized-
> > > ffmpeg_dem_MXF_fuzzer-
> > > 5130394286817280
> > > 
> > > Signed-off-by: Michael Niedermayer <michael@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.
> 
> ok, should i ask for a sample here (so we maybe get a sample)
> or just error out with this message at AV_LOG_ERROR ?

Nah, just error out

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3277 bytes --]

On Thu, Sep 28, 2023 at 01:12:02PM +0200, Tomas Härdin wrote:
> ons 2023-09-27 klockan 23:12 +0200 skrev Michael Niedermayer:
> > Hi
> > 
> > On Wed, Sep 27, 2023 at 01:37:40PM +0200, Tomas Härdin wrote:
> > > fre 2023-09-22 klockan 21:13 +0200 skrev Michael Niedermayer:
> > > > Suggested-by: Tomas Härdin <git@haerdin.se>
> > > > Fixes: 51896/clusterfuzz-testcase-minimized-
> > > > ffmpeg_dem_MXF_fuzzer-
> > > > 5130394286817280
> > > > 
> > > > Signed-off-by: Michael Niedermayer <michael@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.
> > 
> > ok, should i ask for a sample here (so we maybe get a sample)
> > or just error out with this message at AV_LOG_ERROR ?
> 
> Nah, just error out

ok will apply with that

thanks!

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

Take away the freedom of one citizen and you will be jailed, take away
the freedom of all citizens and you will be congratulated by your peers
in Parliament.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 403 bytes --]

On Fri, Sep 29, 2023 at 06:55:22PM +0200, Michael Niedermayer wrote:
[...]
> ok will apply with that

fixing this issue caused ossfuzz to reveal 24 unrelated unfixed issues
that where hidden in the same issue number as this

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Democracy is the form of government in which you can choose your dictator

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Tomas Härdin]

lör 2023-09-30 klockan 01:32 +0200 skrev Michael Niedermayer:
> On Fri, Sep 29, 2023 at 06:55:22PM +0200, Michael Niedermayer wrote:
> [...]
> > ok will apply with that
> 
> fixing this issue caused ossfuzz to reveal 24 unrelated unfixed
> issues
> that where hidden in the same issue number as this

Being strict pays dividends

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".