Message ID | CAPUDrwdgFMebytMhKfUbCok7Yk1wd2h0=vfkGTy97Q2ta_DGJQ@mail.gmail.com |
---|---|
State | New |
Headers | show |
+steve who submitted the original patch. - dale On Thu, Feb 21, 2019 at 2:30 PM Dale Curtis <dalecurtis@chromium.org> wrote: > One of our test clips is behaving differently after this patch: > https://cs.chromium.org/chromium/src/media/test/data/bear-320x240-live.webm > > The printed log message is: > [matroska,webm @ 0x1516c84f4e00] Invalid length 0xffffffffffffff > > 0x10000000000002f in parent > > Looking through the code I'm unsure which of the mixed usage > "(uint64_t)-1" and "2**56-1" as marker values is correct. Changing the code > to: > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > index 9b706ab4e0..3015a0b230 100644 > --- a/libavformat/matroskadec.c > +++ b/libavformat/matroskadec.c > @@ -1205,7 +1205,7 @@ static int ebml_parse_elem(MatroskaDemuxContext > *matroska, > MatroskaLevel *level = &matroska->levels[matroska->num_levels > - 1]; > AVIOContext *pb = matroska->ctx->pb; > int64_t pos = avio_tell(pb); > - if (level->length != (uint64_t) -1 && > + if (level->length != 0xffffffffffffffULL && > (pos + length) > (level->start + level->length)) { > av_log(matroska->ctx, AV_LOG_ERROR, > "Invalid length 0x%"PRIx64" > 0x%"PRIx64" in > parent\n", > > Resolves our issues. Should all other length tests be done the same way? > > - dale > > On Sun, Feb 17, 2019 at 12:45 AM Michael Niedermayer <michaelni@gmx.at> > wrote: > >> On Wed, Feb 13, 2019 at 01:41:31PM +0100, Michael Niedermayer wrote: >> > Reported-by: Steve Lhomme >> > This was found through the Hacker One program on VLC but is not a >> security issue in libavformat >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> > --- >> > libavformat/matroskadec.c | 21 +++++++++++++++++++++ >> > 1 file changed, 21 insertions(+) >> >> will apply an equivalent variant from steve >> >> [...] >> -- >> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB >> >> Asymptotically faster algorithms should always be preferred if you have >> asymptotical amounts of data >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >
Sent http://ffmpeg.org/pipermail/ffmpeg-devel/2019-February/240418.html - which passes fate and fixes the issue with our test clip. - dale On Fri, Feb 22, 2019 at 12:31 PM Dale Curtis <dalecurtis@chromium.org> wrote: > +steve who submitted the original patch. > > - dale > > > On Thu, Feb 21, 2019 at 2:30 PM Dale Curtis <dalecurtis@chromium.org> > wrote: > >> One of our test clips is behaving differently after this patch: >> >> https://cs.chromium.org/chromium/src/media/test/data/bear-320x240-live.webm >> >> The printed log message is: >> [matroska,webm @ 0x1516c84f4e00] Invalid length 0xffffffffffffff > >> 0x10000000000002f in parent >> >> Looking through the code I'm unsure which of the mixed usage >> "(uint64_t)-1" and "2**56-1" as marker values is correct. Changing the code >> to: >> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c >> index 9b706ab4e0..3015a0b230 100644 >> --- a/libavformat/matroskadec.c >> +++ b/libavformat/matroskadec.c >> @@ -1205,7 +1205,7 @@ static int ebml_parse_elem(MatroskaDemuxContext >> *matroska, >> MatroskaLevel *level = >> &matroska->levels[matroska->num_levels - 1]; >> AVIOContext *pb = matroska->ctx->pb; >> int64_t pos = avio_tell(pb); >> - if (level->length != (uint64_t) -1 && >> + if (level->length != 0xffffffffffffffULL && >> (pos + length) > (level->start + level->length)) { >> av_log(matroska->ctx, AV_LOG_ERROR, >> "Invalid length 0x%"PRIx64" > 0x%"PRIx64" in >> parent\n", >> >> Resolves our issues. Should all other length tests be done the same way? >> >> - dale >> >> On Sun, Feb 17, 2019 at 12:45 AM Michael Niedermayer <michaelni@gmx.at> >> wrote: >> >>> On Wed, Feb 13, 2019 at 01:41:31PM +0100, Michael Niedermayer wrote: >>> > Reported-by: Steve Lhomme >>> > This was found through the Hacker One program on VLC but is not a >>> security issue in libavformat >>> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> > --- >>> > libavformat/matroskadec.c | 21 +++++++++++++++++++++ >>> > 1 file changed, 21 insertions(+) >>> >>> will apply an equivalent variant from steve >>> >>> [...] >>> -- >>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB >>> >>> Asymptotically faster algorithms should always be preferred if you have >>> asymptotical amounts of data >>> _______________________________________________ >>> ffmpeg-devel mailing list >>> ffmpeg-devel@ffmpeg.org >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> >>
Hi, It's possible that it fixes the issue. When I did the patch I noticed some places used a similar value and some are using -1. I think this should be a define (probably set to UINT64_MAX) that is used everywhere. It's just a marker to say the length is unknown. > On February 22, 2019 at 9:31 PM Dale Curtis <dalecurtis@chromium.org> wrote: > > +steve who submitted the original patch. > > - dale > > > On Thu, Feb 21, 2019 at 2:30 PM Dale Curtis < dalecurtis@chromium.org mailto:dalecurtis@chromium.org > wrote: > > > > One of our test clips is behaving differently after this patch: > > https://cs.chromium.org/chromium/src/media/test/data/bear-320x240-live.webm > > > > The printed log message is: > > [matroska,webm @ 0x1516c84f4e00] Invalid length 0xffffffffffffff > 0x10000000000002f in parent > > > > Looking through the code I'm unsure which of the mixed usage "(uint64_t)-1" and "2**56-1" as marker values is correct. Changing the code to: > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > > index 9b706ab4e0..3015a0b230 100644 > > --- a/libavformat/matroskadec.c > > +++ b/libavformat/matroskadec.c > > @@ -1205,7 +1205,7 @@ static int ebml_parse_elem(MatroskaDemuxContext *matroska, > > MatroskaLevel *level = &matroska->levels[matroska->num_levels - 1]; > > AVIOContext *pb = matroska->ctx->pb; > > int64_t pos = avio_tell(pb); > > - if (level->length != (uint64_t) -1 && > > + if (level->length != 0xffffffffffffffULL && > > (pos + length) > (level->start + level->length)) { > > av_log(matroska->ctx, AV_LOG_ERROR, > > "Invalid length 0x%"PRIx64" > 0x%"PRIx64" in parent\n", > > > > Resolves our issues. Should all other length tests be done the same way? > > > > - dale > > > > On Sun, Feb 17, 2019 at 12:45 AM Michael Niedermayer < michaelni@gmx.at mailto:michaelni@gmx.at > wrote: > > > > > > > On Wed, Feb 13, 2019 at 01:41:31PM +0100, Michael Niedermayer wrote: > > > > Reported-by: Steve Lhomme > > > > This was found through the Hacker One program on VLC but is not a security issue in libavformat > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavformat/matroskadec.c | 21 +++++++++++++++++++++ > > > > 1 file changed, 21 insertions(+) > > > > > > will apply an equivalent variant from steve > > > > > > [...] > > > -- > > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > > > Asymptotically faster algorithms should always be preferred if you have > > > asymptotical amounts of data > > > _______________________________________________ > > > ffmpeg-devel mailing list > > > ffmpeg-devel@ffmpeg.org mailto:ffmpeg-devel@ffmpeg.org > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > > > > > > >
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index 9b706ab4e0..3015a0b230 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -1205,7 +1205,7 @@ static int ebml_parse_elem(MatroskaDemuxContext *matroska, MatroskaLevel *level = &matroska->levels[matroska->num_levels - 1]; AVIOContext *pb = matroska->ctx->pb; int64_t pos = avio_tell(pb); - if (level->length != (uint64_t) -1 && + if (level->length != 0xffffffffffffffULL && (pos + length) > (level->start + level->length)) { av_log(matroska->ctx, AV_LOG_ERROR,