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

[Paul B Mahol]

On 7/23/19, Paul B Mahol <onemda at gmail.com> wrote:
> On 7/23/19, Michael Niedermayer <michael at niedermayer.cc> wrote:
>> On Tue, Jul 23, 2019 at 09:03:32AM +0200, Paul B Mahol wrote:
>>> On 7/23/19, Michael Niedermayer <michael at niedermayer.cc> wrote:
>>> > On Mon, Jul 22, 2019 at 08:20:54AM +0200, Paul B Mahol wrote:
>>> >> On 7/21/19, Michael Niedermayer <michael at niedermayer.cc> wrote:
>>> >> > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote:
>>> >> >> On 7/21/19, Michael Niedermayer <michael at 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 at 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
>>
>