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

[Andreas Rheinhardt]

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