| Message ID | tencent_35479C24E0C45B76344B81E234533FCD5609@qq.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] checkasm/hevc_pel: fix stack-buffer-overflow | expand |
| 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 |
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
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: > 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
> 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 --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 { \
==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(-)