| Message ID | 20230922191344.7018-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v3] avformat/mxfdec: Remove this_partition | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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
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 [...]
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
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! [...]
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
[...]
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
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); } } }
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(-)