[FFmpeg-devel] [PATCH] avcodec/golomb: Mask shift amount before use in get_ue_golomb()

[Michael Niedermayer]

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 <michael at 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 <michael at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151205/fe238e5d/attachment.sig>