| Message ID | 20221224225034.449-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/mxfdec: Check index_duration | expand |
| 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 |
lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer: > > index_table->nb_ptses += s->index_duration; > + // If index_duration is substantially larger than > nb_index_entries then this algorithm which > + // allocates index_duration elements is a bad idea. All > files i tried have it equal > + if (s->index_duration > 10LL * s->nb_index_entries) > + return AVERROR_PATCHWELCOME; I was going to say this can overflow but the 10LL ensures it can't. So looks OK. /Tomas
On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote: > lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer: > > > > index_table->nb_ptses += s->index_duration; > > + // If index_duration is substantially larger than > > nb_index_entries then this algorithm which > > + // allocates index_duration elements is a bad idea. All > > files i tried have it equal > > + if (s->index_duration > 10LL * s->nb_index_entries) > > + return AVERROR_PATCHWELCOME; > > I was going to say this can overflow but the 10LL ensures it can't. So > looks OK. will apply thx [...]
On Mon, 26 Dec 2022, Michael Niedermayer wrote: > On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote: >> lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer: >>> >>> index_table->nb_ptses += s->index_duration; >>> + // If index_duration is substantially larger than >>> nb_index_entries then this algorithm which >>> + // allocates index_duration elements is a bad idea. All >>> files i tried have it equal >>> + if (s->index_duration > 10LL * s->nb_index_entries) >>> + return AVERROR_PATCHWELCOME; >> >> I was going to say this can overflow but the 10LL ensures it can't. So >> looks OK. > > will apply Please don't, as far as I see this disallows the usage of partial index tables, so practically rejecting valid files, which is not OK. Regards, Marton
On Tue, Dec 27, 2022 at 07:05:44PM +0100, Marton Balint wrote: > > > On Mon, 26 Dec 2022, Michael Niedermayer wrote: > > > On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote: > > > lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer: > > > > > > > > index_table->nb_ptses += s->index_duration; > > > > + // If index_duration is substantially larger than > > > > nb_index_entries then this algorithm which > > > > + // allocates index_duration elements is a bad idea. All > > > > files i tried have it equal > > > > + if (s->index_duration > 10LL * s->nb_index_entries) > > > > + return AVERROR_PATCHWELCOME; > > > > > > I was going to say this can overflow but the 10LL ensures it can't. So > > > looks OK. > > > > will apply > > Please don't, as far as I see this disallows the usage of partial index > tables, so practically rejecting valid files, which is not OK. can you share a file that would break ? thx [...]
On Tue, 27 Dec 2022, Michael Niedermayer wrote: > On Tue, Dec 27, 2022 at 07:05:44PM +0100, Marton Balint wrote: >> >> >> On Mon, 26 Dec 2022, Michael Niedermayer wrote: >> >>> On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote: >>>> lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer: >>>>> >>>>> index_table->nb_ptses += s->index_duration; >>>>> + // If index_duration is substantially larger than >>>>> nb_index_entries then this algorithm which >>>>> + // allocates index_duration elements is a bad idea. All >>>>> files i tried have it equal >>>>> + if (s->index_duration > 10LL * s->nb_index_entries) >>>>> + return AVERROR_PATCHWELCOME; >>>> >>>> I was going to say this can overflow but the 10LL ensures it can't. So >>>> looks OK. >>> >>> will apply >> >> Please don't, as far as I see this disallows the usage of partial index >> tables, so practically rejecting valid files, which is not OK. > > can you share a file that would break ? I don't have such file. But the MXF specs (SMPTE 377-1-2009) explictly defines the concept of partial index tables: "Where all Index Table segments are contiguous, or there is only one segment, but not all Edit Units in the Essence Container are indexed, these tables are called Partial Index Tables." As far as I see here nb_index_entries is corresponding to the number of indexed edit units, and the number is allowed to be smaller than the index duration, because not all edit units have to be indexed. Regards, Marton
tis 2022-12-27 klockan 22:49 +0100 skrev Michael Niedermayer: > On Tue, Dec 27, 2022 at 07:05:44PM +0100, Marton Balint wrote: > > > > > > On Mon, 26 Dec 2022, Michael Niedermayer wrote: > > > > > On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote: > > > > lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer: > > > > > > > > > > index_table->nb_ptses += s->index_duration; > > > > > + // If index_duration is substantially larger than > > > > > nb_index_entries then this algorithm which > > > > > + // allocates index_duration elements is a bad idea. > > > > > All > > > > > files i tried have it equal > > > > > + if (s->index_duration > 10LL * s->nb_index_entries) > > > > > + return AVERROR_PATCHWELCOME; > > > > > > > > I was going to say this can overflow but the 10LL ensures it > > > > can't. So > > > > looks OK. > > > > > > will apply > > > > Please don't, as far as I see this disallows the usage of partial > > index > > tables, so practically rejecting valid files, which is not OK. Damn, not sure how I missed that. The use of the magic constant 10 should have tipped me off. > > can you share a file that would break ? mxfenc will produce files like that. It's actually a major deficiency with the muxer since it produces excessive amounts of partitions. /Tomas
On Tue, 27 Dec 2022, Marton Balint wrote: > > > On Tue, 27 Dec 2022, Michael Niedermayer wrote: > >> On Tue, Dec 27, 2022 at 07:05:44PM +0100, Marton Balint wrote: >>> >>> >>> On Mon, 26 Dec 2022, Michael Niedermayer wrote: >>> >>>> On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote: >>>>> lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer: >>>>>> >>>>>> index_table->nb_ptses += s->index_duration; >>>>>> + // If index_duration is substantially larger than >>>>>> nb_index_entries then this algorithm which >>>>>> + // allocates index_duration elements is a bad idea. All >>>>>> files i tried have it equal >>>>>> + if (s->index_duration > 10LL * s->nb_index_entries) >>>>>> + return AVERROR_PATCHWELCOME; >>>>> >>>>> I was going to say this can overflow but the 10LL ensures it can't. So >>>>> looks OK. >>>> >>>> will apply >>> >>> Please don't, as far as I see this disallows the usage of partial index >>> tables, so practically rejecting valid files, which is not OK. >> >> can you share a file that would break ? > > I don't have such file. But the MXF specs (SMPTE 377-1-2009) explictly > defines the concept of partial index tables: > > "Where all Index Table segments are contiguous, or there is only one segment, > but not all Edit Units in the Essence Container are indexed, these tables are > called Partial Index Tables." > > As far as I see here nb_index_entries is corresponding to the number of > indexed edit units, and the number is allowed to be smaller than the index > duration, because not all edit units have to be indexed. I read the specs again, and it seems that I misread it the first time, because partial index tables mean that the index segments have no gaps between them, but the index still not cover the whole essence. So it is not referring to the index entries in the segment. So, in principal your patch *might* be OK. However, existing code simply ignores a corrupt index table, does not reject it. I kind of prefer we make the check more strict, but gracefully allow corrupted index by ignoring it fully. I will post a follow up patch series. Regards, Marton
On Wed, 28 Dec 2022, Marton Balint wrote: > > > On Tue, 27 Dec 2022, Marton Balint wrote: > >> >> >> On Tue, 27 Dec 2022, Michael Niedermayer wrote: >> >>> On Tue, Dec 27, 2022 at 07:05:44PM +0100, Marton Balint wrote: >>>> >>>> >>>> On Mon, 26 Dec 2022, Michael Niedermayer wrote: >>>> >>>>> On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote: >>>>>> lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer: >>>>>>> >>>>>>> index_table->nb_ptses += s->index_duration; >>>>>>> + // If index_duration is substantially larger than >>>>>>> nb_index_entries then this algorithm which >>>>>>> + // allocates index_duration elements is a bad idea. All >>>>>>> files i tried have it equal >>>>>>> + if (s->index_duration > 10LL * s->nb_index_entries) >>>>>>> + return AVERROR_PATCHWELCOME; >>>>>> >>>>>> I was going to say this can overflow but the 10LL ensures it can't. >>>>>> So >>>>>> looks OK. >>>>> >>>>> will apply >>>> >>>> Please don't, as far as I see this disallows the usage of partial index >>>> tables, so practically rejecting valid files, which is not OK. >>> >>> can you share a file that would break ? >> >> I don't have such file. But the MXF specs (SMPTE 377-1-2009) explictly >> defines the concept of partial index tables: >> >> "Where all Index Table segments are contiguous, or there is only one >> segment, but not all Edit Units in the Essence Container are indexed, >> these tables are called Partial Index Tables." >> >> As far as I see here nb_index_entries is corresponding to the number of >> indexed edit units, and the number is allowed to be smaller than the index >> duration, because not all edit units have to be indexed. > > I read the specs again, and it seems that I misread it the first time, > because partial index tables mean that the index segments have no gaps > between them, but the index still not cover the whole essence. So it is not > referring to the index entries in the segment. > > So, in principal your patch *might* be OK. However, existing code simply > ignores a corrupt index table, does not reject it. I kind of prefer we make > the check more strict, but gracefully allow corrupted index by ignoring it > fully. > > I will post a follow up patch series. Ping for the series I posted. Thanks, Marton
On Sun, 22 Jan 2023, Marton Balint wrote: > > > On Wed, 28 Dec 2022, Marton Balint wrote: > >> >> >> On Tue, 27 Dec 2022, Marton Balint wrote: >> >>> >>> >>> On Tue, 27 Dec 2022, Michael Niedermayer wrote: >>> >>>> On Tue, Dec 27, 2022 at 07:05:44PM +0100, Marton Balint wrote: >>>>> >>>>> >>>>> On Mon, 26 Dec 2022, Michael Niedermayer wrote: >>>>> >>>>>> On Mon, Dec 26, 2022 at 11:32:48AM +0100, Tomas Härdin wrote: >>>>>>> lör 2022-12-24 klockan 23:50 +0100 skrev Michael Niedermayer: >>>>>>>> >>>>>>>> index_table->nb_ptses += s->index_duration; >>>>>>>> + // If index_duration is substantially larger than >>>>>>>> nb_index_entries then this algorithm which >>>>>>>> + // allocates index_duration elements is a bad idea. All >>>>>>>> files i tried have it equal >>>>>>>> + if (s->index_duration > 10LL * s->nb_index_entries) >>>>>>>> + return AVERROR_PATCHWELCOME; >>>>>>> >>>>>>> I was going to say this can overflow but the 10LL ensures it can't. >>>>>>> So >>>>>>> looks OK. >>>>>> >>>>>> will apply >>>>> >>>>> Please don't, as far as I see this disallows the usage of partial >>>>> index >>>>> tables, so practically rejecting valid files, which is not OK. >>>> >>>> can you share a file that would break ? >>> >>> I don't have such file. But the MXF specs (SMPTE 377-1-2009) explictly >>> defines the concept of partial index tables: >>> >>> "Where all Index Table segments are contiguous, or there is only one >>> segment, but not all Edit Units in the Essence Container are indexed, >>> these tables are called Partial Index Tables." >>> >>> As far as I see here nb_index_entries is corresponding to the number of >>> indexed edit units, and the number is allowed to be smaller than the >>> index >>> duration, because not all edit units have to be indexed. >> >> I read the specs again, and it seems that I misread it the first time, >> because partial index tables mean that the index segments have no gaps >> between them, but the index still not cover the whole essence. So it is >> not referring to the index entries in the segment. >> >> So, in principal your patch *might* be OK. However, existing code simply >> ignores a corrupt index table, does not reject it. I kind of prefer we >> make the check more strict, but gracefully allow corrupted index by >> ignoring it fully. >> >> I will post a follow up patch series. > > Ping for the series I posted. Will apply. Regards, Marton
diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index 0553728253..64ac6a44b8 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -1943,6 +1943,10 @@ static int mxf_compute_ptses_fake_index(MXFContext *mxf, MXFIndexTable *index_ta } index_table->nb_ptses += s->index_duration; + // If index_duration is substantially larger than nb_index_entries then this algorithm which + // allocates index_duration elements is a bad idea. All files i tried have it equal + if (s->index_duration > 10LL * s->nb_index_entries) + return AVERROR_PATCHWELCOME; } /* paranoid check */
Fixes: OOM Fixes: 50551/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-6607795234930688 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/mxfdec.c | 4 ++++ 1 file changed, 4 insertions(+)