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

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

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

---
 tests/checkasm/h264dsp.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
index f5f9650224..006532e08b 100644
--- a/tests/checkasm/h264dsp.c
+++ b/tests/checkasm/h264dsp.c
@@ -328,7 +328,7 @@ 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]);
+    LOCAL_ALIGNED_16(int16_t, src, [16 * 2]);
     /* Ensure dst buffers are large enough to hold dctcoefs of all bit-depths. */
     LOCAL_ALIGNED_16(uint8_t, dst0, [16 * 16 * sizeof(int32_t)]);
     LOCAL_ALIGNED_16(uint8_t, dst1, [16 * 16 * sizeof(int32_t)]);
@@ -338,15 +338,21 @@ static void check_idct_dequant(void)
     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 (int j = 0; j < 16; j++)
+                src[j] = (rnd() % 512) - 256;
+        } else {
+            int32_t *p = (int32_t *)src;
+            for (int j = 0; j < 16; j++)
+                p[j] = (rnd() % (1 << (bit_depth + 1))) - (1 << bit_depth);
+        }
+
         memset(dst0, 0, 16 * 16 * SIZEOF_COEF);
         memset(dst1, 0, 16 * 16 * SIZEOF_COEF);
 
-- 
2.25.1

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

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

[Martin Storsjö]

On Mon, 16 Jun 2025, Zhao Zhili wrote:

> From: Zhao Zhili <zhilizhao@tencent.com>
>
> ---
> tests/checkasm/h264dsp.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
> index f5f9650224..006532e08b 100644
> --- a/tests/checkasm/h264dsp.c
> +++ b/tests/checkasm/h264dsp.c
> @@ -328,7 +328,7 @@ 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]);
> +    LOCAL_ALIGNED_16(int16_t, src, [16 * 2]);
>     /* Ensure dst buffers are large enough to hold dctcoefs of all bit-depths. */
>     LOCAL_ALIGNED_16(uint8_t, dst0, [16 * 16 * sizeof(int32_t)]);

Thanks for catching this! If the src buffer also is either int16_t or 
int32_t, can we make it uint8_t and use sizeof(int32_t) there as well just 
like dst, and move (and update) the comment above the src buffer too?

// Martin

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

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

[Andreas Rheinhardt]

Zhao Zhili:
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> ---
>  tests/checkasm/h264dsp.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
> index f5f9650224..006532e08b 100644
> --- a/tests/checkasm/h264dsp.c
> +++ b/tests/checkasm/h264dsp.c
> @@ -328,7 +328,7 @@ 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]);
> +    LOCAL_ALIGNED_16(int16_t, src, [16 * 2]);
>      /* Ensure dst buffers are large enough to hold dctcoefs of all bit-depths. */
>      LOCAL_ALIGNED_16(uint8_t, dst0, [16 * 16 * sizeof(int32_t)]);
>      LOCAL_ALIGNED_16(uint8_t, dst1, [16 * 16 * sizeof(int32_t)]);
> @@ -338,15 +338,21 @@ static void check_idct_dequant(void)
>      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 (int j = 0; j < 16; j++)
> +                src[j] = (rnd() % 512) - 256;
> +        } else {
> +            int32_t *p = (int32_t *)src;
> +            for (int j = 0; j < 16; j++)
> +                p[j] = (rnd() % (1 << (bit_depth + 1))) - (1 << bit_depth);

This is an effective type violation and therefore UB. Furthermore,
increasing the size of the array has the downside that stack overflows
in the 8 bit codepath may go undetected. So better add a
LOCAL_ALIGNED_16(int32_t, src32, [16]) and use that for the >8 bit tests.

> +        }
> +
>          memset(dst0, 0, 16 * 16 * SIZEOF_COEF);
>          memset(dst1, 0, 16 * 16 * SIZEOF_COEF);
>  

_______________________________________________
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] [PATCH v2] checkasm/h264dsp: Fix stack overflow in check_idct_dequant

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

---
 tests/checkasm/h264dsp.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
index f5f9650224..ca4a44ee3b 100644
--- a/tests/checkasm/h264dsp.c
+++ b/tests/checkasm/h264dsp.c
@@ -328,25 +328,32 @@ 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 (int j = 0; j < 16; j++)
+                src[j] = (rnd() % 512) - 256;
+        } else {
+            int32_t *p = (int32_t *)src_buf;
+            for (int j = 0; j < 16; j++)
+                p[j] = (rnd() % (1 << (bit_depth + 1))) - (1 << bit_depth);
+        }
+
         memset(dst0, 0, 16 * 16 * SIZEOF_COEF);
         memset(dst1, 0, 16 * 16 * SIZEOF_COEF);
 
-- 
2.25.1

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

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

[Zhao Zhili]

> On Jun 16, 2025, at 15:16, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 
> Zhao Zhili:
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> 
>> ---
>> tests/checkasm/h264dsp.c | 14 ++++++++++----
>> 1 file changed, 10 insertions(+), 4 deletions(-)
>> 
>> diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
>> index f5f9650224..006532e08b 100644
>> --- a/tests/checkasm/h264dsp.c
>> +++ b/tests/checkasm/h264dsp.c
>> @@ -328,7 +328,7 @@ 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]);
>> +    LOCAL_ALIGNED_16(int16_t, src, [16 * 2]);
>>     /* Ensure dst buffers are large enough to hold dctcoefs of all bit-depths. */
>>     LOCAL_ALIGNED_16(uint8_t, dst0, [16 * 16 * sizeof(int32_t)]);
>>     LOCAL_ALIGNED_16(uint8_t, dst1, [16 * 16 * sizeof(int32_t)]);
>> @@ -338,15 +338,21 @@ static void check_idct_dequant(void)
>>     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 (int j = 0; j < 16; j++)
>> +                src[j] = (rnd() % 512) - 256;
>> +        } else {
>> +            int32_t *p = (int32_t *)src;
>> +            for (int j = 0; j < 16; j++)
>> +                p[j] = (rnd() % (1 << (bit_depth + 1))) - (1 << bit_depth);
> 
> This is an effective type violation and therefore UB.

Yes. And the template functions are UB.

> Furthermore,
> increasing the size of the array has the downside that stack overflows
> in the 8 bit codepath may go undetected. So better add a
> LOCAL_ALIGNED_16(int32_t, src32, [16]) and use that for the >8 bit tests.

I think this is still UB by pass it as argument to h264_luma_dc_dequant_idct,
due to the function prototype.

I have no idea other than union or separate test case.

> 
>> +        }
>> +
>>         memset(dst0, 0, 16 * 16 * SIZEOF_COEF);
>>         memset(dst1, 0, 16 * 16 * SIZEOF_COEF);
>> 
> 
> _______________________________________________
> 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".

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

[Andreas Rheinhardt]

Zhao Zhili:
> 
> 
>> On Jun 16, 2025, at 15:16, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
>>
>> Zhao Zhili:
>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>>
>>> ---
>>> tests/checkasm/h264dsp.c | 14 ++++++++++----
>>> 1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
>>> index f5f9650224..006532e08b 100644
>>> --- a/tests/checkasm/h264dsp.c
>>> +++ b/tests/checkasm/h264dsp.c
>>> @@ -328,7 +328,7 @@ 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]);
>>> +    LOCAL_ALIGNED_16(int16_t, src, [16 * 2]);
>>>     /* Ensure dst buffers are large enough to hold dctcoefs of all bit-depths. */
>>>     LOCAL_ALIGNED_16(uint8_t, dst0, [16 * 16 * sizeof(int32_t)]);
>>>     LOCAL_ALIGNED_16(uint8_t, dst1, [16 * 16 * sizeof(int32_t)]);
>>> @@ -338,15 +338,21 @@ static void check_idct_dequant(void)
>>>     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 (int j = 0; j < 16; j++)
>>> +                src[j] = (rnd() % 512) - 256;
>>> +        } else {
>>> +            int32_t *p = (int32_t *)src;
>>> +            for (int j = 0; j < 16; j++)
>>> +                p[j] = (rnd() % (1 << (bit_depth + 1))) - (1 << bit_depth);
>>
>> This is an effective type violation and therefore UB.
> 
> Yes. And the template functions are UB.

Only if the 32bit values read in the >8 bit depth case have actually
been written as int16_t or vice versa.

> 
>> Furthermore,
>> increasing the size of the array has the downside that stack overflows
>> in the 8 bit codepath may go undetected. So better add a
>> LOCAL_ALIGNED_16(int32_t, src32, [16]) and use that for the >8 bit tests.
> 
> I think this is still UB by pass it as argument to h264_luma_dc_dequant_idct,
> due to the function prototype.
> 
> I have no idea other than union or separate test case.
> 

Converting a pointer to a different pointer type, passing that to a
function which converts it back to the original type and uses it is
allowed (as long as the pointee is suitably aligned for all the
pointed-to types; otherwise it is UB*). The effective type rules only
care about the types of the accesses, not on the chain of pointer
conversions that the pointer used for the access went through.

Anyway, C has a generic pointer: void*.

- Andreas

*: My guess is that this limitation stems from the fact that the
underlying object representation of different pointer types need not be
the same (i.e. converting a pointer need not be a no-op; IIRC there used
to be some long-obsolete systems where this is so). Anyway, imagine a
type T with alignment four; then it would be legal for pointers to this
type to have this backed in the sense that converting a char* to T*
would shift right by two bits. Converting back would then of course
shift left and this only works when the char* is suitably aligned.

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