On Fri, Dec 04, 2015 at 10:28:35PM +0100, Andreas Cadhalpun wrote: > On 03.12.2015 23:09, Michael Niedermayer wrote: > > From: Michael Niedermayer <mich...@niedermayer.cc> > > > > Fixes undefined behavior > > Fixes: mozilla bug 1229208 > > Fixes: > > fbeb8b2c7c996e9b91c6b1af319d7ebc/asan_heap-oob_195450f_2743_e8856ece4579ea486670be2b236099a0.bit > > > > Found-by: Tyson Smith > > Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind > > Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc> > > --- > > libavcodec/golomb.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h > > index d30bb6b..323665d 100644 > > --- a/libavcodec/golomb.h > > +++ b/libavcodec/golomb.h > > @@ -72,7 +72,7 @@ static inline int get_ue_golomb(GetBitContext *gb) > > av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n"); > > return AVERROR_INVALIDDATA; > > } > > - buf >>= log; > > + buf >>= log & 31; > > buf--; > > > > return buf; > > > > While that certainly fixes the undefined behavior, I'm wondering what's the > relation > to commit fd165ac. In other words, why not just remove the CONFIG_FTRAPV from > the error check above?
the & 31 is a hack really (and choosen because at least clang optimizes it out) the "correct" way would be to test, print a warning and return an error code. That way if a valid (non fuzzed) file triggers it we know that the range of get_ue_golomb() is potentially too small. With the & 31 no information is shown, before this patch here ubsan would show it as well without the normal case being slower so its all perfect already ... except ... that its wrong because its undefined behavior maybe someone has a better idea ... > > Also, if you are interested in fixing such undefined behavior, I have lots of > fuzzed samples triggering ubsan all over the place... I think my todo list is too long to fix all. Maybe others are interrested in helping. The really tricky part is to fix some of these issues without causing a slowdown in speed relevant code and without making maintaince harder for example decoding a file from a bug report with ubsan and looking for overflows can in rare instances directly point to the problem or close to it. One needs to keep this in mind when Fixing/Hiding ubsan issues, sometimes a log message and error out is better than extending precission or using unsigned sometimes its not ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Awnsering whenever a program halts or runs forever is On a turing machine, in general impossible (turings halting problem). On any real computer, always possible as a real computer has a finite number of states N, and will either halt in less than N cycles or never halt.
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
