Re: [FFmpeg-devel] [PATCH v3] checkasm/h264dsp: Fix stack overflow in check_idct_dequant

[Zhao Zhili <quinkblack-at-foxmail.com@ffmpeg.org>]

> On Jun 16, 2025, at 17:46, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 
> Zhao Zhili:
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> 
>> ---
>> tests/checkasm/h264dsp.c | 20 +++++++++++++++-----
>> 1 file changed, 15 insertions(+), 5 deletions(-)
>> 
>> diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
>> index f5f9650224..a0f8fd858a 100644
>> --- a/tests/checkasm/h264dsp.c
>> +++ b/tests/checkasm/h264dsp.c
>> @@ -328,25 +328,35 @@ static void check_idct_multiple(void)
>> static void check_idct_dequant(void)
>> {
>>     static const int depths[5] = { 8, 9, 10, 12, 14 };
>> -    LOCAL_ALIGNED_16(int16_t, src, [16]);
>> -    /* Ensure dst buffers are large enough to hold dctcoefs of all bit-depths. */
>> +    /* Ensure buffers are large enough to hold dctcoefs of all bit-depths. */
>> +    LOCAL_ALIGNED_16(uint8_t, src_buf, [16 * sizeof(int32_t)]);
>>     LOCAL_ALIGNED_16(uint8_t, dst0, [16 * 16 * sizeof(int32_t)]);
>>     LOCAL_ALIGNED_16(uint8_t, dst1, [16 * 16 * sizeof(int32_t)]);
>> +    int16_t *src = (int16_t *)src_buf;
>>     int16_t *dst_ref = (int16_t *)dst0;
>>     int16_t *dst_new = (int16_t *)dst1;
>>     H264DSPContext h;
>>     int bit_depth, i, qmul;
>>     declare_func_emms(AV_CPU_FLAG_MMX | AV_CPU_FLAG_SSE2, void, int16_t *output, int16_t *input, int qmul);
>> 
>> -    for (int j = 0; j < 16; j++)
>> -        src[j] = (rnd() % 512) - 256;
>> -
>>     qmul = rnd() % 4096;
>> 
>>     for (i = 0; i < FF_ARRAY_ELEMS(depths); i++) {
>>         bit_depth = depths[i];
>>         ff_h264dsp_init(&h, bit_depth, 1);
>> 
>> +        if (bit_depth == 8) {
>> +            for (size_t j = 0; j < 16; j++) {
>> +                int16_t r = (rnd() % 512) - 256;
>> +                AV_WN16A(&src_buf[j << 1], r);
>> +            }
>> +        } else {
>> +            for (size_t j = 0; j < 16; j++) {
>> +                int32_t r = (rnd() % (1 << (bit_depth + 1))) - (1 << bit_depth);
>> +                AV_WN32A(&src_buf[j << 2], r);
>> +            }
>> +        }
>> +
>>         memset(dst0, 0, 16 * 16 * SIZEOF_COEF);
>>         memset(dst1, 0, 16 * 16 * SIZEOF_COEF);
>> 
> 
> This still has an effective-type violation: src_buf is of type uint8_t,
> yet the ff_h264_luma_dc_dequant_idct functions will read it as
> int16_t/int32_t. It also still has the downside that buffer overflows
> for the 8bit case can go undetected.

A bunch of template has cast like 

    pixel *dst = (pixel *)_dst;
    const pixel *src = (const pixel *)_src;

then read and write as int16_t.

And a bunch of checkasm use uint8_t[] array on stack as src and dst,
which leading to UB.

This patch isn’t specific. And this patch add zero UB (it’s there before the patch,
both src and dst are accessed as int32_t/int16_t while they are int16_t and uint8_t).

> 
> - Andreas
> 
> _______________________________________________
> 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".

_______________________________________________
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".