| Message ID | 20220910223046.5135-2-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 8008940da5aa43895fd4574114309c3324249eab |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avcodec/apedec: Fix integer overflow in filter_3800() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Hi, Down-casting to a signed type (here, int16_t) is implementation-defined. And while normal compilers do the expected thing, with modulo-2^n complement, sanitizers tend to dislike it. AFAIK, the clean solution is via an union whence you assign the uint16_t member, and then read the int16_t member. Fortunately, GCC and LLVM are able to optimise that construct back to a single sign-extension. Br,
Rémi Denis-Courmont: > Hi, > > Down-casting to a signed type (here, int16_t) is implementation-defined. And while normal compilers do the expected thing, with modulo-2^n complement, sanitizers tend to dislike it. > 1. We expect the implementation-defined behaviour for signed types to match the typical two's complement behaviour, see https://ffmpeg.org/developer.html#C-language-features 2. In this case, there is no real-implementation-defined behaviour: While the value (int16_t)v is implementation defined, whether it coincides with v is not (it does so if and only if the value of v is representable in an int16_t). 3. What sanitizers dislike it? After all, this is an explicit cast, so e.g. UBSan would never report it as suspicious. > AFAIK, the clean solution is via an union whence you assign the uint16_t member, and then read the int16_t member. Fortunately, GCC and LLVM are able to optimise that construct back to a single sign-extension. > Type-punning via unions is implementation-defined, too. It will work with two's complement types (as long as the number where all value bits are unset and the sign bit is set is not a trap representation (all exact-width intN_t are of this type)). - Andreas
diff --git a/libavcodec/dstdec.c b/libavcodec/dstdec.c index 6bdd6c885c8..4b1762db33c 100644 --- a/libavcodec/dstdec.c +++ b/libavcodec/dstdec.c @@ -215,7 +215,7 @@ static uint8_t prob_dst_x_bit(int c) return (ff_reverse[c & 127] >> 1) + 1; } -static void build_filter(int16_t table[DST_MAX_ELEMENTS][16][256], const Table *fsets) +static int build_filter(int16_t table[DST_MAX_ELEMENTS][16][256], const Table *fsets) { int i, j, k, l; @@ -226,14 +226,17 @@ static void build_filter(int16_t table[DST_MAX_ELEMENTS][16][256], const Table * int total = av_clip(length - j * 8, 0, 8); for (k = 0; k < 256; k++) { - int v = 0; + int64_t v = 0; for (l = 0; l < total; l++) v += (((k >> l) & 1) * 2 - 1) * fsets->coeff[i][j * 8 + l]; + if ((int16_t)v != v) + return AVERROR_INVALIDDATA; table[i][j][k] = v; } } } + return 0; } static int decode_frame(AVCodecContext *avctx, AVFrame *frame, @@ -328,7 +331,9 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *frame, return AVERROR_INVALIDDATA; ac_init(ac, gb); - build_filter(s->filter, &s->fsets); + ret = build_filter(s->filter, &s->fsets); + if (ret < 0) + return ret; memset(s->status, 0xAA, sizeof(s->status)); memset(dsd, 0, frame->nb_samples * 4 * channels);
Fixes: signed integer overflow: 1917019860 + 265558963 cannot be represented in type 'int' Fixes: 48798/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_DST_fuzzer-4833165046317056 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/dstdec.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)