[FFmpeg-devel] [PATCH] avcodec/h264_mb: Fix undefined shifts
michaelni at gmx.at
Thu Mar 12 16:17:30 CET 2015
On Thu, Mar 12, 2015 at 03:39:51PM +0100, Christophe Gisquet wrote:
> 2015-03-12 14:37 GMT+01:00 Michael Niedermayer <michaelni at gmx.at>:
> >> > const int mx = h->mv_cache[list][scan8[n]] + src_x_offset * 8;
> >> > int my = h->mv_cache[list][scan8[n]] + 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
the compiler should optimize the extra operation out, ive
confirmed this before posting the patch in some cases but i didnt
> 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) )
> # define LEGAL_SHIFT(a, b, type) ( (a) << (b) )
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
Size: 181 bytes
Desc: Digital signature
More information about the ffmpeg-devel