[FFmpeg-devel] [PATCH] avformat/matroskadec: Check parents remaining length

[Steve Lhomme]

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 at chromium.org> wrote:
> 
>     +steve who submitted the original patch.
> 
>     - dale
> 
> 
>     On Thu, Feb 21, 2019 at 2:30 PM Dale Curtis < dalecurtis at chromium.org mailto:dalecurtis at 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 at gmx.at mailto:michaelni at 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 at 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 at ffmpeg.org mailto:ffmpeg-devel at ffmpeg.org
> > >             http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > 
> > >         > > 
> >     >