Re: [FFmpeg-devel] [PATCH] avcodec/h264_mb: Fix undefined shifts
On Thu, Mar 12, 2015 at 07:14:54AM -0400, Ronald S. Bultje wrote: Hi, On Wed, Mar 11, 2015 at 9:00 PM, Michael Niedermayer michae...@gmx.at wrote: Found-by: Clang -fsanitize=shift Reported-by: Thierry Foucu tfo...@google.com Signed-off-by: Michael Niedermayer michae...@gmx.at --- libavcodec/h264_mb.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libavcodec/h264_mb.c b/libavcodec/h264_mb.c index dd406c7..a4653aa 100644 --- a/libavcodec/h264_mb.c +++ b/libavcodec/h264_mb.c @@ -213,7 +213,7 @@ static av_always_inline void mc_dir_part(H264Context *h, H264Picture *pic, 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 6.5.7 Bitwise shift operators ... 4 The result of E1 E2 is E1 left-shifted E2 bit positions; vacated bits are filled with zeros. If E1 has an unsigned type, the value of the result is E1 * 2^E2 , reduced modulo one more than the maximum value representable in the result type. If E1 has a signed type and nonnegative value, and E1 * 2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB it is not once nor twice but times without number that the same ideas make their appearance in the world. -- Aristotle signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/h264_mb: Fix undefined shifts
On Thu, Mar 12, 2015 at 09:57:44AM -0400, Ronald S. Bultje wrote: Hi, On Thu, Mar 12, 2015 at 9:37 AM, Michael Niedermayer michae...@gmx.at wrote: On Thu, Mar 12, 2015 at 07:14:54AM -0400, Ronald S. Bultje wrote: Hi, On Wed, Mar 11, 2015 at 9:00 PM, Michael Niedermayer michae...@gmx.at wrote: Found-by: Clang -fsanitize=shift Reported-by: Thierry Foucu tfo...@google.com Signed-off-by: Michael Niedermayer michae...@gmx.at --- libavcodec/h264_mb.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libavcodec/h264_mb.c b/libavcodec/h264_mb.c index dd406c7..a4653aa 100644 --- a/libavcodec/h264_mb.c +++ b/libavcodec/h264_mb.c @@ -213,7 +213,7 @@ static av_always_inline void mc_dir_part(H264Context *h, H264Picture *pic, 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 6.5.7 Bitwise shift operators ... 4 The result of E1 E2 is E1 left-shifted E2 bit positions; vacated bits are filled with zeros. If E1 has an unsigned type, the value of the result is E1 * 2^E2 , reduced modulo one more than the maximum value representable in the result type. If E1 has a signed type and nonnegative value, and E1 * 2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined. Hm... I guess mx itself is unbounded; ok. applied thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/h264_mb: Fix undefined shifts
Hi, On Thu, Mar 12, 2015 at 9:37 AM, Michael Niedermayer michae...@gmx.at wrote: On Thu, Mar 12, 2015 at 07:14:54AM -0400, Ronald S. Bultje wrote: Hi, On Wed, Mar 11, 2015 at 9:00 PM, Michael Niedermayer michae...@gmx.at wrote: Found-by: Clang -fsanitize=shift Reported-by: Thierry Foucu tfo...@google.com Signed-off-by: Michael Niedermayer michae...@gmx.at --- libavcodec/h264_mb.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libavcodec/h264_mb.c b/libavcodec/h264_mb.c index dd406c7..a4653aa 100644 --- a/libavcodec/h264_mb.c +++ b/libavcodec/h264_mb.c @@ -213,7 +213,7 @@ static av_always_inline void mc_dir_part(H264Context *h, H264Picture *pic, 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 6.5.7 Bitwise shift operators ... 4 The result of E1 E2 is E1 left-shifted E2 bit positions; vacated bits are filled with zeros. If E1 has an unsigned type, the value of the result is E1 * 2^E2 , reduced modulo one more than the maximum value representable in the result type. If E1 has a signed type and nonnegative value, and E1 * 2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined. Hm... I guess mx itself is unbounded; ok. Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/h264_mb: Fix undefined shifts
Hi, 2015-03-12 14:37 GMT+01:00 Michael Niedermayer michae...@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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/h264_mb: Fix undefined shifts
On Thu, Mar 12, 2015 at 03:39:51PM +0100, Christophe Gisquet wrote: Hi, 2015-03-12 14:37 GMT+01:00 Michael Niedermayer michae...@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 signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/h264_mb: Fix undefined shifts
Hi, On Wed, Mar 11, 2015 at 9:00 PM, Michael Niedermayer michae...@gmx.at wrote: Found-by: Clang -fsanitize=shift Reported-by: Thierry Foucu tfo...@google.com Signed-off-by: Michael Niedermayer michae...@gmx.at --- libavcodec/h264_mb.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libavcodec/h264_mb.c b/libavcodec/h264_mb.c index dd406c7..a4653aa 100644 --- a/libavcodec/h264_mb.c +++ b/libavcodec/h264_mb.c @@ -213,7 +213,7 @@ static av_always_inline void mc_dir_part(H264Context *h, H264Picture *pic, 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? Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/h264_mb: Fix undefined shifts
Found-by: Clang -fsanitize=shift
Reported-by: Thierry Foucu tfo...@google.com
Signed-off-by: Michael Niedermayer michae...@gmx.at
---
libavcodec/h264_mb.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/libavcodec/h264_mb.c b/libavcodec/h264_mb.c
index dd406c7..a4653aa 100644
--- a/libavcodec/h264_mb.c
+++ b/libavcodec/h264_mb.c
@@ -213,7 +213,7 @@ static av_always_inline void mc_dir_part(H264Context *h,
H264Picture *pic,
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;
uint8_t *src_y= pic-f.data[0] + offset;
uint8_t *src_cb, *src_cr;
int extra_width = 0;
@@ -288,9 +288,9 @@ static av_always_inline void mc_dir_part(H264Context *h,
H264Picture *pic,
emu |= (my 3) 0 || (my 3) + 8 = (pic_height 1);
}
-src_cb = pic-f.data[1] + ((mx 3) pixel_shift) +
+src_cb = pic-f.data[1] + ((mx 3) * (1 pixel_shift)) +
(my ysh) * h-mb_uvlinesize;
-src_cr = pic-f.data[2] + ((mx 3) pixel_shift) +
+src_cr = pic-f.data[2] + ((mx 3) * (1 pixel_shift)) +
(my ysh) * h-mb_uvlinesize;
if (emu) {
@@ -302,7 +302,7 @@ static av_always_inline void mc_dir_part(H264Context *h,
H264Picture *pic,
}
chroma_op(dest_cb, src_cb, h-mb_uvlinesize,
height (chroma_idc == 1 /* yuv420 */),
- mx 7, (my (chroma_idc == 2 /* yuv422 */)) 7);
+ mx 7, ((unsigned)my (chroma_idc == 2 /* yuv422 */)) 7);
if (emu) {
h-vdsp.emulated_edge_mc(h-edge_emu_buffer, src_cr,
@@ -312,7 +312,7 @@ static av_always_inline void mc_dir_part(H264Context *h,
H264Picture *pic,
src_cr = h-edge_emu_buffer;
}
chroma_op(dest_cr, src_cr, h-mb_uvlinesize, height (chroma_idc == 1 /*
yuv420 */),
- mx 7, (my (chroma_idc == 2 /* yuv422 */)) 7);
+ mx 7, ((unsigned)my (chroma_idc == 2 /* yuv422 */)) 7);
}
static av_always_inline void mc_part_std(H264Context *h, int n, int square,
@@ -485,7 +485,7 @@ static av_always_inline void prefetch_motion(H264Context
*h, int list,
const int mx = (h-mv_cache[list][scan8[0]][0] 2) + 16 * h-mb_x +
8;
const int my = (h-mv_cache[list][scan8[0]][1] 2) + 16 * h-mb_y;
uint8_t **src = h-ref_list[list][refn].f.data;
-int off = (mx pixel_shift) +
+int off = mx * (1 pixel_shift) +
(my + (h-mb_x 3) * 4) * h-mb_linesize +
(64 pixel_shift);
h-vdsp.prefetch(src[0] + off, h-linesize, 4);
@@ -493,7 +493,7 @@ static av_always_inline void prefetch_motion(H264Context
*h, int list,
h-vdsp.prefetch(src[1] + off, h-linesize, 4);
h-vdsp.prefetch(src[2] + off, h-linesize, 4);
} else {
-off= (((mx1)+64)pixel_shift) + ((my1) +
(h-mb_x7))*h-uvlinesize;
+off= ((mx1)+64) * (1pixel_shift) + ((my1) +
(h-mb_x7))*h-uvlinesize;
h-vdsp.prefetch(src[1] + off, src[2] - src[1], 2);
}
}
--
1.7.9.5
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
