undefined-behavior at libavcodec/proresenc_anatoliy.c

[Suhwan]

Summary of the bug:
There's am undefined-behavior at libavcodec/proresenc_anatoliy.c:250:12

How to reproduce:

% ffmpeg_g  -y -r 8 -i tmp.avi -map 0 -c:v snow -c:v prores_aw -disposition:a:79 prores_aw -disposition:a ffv1 -vframes 42 -aframes 83 -r 9 -level 3 -b:v 507k tmp_.adx

ffmpeg version : N-94163-g664a27ea40
built with clang version 9.0.0

negative value "prev_dc" is left shifted

#define TO_GOLOMB(val) (((val) << 1) ^ ((val) >> 31))

242 static void encode_dc_coeffs(PutBitContext *pb, int16_t *in,
243         int blocks_per_slice, int *qmat)
244 {
245     int prev_dc, code;
246     int i, sign, idx;
247     int new_dc, delta, diff_sign, new_code;
248 
249     prev_dc = QSCALE(qmat, 0, in[0] - 16384);
250     code = TO_GOLOMB(prev_dc);
251     encode_codeword(pb, code, FIRST_DC_CB);
252 
253     code = 5; sign = 0; idx = 64;
254     for (i = 1; i < blocks_per_slice; i++, idx += 64) {
255         new_dc    = QSCALE(qmat, 0, in[idx] - 16384);
256         delta     = new_dc - prev_dc;
257         diff_sign = DIFF_SIGN(delta, sign);
258         new_code  = TO_GOLOMB2(get_level(delta), diff_sign);
259 
260         encode_codeword(pb, new_code, dc_codebook[FFMIN(code, 6)]);
261 
262         code      = new_code;
263         sign      = delta >> 31;
264         prev_dc   = new_dc;
265     }
266 }

[Jun Zhao]

Do you use a automated software testing tools to find this type issue? or just by manually test?

[Suhwan]

Replying to mypopy:

Do you use a automated software testing tools to find this type issue? or just by manually test?

I use an automated software testing tool called fuzzer to find issues,

[Balling]

Fixed in 198081efb7c7343349f0a7acc836f001c511e990

You know, you cannot right shift negative values as well?

[mkver]

Replying to Balling:

Fixed in 198081efb7c7343349f0a7acc836f001c511e990

You know, you cannot right shift negative values as well?

That's not true: Right shifting negative values (by a nonnegative value that is less than the width of the (promoted) left operand) is implementation-defined and FFmpeg requires this to use sign-extension (see here). That's also why UBSan doesn't complain about the right shift.

In contrast, left shifting of negative values is undefined.

[Balling]

Replying to mkver:

FFmpeg requires this to use sign-extension That's also why UBSan doesn't complain about the right shift.

"Implemenation defined" is the compiler decision, ffmpeg cannot require it ;) Just saying.

[mkver]

FFmpeg can choose to support only systems that support certain requirements; if your system doesn't fulfill them, it's your problem. Just saying.

Furthermore, you probably don't understand the important difference between undefined and implementation-defined. The former allows a compiler to presume that situations that trigger undefined behaviour simply don't exist and optimize accordingly. In the situation of this patch, this means that the compiler is allowed to infer that val is >= 0, because it is left-shifted; and then this means that (val) >> 31 is 0 (presuming the typical 32bit ints), so that the second part of said macro can be eliminated. (This requires that both (val) in the macro are the same, i.e. that they are not the result of some function-call that would get evaluated twice (with possible side-effects and possibly different results); this requirement is fulfilled here.) Implementation-defined behaviour on the other hand does not confer said right on the compiler.