[FFmpeg-devel] [PATCH 5/6] Revert "avcodec/er: remove check for fields"

[Michael Niedermayer]

On Sun, Apr 09, 2023 at 04:26:26PM +0200, Michael Niedermayer wrote:
> Fixes: out of array write on x86-32
> Fixes: 57825/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG2VIDEO_fuzzer-6094366187061248
> Fixes: 57829/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG2VIDEO_fuzzer-4526419991724032
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> This reverts commit f7abe92bd7939b6aeeb2393fe141033e733305d4.
> ---
>  libavcodec/error_resilience.c | 9 ++-------
>  libavcodec/error_resilience.h | 1 -
>  2 files changed, 2 insertions(+), 8 deletions(-)

Heres a backtrace for this btw

==7150==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xf62fe800 at pc 0x0899a5e5 bp 0xffc46c68 sp 0xffc46c60
WRITE of size 4 at 0xf62fe800 thread T0
    #0 0x899a5e4 in put_pixels8_8_c libavcodec/pel_template.c:78:1
    #1 0x8999ce3 in put_pixels16_8_c libavcodec/pel_template.c:78:1
    #2 0x82fafc4 in mpv_reconstruct_mb_internal libavcodec/mpv_reconstruct_mb_template.c:294:13
    #3 0x82fafc4 in ff_mpv_reconstruct_mb libavcodec/mpegvideo_dec.c:1023
    #4 0x82ae910 in mpeg_er_decode_mb libavcodec/mpeg_er.c:99:5
    #5 0x8752695 in guess_mv libavcodec/error_resilience.c:452:17
    #6 0x873cd02 in ff_er_frame_end libavcodec/error_resilience.c:1231:9
    #7 0x8273b04 in slice_end libavcodec/mpeg12dec.c:2027:9
    #8 0x8273b04 in decode_chunks libavcodec/mpeg12dec.c:2464
    #9 0x824455e in mpeg_decode_frame libavcodec/mpeg12dec.c:2813:11
    #10 0x818e36b in decode_simple_internal libavcodec/decode.c:287:15
    #11 0x816e31b in decode_simple_receive_frame libavcodec/decode.c:540:15
    #12 0x816e31b in decode_receive_frame_internal libavcodec/decode.c:560
    #13 0x816d7d6 in avcodec_send_packet libavcodec/decode.c:635:15
    #14 0x8129196 in LLVMFuzzerTestOneInput tools/target_dec_fuzzer.c:513:25
    #15 0x9060434 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned int) Fuzzer/./FuzzerLoop.cpp:495:13
    #16 0x9056298 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned int) Fuzzer/./FuzzerDriver.cpp:273:6
    #17 0x905a607 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned int)) Fuzzer/./FuzzerDriver.cpp:690:9
    #18 0x9056026 in main Fuzzer/./FuzzerMain.cpp:20:10
    #19 0xf7aaffa0 in __libc_start_main /build/glibc-IskIyT/glibc-2.27/csu/../csu/libc-start.c:310
    #20 0x8079541 in _start (tools/target_dec_mpeg2video_fuzzer+0x8079541)

0xf62fe800 is located 0 bytes to the right of 532480-byte region [0xf627c800,0xf62fe800)
allocated by thread T0 here:
    #0 0x80f0c5c in posix_memalign /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cc:226:3
    #1 0x8fcda9b in av_malloc libavutil/mem.c:105:9
    #2 0x8f290a9 in av_buffer_alloc libavutil/buffer.c:82:12
    #3 0x81319aa in fuzz_video_get_buffer tools/target_dec_fuzzer.c:131:25
    #4 0x81319aa in fuzz_get_buffer2 tools/target_dec_fuzzer.c:152
    #5 0x8186c1c in ff_get_buffer libavcodec/decode.c:1545:11
    #6 0x839c0d1 in ff_thread_get_ext_buffer libavcodec/pthread_frame.c:947:16
    #7 0x8b65f9f in alloc_frame_buffer libavcodec/mpegpicture.c:145:13
    #8 0x8b65f9f in ff_alloc_picture libavcodec/mpegpicture.c:272
    #9 0x82eab7d in alloc_picture libavcodec/mpegvideo_dec.c:245:12
    #10 0x82dd1c7 in ff_mpv_frame_start libavcodec/mpegvideo_dec.c:329:9
    #11 0x825eee7 in mpeg_field_start libavcodec/mpeg12dec.c:1580:20
    #12 0x825eee7 in decode_chunks libavcodec/mpeg12dec.c:2712
    #13 0x824455e in mpeg_decode_frame libavcodec/mpeg12dec.c:2813:11
    #14 0x818e36b in decode_simple_internal libavcodec/decode.c:287:15
    #15 0x816e31b in decode_simple_receive_frame libavcodec/decode.c:540:15
    #16 0x816e31b in decode_receive_frame_internal libavcodec/decode.c:560
    #17 0x816d7d6 in avcodec_send_packet libavcodec/decode.c:635:15
    #18 0x8129196 in LLVMFuzzerTestOneInput tools/target_dec_fuzzer.c:513:25
    #19 0x9060434 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned int) Fuzzer/./FuzzerLoop.cpp:495:13
    #20 0x9056298 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned int) Fuzzer/./FuzzerDriver.cpp:273:6
    #21 0x905a607 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned int)) Fuzzer/./FuzzerDriver.cpp:690:9
    #22 0x9056026 in main Fuzzer/./FuzzerMain.cpp:20:10
    #23 0xf7aaffa0 in __libc_start_main /build/glibc-IskIyT/glibc-2.27/csu/../csu/libc-start.c:310


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20230409/b0c5ca66/attachment.sig>