[FFmpeg-devel,4/4] avcodec/hqx: Check the input data against the image size

Submitted by Michael Niedermayer on July 20, 2019, 10:08 p.m.

Details

Message ID 20190720220856.17516-4-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer July 20, 2019, 10:08 p.m.
Fixes: Timeout (22 -> 7 sec)
Fixes: 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/hqx.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Lynne July 20, 2019, 10:36 p.m.
Jul 20, 2019, 11:08 PM by michael@niedermayer.cc:

> Fixes: Timeout (22 -> 7 sec)
> Fixes: 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
>
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/hqx.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
> index bc24ba91d1..8639d77a41 100644
> --- a/libavcodec/hqx.c
> +++ b/libavcodec/hqx.c
> @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void *data,
>  avctx->height              = ctx->height;
>  avctx->bits_per_raw_sample = 10;
>  
> +    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
> +        (100 - avctx->discard_damaged_percentage) / 100 > 8LL * avpkt->size)
> +        return AVERROR_INVALIDDATA;
> + 
>

Not only are you ignoring my and others opinion, not only you still continue sending these awful patches,
you've just submitted by far the worst one I've ever seen thinking its okay.
Patches like these motivate developers to not even bother including test samples for new decoders, or even write them. Myself included. Doing exactly the opposite of what this system's meant to help.
Sure, you sent this for review, but how can you even consider this utterly ridiculous hack for a problem that doesn't exist even worthy for review in the first place? Just what the fuck?
Carl Eugen Hoyos July 20, 2019, 11:31 p.m.
> Am 21.07.2019 um 00:36 schrieb Lynne <dev@lynne.ee>:
> 
> Jul 20, 2019, 11:08 PM by michael@niedermayer.cc:
> 
>> Fixes: Timeout (22 -> 7 sec)
>> Fixes: 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
>> 
>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>> libavcodec/hqx.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
>> index bc24ba91d1..8639d77a41 100644
>> --- a/libavcodec/hqx.c
>> +++ b/libavcodec/hqx.c
>> @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void *data,
>> avctx->height              = ctx->height;
>> avctx->bits_per_raw_sample = 10;
>> 
>> +    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
>> +        (100 - avctx->discard_damaged_percentage) / 100 > 8LL * avpkt->size)
>> +        return AVERROR_INVALIDDATA;
>> + 
>> 
> 
> Not only are you ignoring my and others opinion, not only you still continue sending these awful patches,
> you've just submitted by far the worst one I've ever seen thinking its okay.
> Patches like these motivate developers to not even bother including test samples for new decoders, or even write them. Myself included. Doing exactly the opposite of what this system's meant to help.
> Sure, you sent this for review, but how can you even consider this utterly ridiculous hack for a problem that doesn't exist even worthy for review in the first place? Just what the fuck?

Ad hominem attacks sadly do not count as reviews.

Carl Eugen
Lynne July 20, 2019, 11:45 p.m.
Jul 21, 2019, 12:31 AM by ceffmpeg@gmail.com:

>
>
>> Am 21.07.2019 um 00:36 schrieb Lynne <dev@lynne.ee>:
>>
>> Jul 20, 2019, 11:08 PM by michael@niedermayer.cc:
>>
>>> Fixes: Timeout (22 -> 7 sec)
>>> Fixes: 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>> libavcodec/hqx.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
>>> index bc24ba91d1..8639d77a41 100644
>>> --- a/libavcodec/hqx.c
>>> +++ b/libavcodec/hqx.c
>>> @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void *data,
>>> avctx->height              = ctx->height;
>>> avctx->bits_per_raw_sample = 10;
>>>
>>> +    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
>>> +        (100 - avctx->discard_damaged_percentage) / 100 > 8LL * avpkt->size)
>>> +        return AVERROR_INVALIDDATA;
>>> + 
>>>
>>
>> Not only are you ignoring my and others opinion, not only you still continue sending these awful patches,
>> you've just submitted by far the worst one I've ever seen thinking its okay.
>> Patches like these motivate developers to not even bother including test samples for new decoders, or even write them. Myself included. Doing exactly the opposite of what this system's meant to help.
>> Sure, you sent this for review, but how can you even consider this utterly ridiculous hack for a problem that doesn't exist even worthy for review in the first place? Just what the fuck?
>>
>
> Ad hominem attacks sadly do not count as reviews.
>

Something like this doesn't deserve anything but the lowest level of criticism.
Can't even be called a patch, so it can't have a review in my opinion.
Carl Eugen Hoyos July 21, 2019, 1:11 a.m.
> Something like this doesn't deserve anything but the lowest level of criticism.

But the lowest level of criticism is not allowed here.

Carl Eugen
Paul B Mahol July 21, 2019, 8:48 a.m.
On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> Fixes: Timeout (22 -> 7 sec)
> Fixes:
> 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/hqx.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
> index bc24ba91d1..8639d77a41 100644
> --- a/libavcodec/hqx.c
> +++ b/libavcodec/hqx.c
> @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void
> *data,
>      avctx->height              = ctx->height;
>      avctx->bits_per_raw_sample = 10;
>
> +    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
> +        (100 - avctx->discard_damaged_percentage) / 100 > 8LL *
> avpkt->size)
> +        return AVERROR_INVALIDDATA;

Why just this change and not something better?

> +
>      switch (ctx->format) {
>      case HQX_422:
>          avctx->pix_fmt = AV_PIX_FMT_YUV422P16;
> --
> 2.22.0
>
> _______________________________________________
> 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 July 21, 2019, 10:38 a.m.
On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote:
> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > Fixes: Timeout (22 -> 7 sec)
> > Fixes:
> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
> >
> > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/hqx.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
> > index bc24ba91d1..8639d77a41 100644
> > --- a/libavcodec/hqx.c
> > +++ b/libavcodec/hqx.c
> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void
> > *data,
> >      avctx->height              = ctx->height;
> >      avctx->bits_per_raw_sample = 10;
> >
> > +    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
> > +        (100 - avctx->discard_damaged_percentage) / 100 > 8LL *
> > avpkt->size)
> > +        return AVERROR_INVALIDDATA;
> 
> Why just this change and not something better?

What would you prefer exactly ?

Thanks

[...]
Reimar Döffinger July 21, 2019, 11:49 a.m.
On 21.07.2019, at 00:36, Lynne <dev@lynne.ee> wrote:

> Jul 20, 2019, 11:08 PM by michael@niedermayer.cc:
> 
>> Fixes: Timeout (22 -> 7 sec)
>> Fixes: 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
>> 
>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>> libavcodec/hqx.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
>> index bc24ba91d1..8639d77a41 100644
>> --- a/libavcodec/hqx.c
>> +++ b/libavcodec/hqx.c
>> @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void *data,
>> avctx->height              = ctx->height;
>> avctx->bits_per_raw_sample = 10;
>> 
>> +    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
>> +        (100 - avctx->discard_damaged_percentage) / 100 > 8LL * avpkt->size)
>> +        return AVERROR_INVALIDDATA;
>> + 
>> 
> 
> Not only are you ignoring my and others opinion, not only you still continue sending these awful patches,
> you've just submitted by far the worst one I've ever seen thinking its okay.
> Patches like these motivate developers to not even bother including test samples for new decoders, or even write them. Myself included. Doing exactly the opposite of what this system's meant to help.
> Sure, you sent this for review, but how can you even consider this utterly ridiculous hack for a problem that doesn't exist even worthy for review in the first place? Just what the fuck?

I kind of understand your point of view, and the fuzzer complaining should not be an excuse to skip writing a commit message with some motivation for example, but I think you are a bit over the top.
There is already a discard_damaged_percentage and there is a point that maybe if a packet is obviously too broken to discard it with minimal overhead.
Those do not seem like utterly ridiculous ideas as your reply makes them out to me.
Nor is the idea of having some hardening against all too easy DoS attacks in some use-cases.
Also some value in just having the fuzzer run efficiently (it also discovers quite some real issues).
I agree it's hackish, I don't know the code enough to know it's correct, it needs to be properly documented (Michael, please, if you add such non-obvious, and especially heuristic checks, there needs to be some comment telling people the idea behind it and how to easily verify its correctness etc.).
With that in mind, can you maybe see why I do think that discussing such patch proposals does have merit?
Can we maybe come up with some compromise without being mad at each other?
Maybe some of these likely to be more controversial could be submitted as RFC instead of patch first?

Best regards,
Reimar
Paul B Mahol July 22, 2019, 6:20 a.m.
On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote:
>> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > Fixes: Timeout (22 -> 7 sec)
>> > Fixes:
>> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
>> >
>> > Found-by: continuous fuzzing process
>> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> > ---
>> >  libavcodec/hqx.c | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
>> > index bc24ba91d1..8639d77a41 100644
>> > --- a/libavcodec/hqx.c
>> > +++ b/libavcodec/hqx.c
>> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx,
>> > void
>> > *data,
>> >      avctx->height              = ctx->height;
>> >      avctx->bits_per_raw_sample = 10;
>> >
>> > +    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
>> > +        (100 - avctx->discard_damaged_percentage) / 100 > 8LL *
>> > avpkt->size)
>> > +        return AVERROR_INVALIDDATA;
>>
>> Why just this change and not something better?
>
> What would you prefer exactly ?

Something that works with pure black video.

>
> Thanks
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Modern terrorism, a quick summary: Need oil, start war with country that
> has oil, kill hundread thousand in war. Let country fall into chaos,
> be surprised about raise of fundamantalists. Drop more bombs, kill more
> people, be surprised about them taking revenge and drop even more bombs
> and strip your own citizens of their rights and freedoms. to be continued
>
Michael Niedermayer July 22, 2019, 11:26 p.m.
On Mon, Jul 22, 2019 at 08:20:54AM +0200, Paul B Mahol wrote:
> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote:
> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> > Fixes: Timeout (22 -> 7 sec)
> >> > Fixes:
> >> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
> >> >
> >> > Found-by: continuous fuzzing process
> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> > ---
> >> >  libavcodec/hqx.c | 4 ++++
> >> >  1 file changed, 4 insertions(+)
> >> >
> >> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
> >> > index bc24ba91d1..8639d77a41 100644
> >> > --- a/libavcodec/hqx.c
> >> > +++ b/libavcodec/hqx.c
> >> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx,
> >> > void
> >> > *data,
> >> >      avctx->height              = ctx->height;
> >> >      avctx->bits_per_raw_sample = 10;
> >> >
> >> > +    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
> >> > +        (100 - avctx->discard_damaged_percentage) / 100 > 8LL *
> >> > avpkt->size)
> >> > +        return AVERROR_INVALIDDATA;
> >>
> >> Why just this change and not something better?
> >
> > What would you prefer exactly ?
> 
> Something that works with pure black video.

Can you share the failing video file ?
I thought theres a minimum size of 1 vlc code (2 bit seem the smallest)
per 16x16 block. But quite possibly i might have missed something

Thanks


[...]
Paul B Mahol July 23, 2019, 7:03 a.m.
On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Mon, Jul 22, 2019 at 08:20:54AM +0200, Paul B Mahol wrote:
>> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote:
>> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> >> > Fixes: Timeout (22 -> 7 sec)
>> >> > Fixes:
>> >> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
>> >> >
>> >> > Found-by: continuous fuzzing process
>> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> >> > ---
>> >> >  libavcodec/hqx.c | 4 ++++
>> >> >  1 file changed, 4 insertions(+)
>> >> >
>> >> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
>> >> > index bc24ba91d1..8639d77a41 100644
>> >> > --- a/libavcodec/hqx.c
>> >> > +++ b/libavcodec/hqx.c
>> >> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext
>> >> > *avctx,
>> >> > void
>> >> > *data,
>> >> >      avctx->height              = ctx->height;
>> >> >      avctx->bits_per_raw_sample = 10;
>> >> >
>> >> > +    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
>> >> > +        (100 - avctx->discard_damaged_percentage) / 100 > 8LL *
>> >> > avpkt->size)
>> >> > +        return AVERROR_INVALIDDATA;
>> >>
>> >> Why just this change and not something better?
>> >
>> > What would you prefer exactly ?
>>
>> Something that works with pure black video.
>
> Can you share the failing video file ?
> I thought theres a minimum size of 1 vlc code (2 bit seem the smallest)
> per 16x16 block. But quite possibly i might have missed something
>

This is very disappointing. There is no freely available encoder for HQX.
And the one who commits stuff need to make sure it does not introduce
regressions.


> Thanks
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The educated differ from the uneducated as much as the living from the
> dead. -- Aristotle
>
Michael Niedermayer July 23, 2019, 7:45 a.m.
On Tue, Jul 23, 2019 at 09:03:32AM +0200, Paul B Mahol wrote:
> On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Mon, Jul 22, 2019 at 08:20:54AM +0200, Paul B Mahol wrote:
> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote:
> >> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> >> > Fixes: Timeout (22 -> 7 sec)
> >> >> > Fixes:
> >> >> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
> >> >> >
> >> >> > Found-by: continuous fuzzing process
> >> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> >> > ---
> >> >> >  libavcodec/hqx.c | 4 ++++
> >> >> >  1 file changed, 4 insertions(+)
> >> >> >
> >> >> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
> >> >> > index bc24ba91d1..8639d77a41 100644
> >> >> > --- a/libavcodec/hqx.c
> >> >> > +++ b/libavcodec/hqx.c
> >> >> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext
> >> >> > *avctx,
> >> >> > void
> >> >> > *data,
> >> >> >      avctx->height              = ctx->height;
> >> >> >      avctx->bits_per_raw_sample = 10;
> >> >> >
> >> >> > +    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
> >> >> > +        (100 - avctx->discard_damaged_percentage) / 100 > 8LL *
> >> >> > avpkt->size)
> >> >> > +        return AVERROR_INVALIDDATA;
> >> >>
> >> >> Why just this change and not something better?
> >> >
> >> > What would you prefer exactly ?
> >>
> >> Something that works with pure black video.
> >
> > Can you share the failing video file ?
> > I thought theres a minimum size of 1 vlc code (2 bit seem the smallest)
> > per 16x16 block. But quite possibly i might have missed something
> >
> 
> This is very disappointing. There is no freely available encoder for HQX.
> And the one who commits stuff need to make sure it does not introduce
> regressions.

The reviewer just has to explain how the problem he speaks of can
occur.

If we look at decode_slice_thread() each slice reads data from a distinct
input area (this is checked to be distinct in the code). So even the
same black slice cannot reuse the same representation.

decode_slice_thread calls decode_slice which calls decode_func
decode_func can be one of 4 implementations each reading at least
a minimum of 2 bit. so we have a minimum of 2 bits per macroblock
the calls happen at a granularity of 16x16 so theres a minimum of
2 bits per 16x16 block.
Its very possible that i have missed some path or case in this
analysis. But we wont really move forward based on riddles.
If you know of a path/case where a frame can be encoded with
less input data just explain that case and ill adjust the
patch accordingly. Otherwise the patch is stuck as i dont
know what case you speak of

PS: also there are no other return pathes in hqx_decode_frame()
all returns are error pathes so i dont see any shortcuts for
black frames which would bypass the minimum size

Thanks

[...]
Paul B Mahol July 23, 2019, 7:52 a.m.
On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Tue, Jul 23, 2019 at 09:03:32AM +0200, Paul B Mahol wrote:
>> On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > On Mon, Jul 22, 2019 at 08:20:54AM +0200, Paul B Mahol wrote:
>> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> >> > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote:
>> >> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> >> >> > Fixes: Timeout (22 -> 7 sec)
>> >> >> > Fixes:
>> >> >> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
>> >> >> >
>> >> >> > Found-by: continuous fuzzing process
>> >> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> >> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> >> >> > ---
>> >> >> >  libavcodec/hqx.c | 4 ++++
>> >> >> >  1 file changed, 4 insertions(+)
>> >> >> >
>> >> >> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
>> >> >> > index bc24ba91d1..8639d77a41 100644
>> >> >> > --- a/libavcodec/hqx.c
>> >> >> > +++ b/libavcodec/hqx.c
>> >> >> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext
>> >> >> > *avctx,
>> >> >> > void
>> >> >> > *data,
>> >> >> >      avctx->height              = ctx->height;
>> >> >> >      avctx->bits_per_raw_sample = 10;
>> >> >> >
>> >> >> > +    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
>> >> >> > +        (100 - avctx->discard_damaged_percentage) / 100 > 8LL *
>> >> >> > avpkt->size)
>> >> >> > +        return AVERROR_INVALIDDATA;
>> >> >>
>> >> >> Why just this change and not something better?
>> >> >
>> >> > What would you prefer exactly ?
>> >>
>> >> Something that works with pure black video.
>> >
>> > Can you share the failing video file ?
>> > I thought theres a minimum size of 1 vlc code (2 bit seem the smallest)
>> > per 16x16 block. But quite possibly i might have missed something
>> >
>>
>> This is very disappointing. There is no freely available encoder for HQX.
>> And the one who commits stuff need to make sure it does not introduce
>> regressions.
>
> The reviewer just has to explain how the problem he speaks of can
> occur.

No, its other way around.
The patch author just has to explain how the problem he tries to solve
is valid solution by given patch.

>
> If we look at decode_slice_thread() each slice reads data from a distinct
> input area (this is checked to be distinct in the code). So even the
> same black slice cannot reuse the same representation.
>
> decode_slice_thread calls decode_slice which calls decode_func
> decode_func can be one of 4 implementations each reading at least
> a minimum of 2 bit. so we have a minimum of 2 bits per macroblock
> the calls happen at a granularity of 16x16 so theres a minimum of
> 2 bits per 16x16 block.
> Its very possible that i have missed some path or case in this
> analysis. But we wont really move forward based on riddles.
> If you know of a path/case where a frame can be encoded with
> less input data just explain that case and ill adjust the
> patch accordingly. Otherwise the patch is stuck as i dont
> know what case you speak of

For start, get a copy of HQX encoder, than we can talk more.

>
> PS: also there are no other return pathes in hqx_decode_frame()
> all returns are error pathes so i dont see any shortcuts for
> black frames which would bypass the minimum size
>
> Thanks
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Never trust a computer, one day, it may think you are the virus. -- Compn
>
Paul B Mahol July 23, 2019, 8:11 a.m.
On 7/23/19, Paul B Mahol <onemda@gmail.com> wrote:
> On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> On Tue, Jul 23, 2019 at 09:03:32AM +0200, Paul B Mahol wrote:
>>> On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>> > On Mon, Jul 22, 2019 at 08:20:54AM +0200, Paul B Mahol wrote:
>>> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>> >> > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote:
>>> >> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>> >> >> > Fixes: Timeout (22 -> 7 sec)
>>> >> >> > Fixes:
>>> >> >> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
>>> >> >> >
>>> >> >> > Found-by: continuous fuzzing process
>>> >> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> >> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> >> >> > ---
>>> >> >> >  libavcodec/hqx.c | 4 ++++
>>> >> >> >  1 file changed, 4 insertions(+)
>>> >> >> >
>>> >> >> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
>>> >> >> > index bc24ba91d1..8639d77a41 100644
>>> >> >> > --- a/libavcodec/hqx.c
>>> >> >> > +++ b/libavcodec/hqx.c
>>> >> >> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext
>>> >> >> > *avctx,
>>> >> >> > void
>>> >> >> > *data,
>>> >> >> >      avctx->height              = ctx->height;
>>> >> >> >      avctx->bits_per_raw_sample = 10;
>>> >> >> >
>>> >> >> > +    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
>>> >> >> > +        (100 - avctx->discard_damaged_percentage) / 100 > 8LL *
>>> >> >> > avpkt->size)
>>> >> >> > +        return AVERROR_INVALIDDATA;
>>> >> >>
>>> >> >> Why just this change and not something better?
>>> >> >
>>> >> > What would you prefer exactly ?
>>> >>
>>> >> Something that works with pure black video.
>>> >
>>> > Can you share the failing video file ?
>>> > I thought theres a minimum size of 1 vlc code (2 bit seem the
>>> > smallest)
>>> > per 16x16 block. But quite possibly i might have missed something
>>> >
>>>
>>> This is very disappointing. There is no freely available encoder for
>>> HQX.
>>> And the one who commits stuff need to make sure it does not introduce
>>> regressions.
>>
>> The reviewer just has to explain how the problem he speaks of can
>> occur.
>
> No, its other way around.
> The patch author just has to explain how the problem he tries to solve
> is valid solution by given patch.
>
>>
>> If we look at decode_slice_thread() each slice reads data from a distinct
>> input area (this is checked to be distinct in the code). So even the
>> same black slice cannot reuse the same representation.
>>
>> decode_slice_thread calls decode_slice which calls decode_func
>> decode_func can be one of 4 implementations each reading at least
>> a minimum of 2 bit. so we have a minimum of 2 bits per macroblock
>> the calls happen at a granularity of 16x16 so theres a minimum of
>> 2 bits per 16x16 block.
>> Its very possible that i have missed some path or case in this
>> analysis. But we wont really move forward based on riddles.
>> If you know of a path/case where a frame can be encoded with
>> less input data just explain that case and ill adjust the
>> patch accordingly. Otherwise the patch is stuck as i dont
>> know what case you speak of
>
> For start, get a copy of HQX encoder, than we can talk more.

Here, I was not lazy so I got one sample for you:

http://0x0.st/zppS.avi

>
>>
>> PS: also there are no other return pathes in hqx_decode_frame()
>> all returns are error pathes so i dont see any shortcuts for
>> black frames which would bypass the minimum size
>>
>> Thanks
>>
>> [...]
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> Never trust a computer, one day, it may think you are the virus. -- Compn
>>
>
Michael Niedermayer July 23, 2019, 12:53 p.m.
On Tue, Jul 23, 2019 at 09:52:59AM +0200, Paul B Mahol wrote:
> On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Tue, Jul 23, 2019 at 09:03:32AM +0200, Paul B Mahol wrote:
> >> On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> > On Mon, Jul 22, 2019 at 08:20:54AM +0200, Paul B Mahol wrote:
> >> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> >> > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote:
> >> >> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> >> >> > Fixes: Timeout (22 -> 7 sec)
> >> >> >> > Fixes:
> >> >> >> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992
> >> >> >> >
> >> >> >> > Found-by: continuous fuzzing process
> >> >> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >> >> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> >> >> > ---
> >> >> >> >  libavcodec/hqx.c | 4 ++++
> >> >> >> >  1 file changed, 4 insertions(+)
> >> >> >> >
> >> >> >> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
> >> >> >> > index bc24ba91d1..8639d77a41 100644
> >> >> >> > --- a/libavcodec/hqx.c
> >> >> >> > +++ b/libavcodec/hqx.c
> >> >> >> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext
> >> >> >> > *avctx,
> >> >> >> > void
> >> >> >> > *data,
> >> >> >> >      avctx->height              = ctx->height;
> >> >> >> >      avctx->bits_per_raw_sample = 10;
> >> >> >> >
> >> >> >> > +    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
> >> >> >> > +        (100 - avctx->discard_damaged_percentage) / 100 > 8LL *
> >> >> >> > avpkt->size)
> >> >> >> > +        return AVERROR_INVALIDDATA;
> >> >> >>
> >> >> >> Why just this change and not something better?
> >> >> >
> >> >> > What would you prefer exactly ?
> >> >>
> >> >> Something that works with pure black video.
> >> >
> >> > Can you share the failing video file ?
> >> > I thought theres a minimum size of 1 vlc code (2 bit seem the smallest)
> >> > per 16x16 block. But quite possibly i might have missed something
> >> >
> >>
> >> This is very disappointing. There is no freely available encoder for HQX.
> >> And the one who commits stuff need to make sure it does not introduce
> >> regressions.
> >
> > The reviewer just has to explain how the problem he speaks of can
> > occur.
> 
> No, its other way around.
> The patch author just has to explain how the problem he tries to solve
> is valid solution by given patch.

I have explained that in the very mail you just replied to.

anyway, no problem ill find a way to encode a pure black hqx video
or will otherwise do a more complete proof of correctness.
(or of course if an issue is found by doing so improve the patch)

I just thought if you know of a issue and told me what it is exactly
that would be much simpler and quicker.

Thanks


[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c
index bc24ba91d1..8639d77a41 100644
--- a/libavcodec/hqx.c
+++ b/libavcodec/hqx.c
@@ -471,6 +471,10 @@  static int hqx_decode_frame(AVCodecContext *avctx, void *data,
     avctx->height              = ctx->height;
     avctx->bits_per_raw_sample = 10;
 
+    if (avctx->coded_width / 16 * (avctx->coded_height / 16) *
+        (100 - avctx->discard_damaged_percentage) / 100 > 8LL * avpkt->size)
+        return AVERROR_INVALIDDATA;
+
     switch (ctx->format) {
     case HQX_422:
         avctx->pix_fmt = AV_PIX_FMT_YUV422P16;