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

[Ganesh Ajjanagadde]

On Fri, Dec 4, 2015 at 9:12 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> 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 ...

Would like to add a comment here: wrt ubsan, I think clang's is
slightly better than gcc's. GCC seems to have some bugs, for instance
it does not work correctly wrt the FFNABS solution for safe absolute
value, while clang's does. I have remarked upon this in the relevant
trac ticket:
https://trac.ffmpeg.org/ticket/4727#comment:8

>
> [...]
> --
> 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.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>