Re: [FFmpeg-devel] [FFmpeg-cvslog] avcodec/mpeg_er: Don't set block_index unnecessarily

Re: [FFmpeg-devel] [FFmpeg-cvslog] avcodec/mpeg_er: Don't set block_index unnecessarily

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 5370 bytes --]

On Thu, Jun 20, 2024 at 05:01:08PM +0000, Andreas Rheinhardt wrote:
> ffmpeg | branch: master | Andreas Rheinhardt <andreas.rheinhardt@outlook.com> | Wed Jun 12 08:37:53 2024 +0200| [65d5ccb808ec93de46a2458ea8cc082ce4460f34] | committer: Andreas Rheinhardt
> 
> avcodec/mpeg_er: Don't set block_index unnecessarily
> 
> ff_init_block_index() sets MpegEncContext.dest and
> MpegEncContext.block_index. The latter is unused by
> ff_mpv_reconstruct_mb() (which is what this code is
> preparatory for) and dest is overwritten a few lines below.
> So don't initialize block_index at all.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> 
> > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=65d5ccb808ec93de46a2458ea8cc082ce4460f34
> ---
> 
>  libavcodec/mpeg_er.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/libavcodec/mpeg_er.c b/libavcodec/mpeg_er.c
> index e7b3197bb1..fe7dcd7efb 100644
> --- a/libavcodec/mpeg_er.c
> +++ b/libavcodec/mpeg_er.c
> @@ -76,10 +76,6 @@ static void mpeg_er_decode_mb(void *opaque, int ref, int mv_dir, int mv_type,
>      s->mcsel      = 0;
>      memcpy(s->mv, mv, sizeof(*mv));
>  
> -    ff_init_block_index(s);
> -    ff_update_block_index(s, s->avctx->bits_per_raw_sample,
> -                          s->avctx->lowres, s->chroma_x_shift);
> -
>      s->bdsp.clear_blocks(s->block[0]);
>      if (!s->chroma_y_shift)
>          s->bdsp.clear_blocks(s->block[6]);

It seems not unnneccesary

Running: 69814/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VC1_fuzzer-4868081575329792
=================================================================
==2146502==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x625000009c80 at pc 0x00000049b63f bp 0x7ffdecbf28c0 sp 0x7ffdecbf2088
WRITE of size 64 at 0x625000009c80 thread T0
    #0 0x49b63e in __asan_memset /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:26:3
    #1 0xe85598 in ff_clean_intra_table_entries ffmpeg/libavcodec/mpegvideo.c:809:5
    #2 0xe9b926 in mpv_reconstruct_mb_internal ffmpeg/libavcodec/mpv_reconstruct_mb_template.c:68:17
    #3 0xe9b926 in ff_mpv_reconstruct_mb ffmpeg/libavcodec/mpegvideo_dec.c:935
    #4 0xb1f790 in guess_mv ffmpeg/libavcodec/error_resilience.c:456:17
    #5 0xb0f9b5 in ff_er_frame_end ffmpeg/libavcodec/error_resilience.c:1224:9
    #6 0x63e595 in vc1_decode_frame ffmpeg/libavcodec/vc1dec.c:1341:13
    #7 0x4fe53f in decode_simple_internal ffmpeg/libavcodec/decode.c:429:20
    #8 0x4fe53f in decode_simple_receive_frame ffmpeg/libavcodec/decode.c:600
    #9 0x4fe53f in decode_receive_frame_internal ffmpeg/libavcodec/decode.c:631
    #10 0x4fcf6d in avcodec_send_packet ffmpeg/libavcodec/decode.c:721:15
    #11 0x4d1da7 in LLVMFuzzerTestOneInput ffmpeg/tools/target_dec_fuzzer.c:533:25
    #12 0x166259d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13
    #13 0x1657172 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6
    #14 0x165c371 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9
    #15 0x1656e50 in main Fuzzer/build/../FuzzerMain.cpp:20:10
    #16 0x7fd643b76082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)
    #17 0x423f6d in _start (ffmpeg/tools/target_dec_vc1_fuzzer+0x423f6d)

Address 0x625000009c80 is a wild pointer.
SUMMARY: AddressSanitizer: heap-buffer-overflow /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:26:3 in __asan_memset
Shadow bytes around the buggy address:
  0x0c4a7fff9340: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a7fff9350: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a7fff9360: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a7fff9370: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a7fff9380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c4a7fff9390:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a7fff93a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a7fff93b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a7fff93c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a7fff93d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a7fff93e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==2146502==ABORTING


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Avoid a single point of failure, be that a person or equipment.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] [FFmpeg-cvslog] avcodec/mpeg_er: Don't set block_index unnecessarily

[Andreas Rheinhardt]

Michael Niedermayer:
> On Thu, Jun 20, 2024 at 05:01:08PM +0000, Andreas Rheinhardt wrote:
>> ffmpeg | branch: master | Andreas Rheinhardt <andreas.rheinhardt@outlook.com> | Wed Jun 12 08:37:53 2024 +0200| [65d5ccb808ec93de46a2458ea8cc082ce4460f34] | committer: Andreas Rheinhardt
>>
>> avcodec/mpeg_er: Don't set block_index unnecessarily
>>
>> ff_init_block_index() sets MpegEncContext.dest and
>> MpegEncContext.block_index. The latter is unused by
>> ff_mpv_reconstruct_mb() (which is what this code is
>> preparatory for) and dest is overwritten a few lines below.
>> So don't initialize block_index at all.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>>
>>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=65d5ccb808ec93de46a2458ea8cc082ce4460f34
>> ---
>>
>>  libavcodec/mpeg_er.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/libavcodec/mpeg_er.c b/libavcodec/mpeg_er.c
>> index e7b3197bb1..fe7dcd7efb 100644
>> --- a/libavcodec/mpeg_er.c
>> +++ b/libavcodec/mpeg_er.c
>> @@ -76,10 +76,6 @@ static void mpeg_er_decode_mb(void *opaque, int ref, int mv_dir, int mv_type,
>>      s->mcsel      = 0;
>>      memcpy(s->mv, mv, sizeof(*mv));
>>  
>> -    ff_init_block_index(s);
>> -    ff_update_block_index(s, s->avctx->bits_per_raw_sample,
>> -                          s->avctx->lowres, s->chroma_x_shift);
>> -
>>      s->bdsp.clear_blocks(s->block[0]);
>>      if (!s->chroma_y_shift)
>>          s->bdsp.clear_blocks(s->block[6]);
> 
> It seems not unnneccesary
> 
> Running: 69814/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VC1_fuzzer-4868081575329792
> =================================================================
> ==2146502==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x625000009c80 at pc 0x00000049b63f bp 0x7ffdecbf28c0 sp 0x7ffdecbf2088
> WRITE of size 64 at 0x625000009c80 thread T0
>     #0 0x49b63e in __asan_memset /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:26:3
>     #1 0xe85598 in ff_clean_intra_table_entries ffmpeg/libavcodec/mpegvideo.c:809:5
>     #2 0xe9b926 in mpv_reconstruct_mb_internal ffmpeg/libavcodec/mpv_reconstruct_mb_template.c:68:17
>     #3 0xe9b926 in ff_mpv_reconstruct_mb ffmpeg/libavcodec/mpegvideo_dec.c:935
>     #4 0xb1f790 in guess_mv ffmpeg/libavcodec/error_resilience.c:456:17
>     #5 0xb0f9b5 in ff_er_frame_end ffmpeg/libavcodec/error_resilience.c:1224:9
>     #6 0x63e595 in vc1_decode_frame ffmpeg/libavcodec/vc1dec.c:1341:13
>     #7 0x4fe53f in decode_simple_internal ffmpeg/libavcodec/decode.c:429:20
>     #8 0x4fe53f in decode_simple_receive_frame ffmpeg/libavcodec/decode.c:600
>     #9 0x4fe53f in decode_receive_frame_internal ffmpeg/libavcodec/decode.c:631
>     #10 0x4fcf6d in avcodec_send_packet ffmpeg/libavcodec/decode.c:721:15
>     #11 0x4d1da7 in LLVMFuzzerTestOneInput ffmpeg/tools/target_dec_fuzzer.c:533:25
>     #12 0x166259d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13
>     #13 0x1657172 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6
>     #14 0x165c371 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9
>     #15 0x1656e50 in main Fuzzer/build/../FuzzerMain.cpp:20:10
>     #16 0x7fd643b76082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082)
>     #17 0x423f6d in _start (ffmpeg/tools/target_dec_vc1_fuzzer+0x423f6d)
> 
> Address 0x625000009c80 is a wild pointer.
> SUMMARY: AddressSanitizer: heap-buffer-overflow /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:26:3 in __asan_memset
> Shadow bytes around the buggy address:
>   0x0c4a7fff9340: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c4a7fff9350: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c4a7fff9360: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c4a7fff9370: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c4a7fff9380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> =>0x0c4a7fff9390:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c4a7fff93a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c4a7fff93b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c4a7fff93c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c4a7fff93d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c4a7fff93e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> Shadow byte legend (one shadow byte represents 8 application bytes):
>   Addressable:           00
>   Partially addressable: 01 02 03 04 05 06 07
>   Heap left redzone:       fa
>   Freed heap region:       fd
>   Stack left redzone:      f1
>   Stack mid redzone:       f2
>   Stack right redzone:     f3
>   Stack after return:      f5
>   Stack use after scope:   f8
>   Global redzone:          f9
>   Global init order:       f6
>   Poisoned by user:        f7
>   Container overflow:      fc
>   Array cookie:            ac
>   Intra object redzone:    bb
>   ASan internal:           fe
>   Left alloca redzone:     ca
>   Right alloca redzone:    cb
>   Shadow gap:              cc
> ==2146502==ABORTING
> 

Sorry for this. I knew that VC-1 (and RV34) use ff_mpv_reconstruct_mb()
for error resilience, but I thought that everything would be fine given
that they initialize block_index. Apparently I was wrong. Anyway, I have
sent a patch that should fix this:
https://ffmpeg.org/pipermail/ffmpeg-devel/2024-July/330463.html Please
confirm that it indeed does fix it.

- Andreas

PS: This patchset also includes a patch that effectively reverts a patch
of yours:
https://ffmpeg.org/pipermail/ffmpeg-devel/2024-July/330471.html Can you
test that it does not reenable the bug your patch fixed?

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