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

[Andreas Cadhalpun]

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.

> 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.

>> 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.

I see. I've also other things higher up in my TODO list...

> 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 ...

Indeed.

Best regards,
Andreas