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

[Michael Niedermayer]

On Sun, Dec 06, 2015 at 08:26:41PM +0100, Andreas Cadhalpun wrote:
> On 05.12.2015 03:12, Michael Niedermayer wrote:
> > On Fri, Dec 04, 2015 at 10:28:35PM +0100, Andreas Cadhalpun wrote:
> >> On 03.12.2015 23:09, Michael Niedermayer wrote:
> >>> 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
> 
> So you're only worried that the check makes this slower?
> This check is only needed in the less-likely/less-optimized branch
> of get_ue_golomb, so it shouldn't matter that much.

well, this is a inlined function in a header
adding this check might cause the compiler to stop inlining it
or the larger code size might lead to more cache misses


> 
> > maybe someone has a better idea ...
> 
> Actually, I think the correct error check would be 'log < 7', because
> UPDATE_CACHE only guarantees 25 meaningful bits. Thus I propose:
> --- a/libavcodec/golomb.h
> +++ b/libavcodec/golomb.h
> @@ -68,7 +68,7 @@ static inline int get_ue_golomb(GetBitContext *gb)
>          int log = 2 * av_log2(buf) - 31;
>          LAST_SKIP_BITS(re, gb, 32 - log);
>          CLOSE_READER(re, gb);
> -        if (CONFIG_FTRAPV && log < 0) {
> +        if (log < 7) {
>              av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n");
>              return AVERROR_INVALIDDATA;
>          }
> 
> I've benchmarked this with the fate-cavs sample (which triggers the error)
> cat'ed 100 times together and it isn't any slower than without this change.

my concern is more on h264 (CAVLC) and hevc speed


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- 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/20151206/49281256/attachment.sig>