[FFmpeg-devel] [PATCH] avcodec/h264_mb: Fix undefined shifts

[Christophe Gisquet]

Hi,

2015-03-12 14:37 GMT+01:00 Michael Niedermayer <michaelni at gmx.at>:
>> >      const int mx      = h->mv_cache[list][scan8[n]][0] + src_x_offset * 8;
>> >      int my            = h->mv_cache[list][scan8[n]][1] + src_y_offset * 8;
>> >      const int luma_xy = (mx & 3) + ((my & 3) << 2);
>> > -    ptrdiff_t offset  = ((mx >> 2) << pixel_shift) + (my >> 2) *
>> > h->mb_linesize;
>> > +    ptrdiff_t offset  = (mx >> 2) * (1 << pixel_shift) + (my >> 2) *
>> > h->mb_linesize;
>>
>>
>> Why is this undefined?
>
> Because C sucks

I actually have an equivalent question to Ronald's. Is there a valid
input that causes the undefined behaviour? For an invalid input (e.g.
DoS tentative), is the behaviour worsened?

More in (uninformed) details:

mx is probably already constrained by AVC specifications. Another
limit to its validness is mx being a bit more than 4 times as large as
the image width. In that case, what would the image width be to cause
this undefined behaviour? It looks to me like for anything vaguely
sensible, it would not fall within the undefined behaviour.

For invalid inputs (where in particular the content of mv_cache was
not properly validated), let's say you get something that is larger
than the image. So what we get is a correctly computed value, yet
still invalid. I'm probably overlooking this here.

I'd prefer some fuzzing to actually yield a crash scenario before
acting on such reports (which are not clear to me as actually causing
a degradation). Otherwise, some of those issues are mostly pedantic.
On the other hand, we are only loosing 3 cycles out of several
hundreds.

If we are going to overengineer such issues, we could have something like:
#if HAVE_FSANITIZED_SHIFT_PLEASURING
# define LEGAL_SHIFT(a, b, type) ( (a) * (((type)1) << (b) )
#else
# define LEGAL_SHIFT(a, b, type) ( (a) << (b) )
#endif

-- 
Christophe