Janne Grunau <janne-li...@jannau.net> writes: > 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.
Quite so. >> 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. Detecting errors as soon as possible might be more robust, but simply reading until it naturally terminates is probably faster. -- Måns Rullgård m...@mansr.com _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
