[FFmpeg-devel] [PATCH] avcodec/golomb: Prevent shift by negative number

[Michael Niedermayer]

On Mon, Jul 13, 2020 at 09:04:30PM +0200, Tomas Härdin wrote:
> fre 2020-07-10 klockan 15:48 +0200 skrev Andreas Rheinhardt:
> > This happened in get_ue_golomb() if the cached bitstream reader was
> > in
> > use, because there was no check to handle the case of the read value
> > not being in the range 0..8190.
> > 
> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> > ---
> >  libavcodec/golomb.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h
> > index 7fd46a91bd..5bfcfe085f 100644
> > --- a/libavcodec/golomb.h
> > +++ b/libavcodec/golomb.h
> > @@ -66,6 +66,10 @@ static inline int get_ue_golomb(GetBitContext *gb)
> >          return ff_ue_golomb_vlc_code[buf];
> >      } else {
> >          int log = 2 * av_log2(buf) - 31;
> > +        if (log < 0) {
> > +            av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n");
> > +            return AVERROR_INVALIDDATA;
> > +        }
> 
> How come invalid codes can even be present like this? Seems
> inefficient. Or is the decoder wrong? Maybe Michael wants to chime in
> here, since he wrote the original implementation.

The codepath of the non cached one does a check too. It should be
done consistently.
If the check causes a meassurable speedloss, then it can be implemented
without a check. For example by using asm for the shift operation, or maybe
compilers have some nicer function doing a never undefined shift. If such
function does not exist, its something that maybe someone should add to
gcc & clang because other projects could benefit from it too in similar
situations.
Of course all such improvments make the code more complex as a fallback
with check is needed if none of the non standard "safe" functions exist.
So it only makes sense to go that way if this actually affects speed.
It is after all not on the main codepath which is handled with a table

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200713/89a31396/attachment.sig>