Opened 2 years ago

Closed 23 months ago

Last modified 23 months ago

#7997 closed defect (fixed)

undefined-behavior at libavcodec/proresenc_anatoliy.c

Reported by: Suhwan Owned by:
Priority: normal Component: avcodec
Version: git-master Keywords: ubsan
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

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 }

Attachments (2)

gdb_log_7997 (14.8 KB ) - added by Suhwan 2 years ago.
tmp.avi (282.5 KB ) - added by Suhwan 2 years ago.

Download all attachments as: .zip

Change History (8)

by Suhwan, 2 years ago

Attachment: gdb_log_7997 added

by Suhwan, 2 years ago

Attachment: tmp.avi added

comment:1 by Jun Zhao, 2 years ago

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

in reply to:  1 comment:2 by Suhwan, 2 years ago

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,

Last edited 2 years ago by Suhwan (previous) (diff)

comment:3 by Balling, 23 months ago

Resolution: fixed
Status: newclosed

Fixed in 198081efb7c7343349f0a7acc836f001c511e990

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

Last edited 23 months ago by Balling (previous) (diff)

in reply to:  3 ; comment:4 by mkver, 23 months ago

Component: undeterminedavcodec

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.

in reply to:  4 comment:5 by Balling, 23 months ago

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.

comment:6 by mkver, 23 months ago

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.

Note: See TracTickets for help on using tickets.