diff mbox

[FFmpeg-devel] avcodec/webp: avoid trying to decode trailing junk in bitstreams

Message ID CAKkUsvyDpQ-=ktUcq2d1-4X6ieqh6KZhG1GBznitKdap8c11FA@mail.gmail.com
State New
Headers show

Commit Message

Pascal Massimino Sept. 6, 2019, 11:11 a.m. UTC 
Michael,

On Thu, Sep 5, 2019 at 6:20 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Wed, Sep 04, 2019 at 07:43:15AM +0200, Pascal Massimino wrote:
> > Hi,
> >
> > this patch break the decoding loop when invalid webp chunk is
> encountered.
> > We can still have a valid frame ready to be returned (*got_frame = 1).
> >
>
> > fixes trac #8107 (/#7612)
>
> These bug references should be in the commit message
>

done, new patch attached.


>
>
> >
> > skal/
>
> >  webp.c |    7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 2d80b062adade6044f64a00838b55f9427cc1f73
> 0001-webp-fix-decoding-for-trailing-junk.patch
> > From 9edff4f9812fad7f605bdc12954f82a8745a25ee Mon Sep 17 00:00:00 2001
> > From: Pascal Massimino <pascal.massimino@gmail.com>
> > Date: Wed, 28 Aug 2019 09:41:42 +0200
> > Subject: [PATCH] webp: fix decoding for trailing junk
> >
> > some bitstream have trailing junk, despite being valid webp data.
> > In case of apparent error, abort the loop and let *got_frame
> > decide whether this is an error or not.
> > Another possibility would be turning the loop into:
> >     while (!*got_frame) {...}
>
> what is that trailing junk ?
>

As mentioned in the trac description, one frequent source of such junk
is padding of encrypted packets (or, just, left-overs).


>
> i would guess its not a known chunk but rather hits the default
> is that just a bunch of 0 or 0xFF bytes ?
> detecting before we read into the end feels more robust if
> we can simply detect the "junk"
>

As i mentioned in the description, a possibly more robust solution would be
just stopping the loop as soon as 'chunk_size' bytes have been consumed
(leading to *got_frame = 1) and no more. This current patch is minimalist,
though.

skal/





>
> thanks
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Comments

Michael Niedermayer Sept. 6, 2019, 4:20 p.m. UTC | #1 
On Fri, Sep 06, 2019 at 01:11:50PM +0200, Pascal Massimino wrote:
> Michael,
> 
> On Thu, Sep 5, 2019 at 6:20 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Wed, Sep 04, 2019 at 07:43:15AM +0200, Pascal Massimino wrote:
[...]

> >
> > i would guess its not a known chunk but rather hits the default
> > is that just a bunch of 0 or 0xFF bytes ?
> > detecting before we read into the end feels more robust if
> > we can simply detect the "junk"
> >
> 
> As i mentioned in the description, a possibly more robust solution would be
> just stopping the loop as soon as 'chunk_size' bytes have been consumed
> (leading to *got_frame = 1) and no more. This current patch is minimalist,
> though.

well, which solution do you prefer ?

thx

[...]
Pascal Massimino Sept. 9, 2019, 9:13 a.m. UTC | #2 
Michael,

On Fri, Sep 6, 2019 at 6:21 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Fri, Sep 06, 2019 at 01:11:50PM +0200, Pascal Massimino wrote:
> > Michael,
> >
> > On Thu, Sep 5, 2019 at 6:20 PM Michael Niedermayer
> <michael@niedermayer.cc>
> > wrote:
> >
> > > On Wed, Sep 04, 2019 at 07:43:15AM +0200, Pascal Massimino wrote:
> [...]
>
> > >
> > > i would guess its not a known chunk but rather hits the default
> > > is that just a bunch of 0 or 0xFF bytes ?
> > > detecting before we read into the end feels more robust if
> > > we can simply detect the "junk"
> > >
> >
> > As i mentioned in the description, a possibly more robust solution would
> be
> > just stopping the loop as soon as 'chunk_size' bytes have been consumed
> > (leading to *got_frame = 1) and no more. This current patch is
> minimalist,
> > though.
>
> well, which solution do you prefer ?
>

the one in the patch is fine.

skal/


>
> thx
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The educated differ from the uneducated as much as the living from the
> dead. -- Aristotle
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Sept. 10, 2019, 3:11 p.m. UTC | #3 
On Mon, Sep 09, 2019 at 11:13:53AM +0200, Pascal Massimino wrote:
> Michael,
> 
> On Fri, Sep 6, 2019 at 6:21 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Fri, Sep 06, 2019 at 01:11:50PM +0200, Pascal Massimino wrote:
> > > Michael,
> > >
> > > On Thu, Sep 5, 2019 at 6:20 PM Michael Niedermayer
> > <michael@niedermayer.cc>
> > > wrote:
> > >
> > > > On Wed, Sep 04, 2019 at 07:43:15AM +0200, Pascal Massimino wrote:
> > [...]
> >
> > > >
> > > > i would guess its not a known chunk but rather hits the default
> > > > is that just a bunch of 0 or 0xFF bytes ?
> > > > detecting before we read into the end feels more robust if
> > > > we can simply detect the "junk"
> > > >
> > >
> > > As i mentioned in the description, a possibly more robust solution would
> > be
> > > just stopping the loop as soon as 'chunk_size' bytes have been consumed
> > > (leading to *got_frame = 1) and no more. This current patch is
> > minimalist,
> > > though.
> >
> > well, which solution do you prefer ?
> >
> 
> the one in the patch is fine.

ok, will apply

thx

[...]
Pascal Massimino Sept. 10, 2019, 11:03 p.m. UTC | #4 
On Tue, Sep 10, 2019 at 5:12 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Mon, Sep 09, 2019 at 11:13:53AM +0200, Pascal Massimino wrote:
> > Michael,
> >
> > On Fri, Sep 6, 2019 at 6:21 PM Michael Niedermayer
> <michael@niedermayer.cc>
> > wrote:
> >
> > > On Fri, Sep 06, 2019 at 01:11:50PM +0200, Pascal Massimino wrote:
> > > > Michael,
> > > >
> > > > On Thu, Sep 5, 2019 at 6:20 PM Michael Niedermayer
> > > <michael@niedermayer.cc>
> > > > wrote:
> > > >
> > > > > On Wed, Sep 04, 2019 at 07:43:15AM +0200, Pascal Massimino wrote:
> > > [...]
> > >
> > > > >
> > > > > i would guess its not a known chunk but rather hits the default
> > > > > is that just a bunch of 0 or 0xFF bytes ?
> > > > > detecting before we read into the end feels more robust if
> > > > > we can simply detect the "junk"
> > > > >
> > > >
> > > > As i mentioned in the description, a possibly more robust solution
> would
> > > be
> > > > just stopping the loop as soon as 'chunk_size' bytes have been
> consumed
> > > > (leading to *got_frame = 1) and no more. This current patch is
> > > minimalist,
> > > > though.
> > >
> > > well, which solution do you prefer ?
> > >
> >
> > the one in the patch is fine.
>
> ok, will apply
>

thx


>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If you drop bombs on a foreign country and kill a hundred thousand
> innocent people, expect your government to call the consequence
> "unprovoked inhuman terrorist attacks" and use it to justify dropping
> more bombs and killing more people. The technology changed, the idea is
> old.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox

Patch

From 5c3a8878cce7e2920c39e58a0b57872d8c96b511 Mon Sep 17 00:00:00 2001
From: Pascal Massimino <pascal.massimino@gmail.com>
Date: Wed, 28 Aug 2019 09:41:42 +0200
Subject: [PATCH] avcodec/webp: fix decoding for trailing junk

some bitstream have trailing junk, despite being valid webp data.
In case of apparent error, abort the loop and let *got_frame
decide whether this is an error or not.

fixes trac #8107 (/#7612)

Another possibility would be turning the loop into:
    while (!*got_frame) {...}
---
 libavcodec/webp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libavcodec/webp.c b/libavcodec/webp.c
index 077bb06f85..c6d0206846 100644
--- a/libavcodec/webp.c
+++ b/libavcodec/webp.c
@@ -1412,8 +1412,11 @@  static int webp_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
             return AVERROR_INVALIDDATA;
         chunk_size += chunk_size & 1;
 
-        if (bytestream2_get_bytes_left(&gb) < chunk_size)
-            return AVERROR_INVALIDDATA;
+        if (bytestream2_get_bytes_left(&gb) < chunk_size) {
+           /* we seem to be running out of data, but it could also be that the
+              bitstream has trailing junk leading to bogus chunk_size. */
+            break;
+        }
 
         switch (chunk_type) {
         case MKTAG('V', 'P', '8', ' '):
-- 
2.23.0.187.g17f5b7556c-goog