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

[Michael Niedermayer]

On Thu, Mar 12, 2015 at 03:39:51PM +0100, Christophe Gisquet wrote:
> 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?

i belive any motion vector that points left outside the picture will
trigger this one, its also happening with multiple fate samples
This issue is about undefined behavor according to the C specification
not about current gcc generating actually bad code from it AFAIK


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

the compiler should optimize the extra operation out, ive
confirmed this before posting the patch in some cases but i didnt
check all


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

that could be done it would make the code less readable though

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

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150312/4c58b0b9/attachment.asc>