On 2012-11-29 17:21:45 +0000, Måns Rullgård wrote: > Janne Grunau <janne-li...@jannau.net> writes: > > > On 2012-11-29 13:46:12 +0000, Måns Rullgård wrote: > >> Janne Grunau <janne-li...@jannau.net> writes: > >> > >> > On 2012-11-29 00:08:52 +0000, Måns Rullgård wrote: > >> >> Janne Grunau <janne-li...@jannau.net> writes: > >> >> > >> >> > On 2012-11-28 23:52:27 +0100, Luca Barbato wrote: > >> >> >> > >> >> >> Is INVALID_VLC value negative? > >> >> > > >> >> > no, 0x80000000. But arithmetic conversion saves us. > >> >> > >> >> Ouch, that's _really_ bad. The svq3_get_ue_golomb() return type is > >> >> (inexplicably) int, so returning that value entails a conversion with > >> >> implementation-defined behaviour. Most compilers leave the bits intact > >> >> in such conversions, but I'd rather not depend on it. Also, such code > >> >> is nothing short of obfuscated even if it does work reliably. > >> > > >> > 6.3.1.3 reads to me as if signed int to unsigned int conversion is well > >> > defined: > >> > >> Yes, but we're dealing with unsigned to signed here. The integer > >> constant 0x80000000 has type unsigned int (if int is 32-bit). Returning > >> this from svq3_get_ue_golomb() as a signed int invokes an > >> implementation-defined conversion. > > > > svq3_get_ue_golomb() doesn't return INVALID_VLC explicitly. > > Right, I confused it with svq3_get_se_golomb(). svq3_get_ue_golomb() is > actually worse. > > > The only it can return something negative is due to signed arithmetic > > on the int ret in the else branch. > > The function has two return statements. The first returns a value from > ff_interleaved_ue_golomb_vlc_code[] (array of uint8_t), so this one is > safe. The other returns "ret - 1" where ret is a signed int. For this > to produce a negative value other than -1, ret must itself be negative. > This can only happen through the left shifts in the loop overflowing > into the sign bit. Such an overflow has *undefined* behaviour.
returning -1 relies on undefined behaviour too. ret is initialized to 1 and the only operations are left shifts and bitwise ORs. > input can cause this to happen (and your patch suggests this is the > case), the code is broken and must be fixed here. Checking it after the > fact is not good enough. I see no reason why the computation in svq3_get_ue_golomb() can't be changed to unsigned and at least make fate agrees. The only decision to be made is whether to detect truncation/invalid codes or keep reading until it is properly terminated. Janne _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
