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

[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?

- Andreas