| Message ID | 20240403225134.31764-4-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | ed49391961999f028e0bc55767d0eef6eeb15e49 |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avcodec/wavarc: fix signed integer overflow in block type 6/19 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
tor 2024-04-04 klockan 00:51 +0200 skrev Michael Niedermayer: > Fixes: Assertion b >=0 failed at libavutil/mathematics.c:62 > Fixes: 67811/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer- > 5108429687422976 > > 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 | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > index 04de4c1d5e3..233d614f783 100644 > --- a/libavformat/mxfdec.c > +++ b/libavformat/mxfdec.c > @@ -1264,6 +1264,9 @@ static int mxf_read_index_table_segment(void > *arg, AVIOContext *pb, int tag, int > case 0x3F0B: > segment->index_edit_rate.num = avio_rb32(pb); > segment->index_edit_rate.den = avio_rb32(pb); > + if (segment->index_edit_rate.num <= 0 || > + segment->index_edit_rate.den <= 0) > + return AVERROR_INVALIDDATA; mxf_compute_index_tables() has a check for index_edit_rate that you probably want to remove as well. It was introduced in c6fff3d, but the files it supposedly fixes aren't in FATE. We shouldn't encourage broken muxers. /Tomas
On Mon, 8 Apr 2024, Tomas Härdin wrote: > tor 2024-04-04 klockan 00:51 +0200 skrev Michael Niedermayer: >> Fixes: Assertion b >=0 failed at libavutil/mathematics.c:62 >> Fixes: 67811/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer- >> 5108429687422976 >> >> 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 | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c >> index 04de4c1d5e3..233d614f783 100644 >> --- a/libavformat/mxfdec.c >> +++ b/libavformat/mxfdec.c >> @@ -1264,6 +1264,9 @@ static int mxf_read_index_table_segment(void >> *arg, AVIOContext *pb, int tag, int >> case 0x3F0B: >> segment->index_edit_rate.num = avio_rb32(pb); >> segment->index_edit_rate.den = avio_rb32(pb); >> + if (segment->index_edit_rate.num <= 0 || >> + segment->index_edit_rate.den <= 0) >> + return AVERROR_INVALIDDATA; > > mxf_compute_index_tables() has a check for index_edit_rate that you > probably want to remove as well. It was introduced in c6fff3d, but the > files it supposedly fixes aren't in FATE. We shouldn't encourage broken > muxers. I don't quite get what FATE has to do with it. And the samples mentioned in the patch has valid index segment edit rates, only they are different from the track edit rate, and the patch was intended to fix that case. Regards, Marton
mån 2024-04-08 klockan 21:46 +0200 skrev Marton Balint: > > > On Mon, 8 Apr 2024, Tomas Härdin wrote: > > > tor 2024-04-04 klockan 00:51 +0200 skrev Michael Niedermayer: > > > Fixes: Assertion b >=0 failed at libavutil/mathematics.c:62 > > > Fixes: 67811/clusterfuzz-testcase-minimized- > > > ffmpeg_dem_MXF_fuzzer- > > > 5108429687422976 > > > > > > 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 | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > > > index 04de4c1d5e3..233d614f783 100644 > > > --- a/libavformat/mxfdec.c > > > +++ b/libavformat/mxfdec.c > > > @@ -1264,6 +1264,9 @@ static int > > > mxf_read_index_table_segment(void > > > *arg, AVIOContext *pb, int tag, int > > > case 0x3F0B: > > > segment->index_edit_rate.num = avio_rb32(pb); > > > segment->index_edit_rate.den = avio_rb32(pb); > > > + if (segment->index_edit_rate.num <= 0 || > > > + segment->index_edit_rate.den <= 0) > > > + return AVERROR_INVALIDDATA; > > > > mxf_compute_index_tables() has a check for index_edit_rate that you > > probably want to remove as well. It was introduced in c6fff3d, but > > the > > files it supposedly fixes aren't in FATE. We shouldn't encourage > > broken > > muxers. > > I don't quite get what FATE has to do with it. And the samples > mentioned > in the patch has valid index segment edit rates, only they are > different > from the track edit rate, and the patch was intended to fix that > case. Then why does it check against 0/0? /Tomas
tis 2024-04-09 klockan 22:58 +0200 skrev Marton Balint: > > > On Tue, 9 Apr 2024, Tomas Härdin wrote: > > > mån 2024-04-08 klockan 21:46 +0200 skrev Marton Balint: > > > > > > > > > On Mon, 8 Apr 2024, Tomas Härdin wrote: > > > > > > > tor 2024-04-04 klockan 00:51 +0200 skrev Michael Niedermayer: > > > > > Fixes: Assertion b >=0 failed at libavutil/mathematics.c:62 > > > > > Fixes: 67811/clusterfuzz-testcase-minimized- > > > > > ffmpeg_dem_MXF_fuzzer- > > > > > 5108429687422976 > > > > > > > > > > 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 | 3 +++ > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > > > > > index 04de4c1d5e3..233d614f783 100644 > > > > > --- a/libavformat/mxfdec.c > > > > > +++ b/libavformat/mxfdec.c > > > > > @@ -1264,6 +1264,9 @@ static int > > > > > mxf_read_index_table_segment(void > > > > > *arg, AVIOContext *pb, int tag, int > > > > > case 0x3F0B: > > > > > segment->index_edit_rate.num = avio_rb32(pb); > > > > > segment->index_edit_rate.den = avio_rb32(pb); > > > > > + if (segment->index_edit_rate.num <= 0 || > > > > > + segment->index_edit_rate.den <= 0) > > > > > + return AVERROR_INVALIDDATA; > > > > > > > > mxf_compute_index_tables() has a check for index_edit_rate that > > > > you > > > > probably want to remove as well. It was introduced in c6fff3d, > > > > but > > > > the > > > > files it supposedly fixes aren't in FATE. We shouldn't > > > > encourage > > > > broken > > > > muxers. > > > > > > I don't quite get what FATE has to do with it. And the samples > > > mentioned > > > in the patch has valid index segment edit rates, only they are > > > different > > > from the track edit rate, and the patch was intended to fix that > > > case. > > > > Then why does it check against 0/0? > > Probably to avoid divison by zero. I think it's safe to say that EditRates with zero in the numerator or denominator are not allowed. We currently default to 25/1 in this case for Tracks, but I am skeptical of this since it encourages broken muxers. As for IndexEditRate, here's what ST 377-1:2019 has to say: > Edit Rate copied from the Essence Tracks of the > Essence Container > [Note: SMPTE RP 210 definition Specifies the > indexing rate in hertz] It's possible to encode a file that does not specify IndexEditRate, but this is not allowed since the field is marked Required in Table 26. mxfdec.c will default to 0/0 since the segment is calloc'd. Michael's fix won't work if one changes the IndexEditRate local tag in the file to something else, say FFFF instead of 3F0B. In short, IndexEditRate MUST be set and it MUST equal the EditRate of the associated Essence Track (confusingly called source_track in the code). Section 11 is even more explicit: > An Index Table shall be used to index a single Essence Container. > Each Index Table shall index Edit Units > stored Essence of the Essence Container. The Edit Unit rate of an > Index Table is defined by the Edit Rate of the > Essence Tracks of the Package that describes the Essence Container > that the Index Table indexes. EditRate MAY be different between MaterialPackage and FilePackage however. This is a consequence of MXF's AAF heritage. MXF is really an NLE format. Section 11.6.2 "Look-up Algorithm for Conversion of Index Position to Stream Offset" is also of relevance. It doesn't make use of IndexEditRate at all. /Tomas
On Wed, 10 Apr 2024, Tomas Härdin wrote: > tis 2024-04-09 klockan 22:58 +0200 skrev Marton Balint: >> >> >> On Tue, 9 Apr 2024, Tomas Härdin wrote: >> >> > mån 2024-04-08 klockan 21:46 +0200 skrev Marton Balint: >> > > >> > > >> > > On Mon, 8 Apr 2024, Tomas Härdin wrote: >> > > >> > > > tor 2024-04-04 klockan 00:51 +0200 skrev Michael Niedermayer: >> > > > > Fixes: Assertion b >=0 failed at libavutil/mathematics.c:62 >> > > > > Fixes: 67811/clusterfuzz-testcase-minimized- >> > > > > ffmpeg_dem_MXF_fuzzer- >> > > > > 5108429687422976 >> > > > > >> > > > > 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 | 3 +++ >> > > > > 1 file changed, 3 insertions(+) >> > > > > >> > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c >> > > > > index 04de4c1d5e3..233d614f783 100644 >> > > > > --- a/libavformat/mxfdec.c >> > > > > +++ b/libavformat/mxfdec.c >> > > > > @@ -1264,6 +1264,9 @@ static int >> > > > > mxf_read_index_table_segment(void >> > > > > *arg, AVIOContext *pb, int tag, int >> > > > > case 0x3F0B: >> > > > > segment->index_edit_rate.num = avio_rb32(pb); >> > > > > segment->index_edit_rate.den = avio_rb32(pb); >> > > > > + if (segment->index_edit_rate.num <= 0 || >> > > > > + segment->index_edit_rate.den <= 0) >> > > > > + return AVERROR_INVALIDDATA; >> > > > >> > > > mxf_compute_index_tables() has a check for index_edit_rate that >> > > > you >> > > > probably want to remove as well. It was introduced in c6fff3d, >> > > > but >> > > > the >> > > > files it supposedly fixes aren't in FATE. We shouldn't >> > > > encourage >> > > > broken >> > > > muxers. >> > > >> > > I don't quite get what FATE has to do with it. And the samples >> > > mentioned >> > > in the patch has valid index segment edit rates, only they are >> > > different >> > > from the track edit rate, and the patch was intended to fix that >> > > case. >> > >> > Then why does it check against 0/0? >> >> Probably to avoid divison by zero. > > I think it's safe to say that EditRates with zero in the numerator or > denominator are not allowed. We currently default to 25/1 in this case > for Tracks, but I am skeptical of this since it encourages broken > muxers. In general, I don't like the idea of rejecting everything which is not following the standard to the letter. Decoding and demuxing should be based on the "Robustness principle", as in being liberal in what we accept and strict in what we generate. I am also not sure about your reasoning that rejecting files will force vendors to fix their muxers, because the users will have to pay the price for this approach. Users may well already have their archives full of non-compliant files, their camera, phone, whatever is likely out of warranty/support, so they might not even be in a position to request anything from vendors. Sure, I get it, some issues cannot be worked around easily, and I am not saying that everything must be supported with huge hacks if needed. But an effort should be made to not break existing real files, and support what we reasonably can. > > As for IndexEditRate, here's what ST 377-1:2019 has to say: > > >> Edit Rate copied from the Essence Tracks of the >> Essence Container >> [Note: SMPTE RP 210 definition Specifies the >> indexing rate in hertz] > > It's possible to encode a file that does not specify IndexEditRate, but > this is not allowed since the field is marked Required in Table 26. > mxfdec.c will default to 0/0 since the segment is calloc'd. Michael's > fix won't work if one changes the IndexEditRate local tag in the file > to something else, say FFFF instead of 3F0B. Yes, you are right about this. To be honest, I'd rather fix the invalid index edit rate issue by dropping the invalid segments when sorting them. That should work for both the explicitly and implicitly invalid index edit rates. > > In short, IndexEditRate MUST be set and it MUST equal the EditRate of > the associated Essence Track (confusingly called source_track in the > code). Section 11 is even more explicit: > >> An Index Table shall be used to index a single Essence Container. >> Each Index Table shall index Edit Units >> stored Essence of the Essence Container. The Edit Unit rate of an >> Index Table is defined by the Edit Rate of the >> Essence Tracks of the Package that describes the Essence Container >> that the Index Table indexes. > > EditRate MAY be different between MaterialPackage and FilePackage > however. This is a consequence of MXF's AAF heritage. MXF is really an > NLE format. > > Section 11.6.2 "Look-up Algorithm for Conversion of Index Position to > Stream Offset" is also of relevance. It doesn't make use of > IndexEditRate at all. Regards, Marton
sön 2024-04-14 klockan 22:55 +0200 skrev Marton Balint: > > > On Wed, 10 Apr 2024, Tomas Härdin wrote: > > > tis 2024-04-09 klockan 22:58 +0200 skrev Marton Balint: > > > > > > > > > On Tue, 9 Apr 2024, Tomas Härdin wrote: > > > > > > > mån 2024-04-08 klockan 21:46 +0200 skrev Marton Balint: > > > > > > > > > > > > > > > On Mon, 8 Apr 2024, Tomas Härdin wrote: > > > > > > > > > > > tor 2024-04-04 klockan 00:51 +0200 skrev Michael > > > > > > Niedermayer: > > > > > > > Fixes: Assertion b >=0 failed at > > > > > > > libavutil/mathematics.c:62 > > > > > > > Fixes: 67811/clusterfuzz-testcase-minimized- > > > > > > > ffmpeg_dem_MXF_fuzzer- > > > > > > > 5108429687422976 > > > > > > > > > > > > > > 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 | 3 +++ > > > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > > > > > > > index 04de4c1d5e3..233d614f783 100644 > > > > > > > --- a/libavformat/mxfdec.c > > > > > > > +++ b/libavformat/mxfdec.c > > > > > > > @@ -1264,6 +1264,9 @@ static int > > > > > > > mxf_read_index_table_segment(void > > > > > > > *arg, AVIOContext *pb, int tag, int > > > > > > > case 0x3F0B: > > > > > > > segment->index_edit_rate.num = avio_rb32(pb); > > > > > > > segment->index_edit_rate.den = avio_rb32(pb); > > > > > > > + if (segment->index_edit_rate.num <= 0 || > > > > > > > + segment->index_edit_rate.den <= 0) > > > > > > > + return AVERROR_INVALIDDATA; > > > > > > > > > > > > mxf_compute_index_tables() has a check for index_edit_rate > > > > > > that > > > > > > you > > > > > > probably want to remove as well. It was introduced in > > > > > > c6fff3d, > > > > > > but > > > > > > the > > > > > > files it supposedly fixes aren't in FATE. We shouldn't > > > > > > encourage > > > > > > broken > > > > > > muxers. > > > > > > > > > > I don't quite get what FATE has to do with it. And the > > > > > samples > > > > > mentioned > > > > > in the patch has valid index segment edit rates, only they > > > > > are > > > > > different > > > > > from the track edit rate, and the patch was intended to fix > > > > > that > > > > > case. > > > > > > > > Then why does it check against 0/0? > > > > > > Probably to avoid divison by zero. > > > > I think it's safe to say that EditRates with zero in the numerator > > or > > denominator are not allowed. We currently default to 25/1 in this > > case > > for Tracks, but I am skeptical of this since it encourages broken > > muxers. > > In general, I don't like the idea of rejecting everything which is > not > following the standard to the letter. Decoding and demuxing should be > based on the "Robustness principle", as in being liberal in what we > accept > and strict in what we generate. No. We should not encourage proliferation of broken muxers. This leads to compounding headaches down the line. > I am also not sure about your reasoning that rejecting files will > force > vendors to fix their muxers, because the users will have to pay the > price > for this approach. Users may well already have their archives full of > non-compliant files, their camera, phone, whatever is likely out of > warranty/support, so they might not even be in a position to request > anything from vendors. These users can sign an SLA if they want support for these cameras. > Sure, I get it, some issues cannot be worked around easily, and I am > not > saying that everything must be supported with huge hacks if needed. > But an > effort should be made to not break existing real files, and support > what > we reasonably can. I mean, at the very least we need such samples in FATE or we can't refactor mxfdec with any level of confidence. Restricting hacks to specific vendors as identified by the Identification set would help bring some order to them. > > As for IndexEditRate, here's what ST 377-1:2019 has to say: > > > > > > > Edit Rate copied from the Essence Tracks of the > > > Essence Container > > > [Note: SMPTE RP 210 definition Specifies the > > > indexing rate in hertz] > > > > It's possible to encode a file that does not specify IndexEditRate, > > but > > this is not allowed since the field is marked Required in Table 26. > > mxfdec.c will default to 0/0 since the segment is calloc'd. > > Michael's > > fix won't work if one changes the IndexEditRate local tag in the > > file > > to something else, say FFFF instead of 3F0B. > > Yes, you are right about this. To be honest, I'd rather fix the > invalid > index edit rate issue by dropping the invalid segments when sorting > them. > That should work for both the explicitly and implicitly invalid index > edit > rates. This is in contradiction with your desire to support broken muxers (: If IndexEditRate doesn't match the IndexRate of the Essence Track then we can't in general know what to do. We can know what to do for files coming out of *specific muxers*, assuming they actually identify themselves. Without knowing this, and without samples, we're just engaging in cargo cult programming, or fear driven development. /Tomas
On Mon, 15 Apr 2024, Tomas Härdin wrote: > sön 2024-04-14 klockan 22:55 +0200 skrev Marton Balint: >> >> >> On Wed, 10 Apr 2024, Tomas Härdin wrote: >> >> > tis 2024-04-09 klockan 22:58 +0200 skrev Marton Balint: >> > > >> > > >> > > On Tue, 9 Apr 2024, Tomas Härdin wrote: >> > > >> > > > mån 2024-04-08 klockan 21:46 +0200 skrev Marton Balint: >> > > > > >> > > > > >> > > > > On Mon, 8 Apr 2024, Tomas Härdin wrote: >> > > > > >> > > > > > tor 2024-04-04 klockan 00:51 +0200 skrev Michael >> > > > > > Niedermayer: >> > > > > > > Fixes: Assertion b >=0 failed at >> > > > > > > libavutil/mathematics.c:62 >> > > > > > > Fixes: 67811/clusterfuzz-testcase-minimized- >> > > > > > > ffmpeg_dem_MXF_fuzzer- >> > > > > > > 5108429687422976 >> > > > > > > >> > > > > > > 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 | 3 +++ >> > > > > > > 1 file changed, 3 insertions(+) >> > > > > > > >> > > > > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c >> > > > > > > index 04de4c1d5e3..233d614f783 100644 >> > > > > > > --- a/libavformat/mxfdec.c >> > > > > > > +++ b/libavformat/mxfdec.c >> > > > > > > @@ -1264,6 +1264,9 @@ static int >> > > > > > > mxf_read_index_table_segment(void >> > > > > > > *arg, AVIOContext *pb, int tag, int >> > > > > > > case 0x3F0B: >> > > > > > > segment->index_edit_rate.num = avio_rb32(pb); >> > > > > > > segment->index_edit_rate.den = avio_rb32(pb); >> > > > > > > + if (segment->index_edit_rate.num <= 0 || >> > > > > > > + segment->index_edit_rate.den <= 0) >> > > > > > > + return AVERROR_INVALIDDATA; >> > > > > > >> > > > > > mxf_compute_index_tables() has a check for index_edit_rate >> > > > > > that >> > > > > > you >> > > > > > probably want to remove as well. It was introduced in >> > > > > > c6fff3d, >> > > > > > but >> > > > > > the >> > > > > > files it supposedly fixes aren't in FATE. We shouldn't >> > > > > > encourage >> > > > > > broken >> > > > > > muxers. >> > > > > >> > > > > I don't quite get what FATE has to do with it. And the >> > > > > samples >> > > > > mentioned >> > > > > in the patch has valid index segment edit rates, only they >> > > > > are >> > > > > different >> > > > > from the track edit rate, and the patch was intended to fix >> > > > > that >> > > > > case. >> > > > >> > > > Then why does it check against 0/0? >> > > >> > > Probably to avoid divison by zero. >> > >> > I think it's safe to say that EditRates with zero in the numerator >> > or >> > denominator are not allowed. We currently default to 25/1 in this >> > case >> > for Tracks, but I am skeptical of this since it encourages broken >> > muxers. >> >> In general, I don't like the idea of rejecting everything which is >> not >> following the standard to the letter. Decoding and demuxing should be >> based on the "Robustness principle", as in being liberal in what we >> accept >> and strict in what we generate. > > No. We should not encourage proliferation of broken muxers. This leads > to compounding headaches down the line. > >> I am also not sure about your reasoning that rejecting files will >> force >> vendors to fix their muxers, because the users will have to pay the >> price >> for this approach. Users may well already have their archives full of >> non-compliant files, their camera, phone, whatever is likely out of >> warranty/support, so they might not even be in a position to request >> anything from vendors. > > These users can sign an SLA if they want support for these cameras. You only want to support non-compliant files if the users pay for it? Regards, Marton
diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index 04de4c1d5e3..233d614f783 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -1264,6 +1264,9 @@ static int mxf_read_index_table_segment(void *arg, AVIOContext *pb, int tag, int case 0x3F0B: segment->index_edit_rate.num = avio_rb32(pb); segment->index_edit_rate.den = avio_rb32(pb); + if (segment->index_edit_rate.num <= 0 || + segment->index_edit_rate.den <= 0) + return AVERROR_INVALIDDATA; av_log(NULL, AV_LOG_TRACE, "IndexEditRate %d/%d\n", segment->index_edit_rate.num, segment->index_edit_rate.den); break;
Fixes: Assertion b >=0 failed at libavutil/mathematics.c:62 Fixes: 67811/clusterfuzz-testcase-minimized-ffmpeg_dem_MXF_fuzzer-5108429687422976 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 | 3 +++ 1 file changed, 3 insertions(+)