signed integer overflow in ff_h264_decode_mb_cabac()

[tsmith]

Summary of the bug:

UBSan: libavcodec/h264_cabac.c:2168:25: runtime error: signed integer overflow: -37 + -2147483648 cannot be represented in type 'int'

How to reproduce:

% ffmpeg -f h264 -i test_case.264 -f null -
ffmpeg version N-77801-gd637a58 Copyright (c) 2000-2016 the FFmpeg developers
  built with Ubuntu clang version 3.7.1-svn253742-1~exp1 (branches/release_37) (based on LLVM 3.7.1)
  configuration: --cc=clang --cxx=clang++ --disable-libxcb --disable-xlib --disable-ffprobe --disable-ffplay --disable-sdl --disable-ffserver --disable-doc --disable-pthreads --disable-network --disable-d3d11va --disable-dxva2 --disable-vaapi --disable-vda --disable-vdpau --disable-stripping --disable-runtime-cpudetect --disable-securetransport --disable-iconv
  libavutil      55. 13.100 / 55. 13.100
  libavcodec     57. 22.100 / 57. 22.100
  libavformat    57. 21.101 / 57. 21.101
  libavdevice    57.  0.100 / 57.  0.100
  libavfilter     6. 23.100 /  6. 23.100
  libswscale      4.  0.100 /  4.  0.100
  libswresample   2.  0.101 /  2.  0.101
[h264 @ 0x619000004680] Warning: not compiled with thread support, using thread emulation
[h264 @ 0x619000004680] Reducing left cropping to 0 chroma samples to preserve alignment.
[h264 @ 0x619000004680] crop values invalid 0 1 3 23 / 48 16
[h264 @ 0x619000004680] Overread SPS by 8 bits
[h264 @ 0x619000004680] non-existing PPS 2 referenced
[h264 @ 0x619000004680] pps_id 764 out of range
[h264 @ 0x619000004680] illegal aspect ratio
[h264 @ 0x619000004680] Different chroma and luma bit depth is not implemented. Update your FFmpeg version to the newest one from Git. If the problem still occurs, it means that your file has a feature which has not been implemented.
[h264 @ 0x619000004680] If you want to help, upload a sample of this file to ftp://upload.ffmpeg.org/incoming/ and contact the ffmpeg-devel mailing list. (ffmpeg-devel@ffmpeg.org)
[h264 @ 0x619000004680] Overread SPS by 3 bits
[h264 @ 0x619000004680] overflow in decode_cabac_mb_mvd
libavcodec/h264_cabac.c:2168:25: runtime error: signed integer overflow: -37 + -2147483648 cannot be represented in type 'int'
    #0 0x23746ac in ff_h264_decode_mb_cabac /home/user/code/ffmpeg/libavcodec/h264_cabac.c:2164:46
    #1 0x1063129 in decode_slice /home/user/code/ffmpeg/libavcodec/h264_slice.c:2377:19
    #2 0x10619e4 in ff_h264_execute_decode_slices /home/user/code/ffmpeg/libavcodec/h264_slice.c:2550:15
    #3 0xf94951 in decode_nal_units /home/user/code/ffmpeg/libavcodec/h264.c:1647:23
    #4 0xf9c490 in h264_decode_frame /home/user/code/ffmpeg/libavcodec/h264.c:1832:17
    #5 0x1a46a86 in avcodec_decode_video2 /home/user/code/ffmpeg/libavcodec/utils.c:2115:19
    #6 0xc2f0e5 in try_decode_frame /home/user/code/ffmpeg/libavformat/utils.c:2760:19
    #7 0xc26a80 in avformat_find_stream_info /home/user/code/ffmpeg/libavformat/utils.c:3416:9
    #8 0x53c6cb in open_input_file /home/user/code/ffmpeg/ffmpeg_opt.c:970:11
    #9 0x53a94f in open_files /home/user/code/ffmpeg/ffmpeg_opt.c:2999:15
    #10 0x53a11c in ffmpeg_parse_options /home/user/code/ffmpeg/ffmpeg_opt.c:3036:11
    #11 0x56f5ab in main /home/user/code/ffmpeg/ffmpeg.c:4299:11
    #12 0x7fbe386acec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
    #13 0x466445 in _start (/home/user/Desktop/ffmpeg/ffmpeg_full+0x466445)

[Joshua Bowman]

Is there a need for strictly defined behavior in integer data overflows for corrupt files? No pointers are involved.

[tsmith]

It seems this issue is triggered when calculating a value for 'const int index'.
const int index= 4*i + block_width*j;

Worst case this could lead to reading or writing from somewhere that was unintended. And in that case corrupted input or not it needs to be prevented to avoid potential attacks (assuming that it can access interesting data). On the other hand it may just turn out to be a blip in the output (which really doesn't matter if the input is corrupted to begin with). Also I'm not sure if this can be triggered by a valid file I just assume it can.

The real issue here is that signed integer overflows are undefined in C (and C++). Since it is undefined this allows the compiler to make certain assumptions and optimization that can result in strange behavior. With signed integers it is not guaranteed that INT_MAX + 1 will result in INT_MIN.

It is nice to have issues like this fixed from a bug hunting perspective but I understand there is more to it than that. If this truly is benign and not worth fixing (this also goes for similar overflows I have logged) then I will let you guys make the call here.

Perhaps using '-fwrapv'[1] to make signed int overflows defined is an option? This also makes them operate in the manner that most people assume. This may also prevent the use of some optimizations, decreasing performance and this is also bad, likely worse.

[1]​https://gcc.gnu.org/onlinedocs/gcc-4.0.2/gcc/Code-Gen-Options.html

[Carl Eugen Hoyos]

Should be fixed by Michael in e5655a32bc745462cb820f4ccc3eaee146dd2cdc