diff mbox series

[FFmpeg-devel] checkasm/hevc_pel: fix stack-buffer-overflow

Message ID tencent_35479C24E0C45B76344B81E234533FCD5609@qq.com
State New
Headers show
Series [FFmpeg-devel] checkasm/hevc_pel: fix stack-buffer-overflow
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

zhilizhao(赵志立) Sept. 21, 2021, 9:14 a.m. UTC
==225880==ERROR: AddressSanitizer: stack-buffer-overflow on address ...
READ of size 2 at 0x7fffe49ab400 thread T0
    #0 0x18301da in put_hevc_qpel_hv_9 src/libavcodec/hevcdsp_template.c:666
    #1 0x6c6bc4 in checkasm_check_hevc_qpel src/tests/checkasm/hevc_pel.c:97
    #2 0x6cecc8 in checkasm_check_hevc_pel src/tests/checkasm/hevc_pel.c:528
---
 tests/checkasm/hevc_pel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Martin Storsjö Sept. 21, 2021, 11:09 a.m. UTC | #1
On Tue, 21 Sep 2021, Zhao Zhili wrote:

> ==225880==ERROR: AddressSanitizer: stack-buffer-overflow on address ...
> READ of size 2 at 0x7fffe49ab400 thread T0
>    #0 0x18301da in put_hevc_qpel_hv_9 src/libavcodec/hevcdsp_template.c:666
>    #1 0x6c6bc4 in checkasm_check_hevc_qpel src/tests/checkasm/hevc_pel.c:97
>    #2 0x6cecc8 in checkasm_check_hevc_pel src/tests/checkasm/hevc_pel.c:528
> ---
> tests/checkasm/hevc_pel.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/checkasm/hevc_pel.c b/tests/checkasm/hevc_pel.c
> index ec24309081..3dc7cd9090 100644
> --- a/tests/checkasm/hevc_pel.c
> +++ b/tests/checkasm/hevc_pel.c
> @@ -34,7 +34,7 @@ static const int denoms[] = {0, 7, 12, -1 };
> static const int offsets[] = {0, 255, -1 };
>
> #define SIZEOF_PIXEL ((bit_depth + 7) / 8)
> -#define BUF_SIZE (2 * MAX_PB_SIZE * (2 * 4 + MAX_PB_SIZE))
> +#define BUF_SIZE (2 * MAX_PB_SIZE * (2 * 4 + MAX_PB_SIZE) + 8)
>
> #define randomize_buffers()                          \
>     do {                                             \
> -- 
> 2.31.1

Probably ok (I haven't studied the issue, but this seems plausibly 
correct).

// Martin
Andreas Rheinhardt Sept. 21, 2021, 2:40 p.m. UTC | #2
Martin Storsjö:
> On Tue, 21 Sep 2021, Zhao Zhili wrote:
> 
>> ==225880==ERROR: AddressSanitizer: stack-buffer-overflow on address ...
>> READ of size 2 at 0x7fffe49ab400 thread T0
>>    #0 0x18301da in put_hevc_qpel_hv_9
>> src/libavcodec/hevcdsp_template.c:666
>>    #1 0x6c6bc4 in checkasm_check_hevc_qpel
>> src/tests/checkasm/hevc_pel.c:97
>>    #2 0x6cecc8 in checkasm_check_hevc_pel
>> src/tests/checkasm/hevc_pel.c:528
>> ---
>> tests/checkasm/hevc_pel.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/checkasm/hevc_pel.c b/tests/checkasm/hevc_pel.c
>> index ec24309081..3dc7cd9090 100644
>> --- a/tests/checkasm/hevc_pel.c
>> +++ b/tests/checkasm/hevc_pel.c
>> @@ -34,7 +34,7 @@ static const int denoms[] = {0, 7, 12, -1 };
>> static const int offsets[] = {0, 255, -1 };
>>
>> #define SIZEOF_PIXEL ((bit_depth + 7) / 8)
>> -#define BUF_SIZE (2 * MAX_PB_SIZE * (2 * 4 + MAX_PB_SIZE))
>> +#define BUF_SIZE (2 * MAX_PB_SIZE * (2 * 4 + MAX_PB_SIZE) + 8)
>>
>> #define randomize_buffers()                          \
>>     do {                                             \
>> -- 
>> 2.31.1
> 
> Probably ok (I haven't studied the issue, but this seems plausibly
> correct).
> 

I have also found this issue quite a while ago and am using this here as
a workaround (it is the minimal set of changes that makes the test pass
for me):

diff --git a/tests/checkasm/hevc_pel.c b/tests/checkasm/hevc_pel.c

index ec24309081..7fb922c6d0 100644

--- a/tests/checkasm/hevc_pel.c

+++ b/tests/checkasm/hevc_pel.c

@@ -67,8 +67,8 @@ static const int offsets[] = {0, 255, -1 };



 static void checkasm_check_hevc_qpel(void)

 {

-    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);

-    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);

+    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE+8]);

+    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE+8]);

     LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);

     LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);



@@ -111,8 +111,8 @@ static void checkasm_check_hevc_qpel(void)



 static void checkasm_check_hevc_qpel_uni(void)

 {

-    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);

-    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);

+    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE+8]);

+    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE+8]);

     LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);

     LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);



@@ -152,8 +152,8 @@ static void checkasm_check_hevc_qpel_uni(void)



 static void checkasm_check_hevc_qpel_uni_w(void)

 {

-    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);

-    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);

+    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE+8]);

+    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE+8]);

     LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);

     LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);



@@ -200,8 +200,8 @@ static void checkasm_check_hevc_qpel_uni_w(void)



 static void checkasm_check_hevc_qpel_bi(void)

 {

-    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);

-    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);

+    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE+8]);

+    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE+8]);

     LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);

     LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);

     LOCAL_ALIGNED_32(int16_t, ref0, [BUF_SIZE]);

@@ -244,8 +244,8 @@ static void checkasm_check_hevc_qpel_bi(void)



 static void checkasm_check_hevc_qpel_bi_w(void)

 {

-    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);

-    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);

+    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE+8]);

+    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE+8]);

     LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);

     LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);

     LOCAL_ALIGNED_32(int16_t, ref0, [BUF_SIZE]);

But I have never ever investigated why these buffers and only these
buffers need to be enlarged and whether there is an underlying bug (i.e.
whether an access beyond the end of the buffer might happen in a
non-test scenario). Have you?

- Andreas
Andreas Rheinhardt Sept. 21, 2021, 3:18 p.m. UTC | #3
Andreas Rheinhardt:
> Martin Storsjö:
>> On Tue, 21 Sep 2021, Zhao Zhili wrote:
>>
>>> ==225880==ERROR: AddressSanitizer: stack-buffer-overflow on address ...
>>> READ of size 2 at 0x7fffe49ab400 thread T0
>>>    #0 0x18301da in put_hevc_qpel_hv_9
>>> src/libavcodec/hevcdsp_template.c:666
>>>    #1 0x6c6bc4 in checkasm_check_hevc_qpel
>>> src/tests/checkasm/hevc_pel.c:97
>>>    #2 0x6cecc8 in checkasm_check_hevc_pel
>>> src/tests/checkasm/hevc_pel.c:528
>>> ---
>>> tests/checkasm/hevc_pel.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/checkasm/hevc_pel.c b/tests/checkasm/hevc_pel.c
>>> index ec24309081..3dc7cd9090 100644
>>> --- a/tests/checkasm/hevc_pel.c
>>> +++ b/tests/checkasm/hevc_pel.c
>>> @@ -34,7 +34,7 @@ static const int denoms[] = {0, 7, 12, -1 };
>>> static const int offsets[] = {0, 255, -1 };
>>>
>>> #define SIZEOF_PIXEL ((bit_depth + 7) / 8)
>>> -#define BUF_SIZE (2 * MAX_PB_SIZE * (2 * 4 + MAX_PB_SIZE))
>>> +#define BUF_SIZE (2 * MAX_PB_SIZE * (2 * 4 + MAX_PB_SIZE) + 8)
>>>
>>> #define randomize_buffers()                          \
>>>     do {                                             \
>>> -- 
>>> 2.31.1
>>
>> Probably ok (I haven't studied the issue, but this seems plausibly
>> correct).
>>
> 
> I have also found this issue quite a while ago and am using this here as
> a workaround (it is the minimal set of changes that makes the test pass
> for me):
> 
> diff --git a/tests/checkasm/hevc_pel.c b/tests/checkasm/hevc_pel.c
> 
> index ec24309081..7fb922c6d0 100644
> 
> --- a/tests/checkasm/hevc_pel.c
> 
> +++ b/tests/checkasm/hevc_pel.c
> 
> @@ -67,8 +67,8 @@ static const int offsets[] = {0, 255, -1 };
> 
> 
> 
>  static void checkasm_check_hevc_qpel(void)
> 
>  {
> 
> -    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);
> 
> -    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);
> 
> +    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE+8]);
> 
> +    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE+8]);
> 
>      LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);
> 
>      LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);
> 
> 
> 
> @@ -111,8 +111,8 @@ static void checkasm_check_hevc_qpel(void)
> 
> 
> 
>  static void checkasm_check_hevc_qpel_uni(void)
> 
>  {
> 
> -    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);
> 
> -    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);
> 
> +    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE+8]);
> 
> +    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE+8]);
> 
>      LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);
> 
>      LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);
> 
> 
> 
> @@ -152,8 +152,8 @@ static void checkasm_check_hevc_qpel_uni(void)
> 
> 
> 
>  static void checkasm_check_hevc_qpel_uni_w(void)
> 
>  {
> 
> -    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);
> 
> -    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);
> 
> +    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE+8]);
> 
> +    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE+8]);
> 
>      LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);
> 
>      LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);
> 
> 
> 
> @@ -200,8 +200,8 @@ static void checkasm_check_hevc_qpel_uni_w(void)
> 
> 
> 
>  static void checkasm_check_hevc_qpel_bi(void)
> 
>  {
> 
> -    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);
> 
> -    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);
> 
> +    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE+8]);
> 
> +    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE+8]);
> 
>      LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);
> 
>      LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);
> 
>      LOCAL_ALIGNED_32(int16_t, ref0, [BUF_SIZE]);
> 
> @@ -244,8 +244,8 @@ static void checkasm_check_hevc_qpel_bi(void)
> 
> 
> 
>  static void checkasm_check_hevc_qpel_bi_w(void)
> 
>  {
> 
> -    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE]);
> 
> -    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE]);
> 
> +    LOCAL_ALIGNED_32(uint8_t, buf0, [BUF_SIZE+8]);
> 
> +    LOCAL_ALIGNED_32(uint8_t, buf1, [BUF_SIZE+8]);
> 
>      LOCAL_ALIGNED_32(uint8_t, dst0, [BUF_SIZE]);
> 
>      LOCAL_ALIGNED_32(uint8_t, dst1, [BUF_SIZE]);
> 
>      LOCAL_ALIGNED_32(int16_t, ref0, [BUF_SIZE]);
> 
> But I have never ever investigated why these buffers and only these
> buffers need to be enlarged and whether there is an underlying bug (i.e.
> whether an access beyond the end of the buffer might happen in a
> non-test scenario). Have you?
> 

This was intended as a reply (and a question) to Zhao Zhili. Sorry.

- Andreas
zhilizhao(赵志立) Sept. 21, 2021, 3:33 p.m. UTC | #4
> On Sep 21, 2021, at 10:40 PM, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 
> Martin Storsjö:
>> On Tue, 21 Sep 2021, Zhao Zhili wrote:
>> 
>>> ==225880==ERROR: AddressSanitizer: stack-buffer-overflow on address ...
>>> READ of size 2 at 0x7fffe49ab400 thread T0
>>>    #0 0x18301da in put_hevc_qpel_hv_9
>>> src/libavcodec/hevcdsp_template.c:666
>>>    #1 0x6c6bc4 in checkasm_check_hevc_qpel
>>> src/tests/checkasm/hevc_pel.c:97
>>>    #2 0x6cecc8 in checkasm_check_hevc_pel
>>> src/tests/checkasm/hevc_pel.c:528
>>> ---
>>> tests/checkasm/hevc_pel.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/tests/checkasm/hevc_pel.c b/tests/checkasm/hevc_pel.c
>>> index ec24309081..3dc7cd9090 100644
>>> --- a/tests/checkasm/hevc_pel.c
>>> +++ b/tests/checkasm/hevc_pel.c
>>> @@ -34,7 +34,7 @@ static const int denoms[] = {0, 7, 12, -1 };
>>> static const int offsets[] = {0, 255, -1 };
>>> 
>>> #define SIZEOF_PIXEL ((bit_depth + 7) / 8)
>>> -#define BUF_SIZE (2 * MAX_PB_SIZE * (2 * 4 + MAX_PB_SIZE))
>>> +#define BUF_SIZE (2 * MAX_PB_SIZE * (2 * 4 + MAX_PB_SIZE) + 8)
>>> 
>>> #define randomize_buffers()                          \
>>>     do {                                             \
>>> -- 
>>> 2.31.1
>> 
>> Probably ok (I haven't studied the issue, but this seems plausibly
>> correct).
>> 
> 
> I have also found this issue quite a while ago and am using this here as
> a workaround (it is the minimal set of changes that makes the test pass
> for me):
> 
[…]
> 
> But I have never ever investigated why these buffers and only these
> buffers need to be enlarged and whether there is an underlying bug (i.e.
> whether an access beyond the end of the buffer might happen in a
> non-test scenario). Have you?
> 

I only did the math and test on the upper bound of 2* (src - (uint16_t*)la_buf0)
which src is `pixel *src` in put_hevc_qpel_hv. I didn’t tried to figure out how
the code works or how to make it less error prone.

ffmpeg cmd doesn’t show such error when decoding a HEVC sample with -cpuflags 0:

hevc (Main) (hev1 / 0x31766568), yuv420p10le(tv, progressive), 1920x1080

> - 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".
diff mbox series

Patch

diff --git a/tests/checkasm/hevc_pel.c b/tests/checkasm/hevc_pel.c
index ec24309081..3dc7cd9090 100644
--- a/tests/checkasm/hevc_pel.c
+++ b/tests/checkasm/hevc_pel.c
@@ -34,7 +34,7 @@  static const int denoms[] = {0, 7, 12, -1 };
 static const int offsets[] = {0, 255, -1 };
 
 #define SIZEOF_PIXEL ((bit_depth + 7) / 8)
-#define BUF_SIZE (2 * MAX_PB_SIZE * (2 * 4 + MAX_PB_SIZE))
+#define BUF_SIZE (2 * MAX_PB_SIZE * (2 * 4 + MAX_PB_SIZE) + 8)
 
 #define randomize_buffers()                          \
     do {                                             \