| Message ID | 20170310142452.22210-2-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | a557ae8d52ce1cfaf3be5cdb13728b7b2b9512b9 |
| Headers | show |
On Fri, 10 Mar 2017 15:24:52 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: 755/clusterfuzz-testcase-5369072516595712 > > See: [FFmpeg-devel] [PATCH 1/2] avcodec/h264_direct: Fix runtime error: signed integer overflow: 2147483647 - -14133 cannot be represented in type 'int' > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/h264_direct.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/h264_direct.c b/libavcodec/h264_direct.c > index cbb84665b3..66e54479d1 100644 > --- a/libavcodec/h264_direct.c > +++ b/libavcodec/h264_direct.c > @@ -39,7 +39,12 @@ static int get_scale_factor(H264SliceContext *sl, > int poc, int poc1, int i) > { > int poc0 = sl->ref_list[0][i].poc; > - int td = av_clip_int8(poc1 - poc0); > + int64_t pocdiff = poc1 - (int64_t)poc0; > + int td = av_clip_int8(pocdiff); > + > + if (pocdiff != (int)pocdiff) > + avpriv_request_sample(sl->h264->avctx, "pocdiff overflow\n"); > + > if (td == 0 || sl->ref_list[0][i].parent->long_ref) { > return 256; > } else { Hard to image that these poc values aren't bounded by something else, but I don't know. Also the previous patch didn't have this request_sample call, which inflates this whole thing by 5 lines of code.
On Fri, Mar 10, 2017 at 04:01:36PM +0100, wm4 wrote: > On Fri, 10 Mar 2017 15:24:52 +0100 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > Fixes: 755/clusterfuzz-testcase-5369072516595712 > > > > See: [FFmpeg-devel] [PATCH 1/2] avcodec/h264_direct: Fix runtime error: signed integer overflow: 2147483647 - -14133 cannot be represented in type 'int' > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/h264_direct.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/h264_direct.c b/libavcodec/h264_direct.c > > index cbb84665b3..66e54479d1 100644 > > --- a/libavcodec/h264_direct.c > > +++ b/libavcodec/h264_direct.c > > @@ -39,7 +39,12 @@ static int get_scale_factor(H264SliceContext *sl, > > int poc, int poc1, int i) > > { > > int poc0 = sl->ref_list[0][i].poc; > > - int td = av_clip_int8(poc1 - poc0); > > + int64_t pocdiff = poc1 - (int64_t)poc0; > > + int td = av_clip_int8(pocdiff); > > + > > + if (pocdiff != (int)pocdiff) > > + avpriv_request_sample(sl->h264->avctx, "pocdiff overflow\n"); > > + > > if (td == 0 || sl->ref_list[0][i].parent->long_ref) { > > return 256; > > } else { > > Hard to image that these poc values aren't bounded by something else, > but I don't know. > > Also the previous patch didn't have this request_sample call, which > inflates this whole thing by 5 lines of code. yes thats why i suggested it initially. SUINT allows overflow detection simply by #define CHECKED 1 and running under ubsan otherwise an excplicit check is needed to detect such occurances [...]
On Sat, 11 Mar 2017 01:26:33 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Mar 10, 2017 at 04:01:36PM +0100, wm4 wrote: > > On Fri, 10 Mar 2017 15:24:52 +0100 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > Fixes: 755/clusterfuzz-testcase-5369072516595712 > > > > > > See: [FFmpeg-devel] [PATCH 1/2] avcodec/h264_direct: Fix runtime error: signed integer overflow: 2147483647 - -14133 cannot be represented in type 'int' > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/h264_direct.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/libavcodec/h264_direct.c b/libavcodec/h264_direct.c > > > index cbb84665b3..66e54479d1 100644 > > > --- a/libavcodec/h264_direct.c > > > +++ b/libavcodec/h264_direct.c > > > @@ -39,7 +39,12 @@ static int get_scale_factor(H264SliceContext *sl, > > > int poc, int poc1, int i) > > > { > > > int poc0 = sl->ref_list[0][i].poc; > > > - int td = av_clip_int8(poc1 - poc0); > > > + int64_t pocdiff = poc1 - (int64_t)poc0; > > > + int td = av_clip_int8(pocdiff); > > > + > > > + if (pocdiff != (int)pocdiff) > > > + avpriv_request_sample(sl->h264->avctx, "pocdiff overflow\n"); > > > + > > > if (td == 0 || sl->ref_list[0][i].parent->long_ref) { > > > return 256; > > > } else { > > > > Hard to image that these poc values aren't bounded by something else, > > but I don't know. > > > > > Also the previous patch didn't have this request_sample call, which > > inflates this whole thing by 5 lines of code. > > yes thats why i suggested it initially. > SUINT allows overflow detection simply by #define CHECKED 1 > and running under ubsan > > otherwise an excplicit check is needed to detect such occurances You can either 1. ignore the error in some way that doesn't cause problems 2. ignore the error in some way that doesn't cause problems in debug mode 3. make the error explicit and log it Your first patch did 2 (which I find questionable, btw.), your current patch does 3 - why not do 1? That solution seems safest and has the smallest footprint.
On Sat, Mar 11, 2017 at 02:04:25PM +0100, wm4 wrote: > On Sat, 11 Mar 2017 01:26:33 +0100 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Fri, Mar 10, 2017 at 04:01:36PM +0100, wm4 wrote: > > > On Fri, 10 Mar 2017 15:24:52 +0100 > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > Fixes: 755/clusterfuzz-testcase-5369072516595712 > > > > > > > > See: [FFmpeg-devel] [PATCH 1/2] avcodec/h264_direct: Fix runtime error: signed integer overflow: 2147483647 - -14133 cannot be represented in type 'int' > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavcodec/h264_direct.c | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/libavcodec/h264_direct.c b/libavcodec/h264_direct.c > > > > index cbb84665b3..66e54479d1 100644 > > > > --- a/libavcodec/h264_direct.c > > > > +++ b/libavcodec/h264_direct.c > > > > @@ -39,7 +39,12 @@ static int get_scale_factor(H264SliceContext *sl, > > > > int poc, int poc1, int i) > > > > { > > > > int poc0 = sl->ref_list[0][i].poc; > > > > - int td = av_clip_int8(poc1 - poc0); > > > > + int64_t pocdiff = poc1 - (int64_t)poc0; > > > > + int td = av_clip_int8(pocdiff); > > > > + > > > > + if (pocdiff != (int)pocdiff) > > > > + avpriv_request_sample(sl->h264->avctx, "pocdiff overflow\n"); > > > > + > > > > if (td == 0 || sl->ref_list[0][i].parent->long_ref) { > > > > return 256; > > > > } else { > > > > > > Hard to image that these poc values aren't bounded by something else, > > > but I don't know. > > > > > > > > Also the previous patch didn't have this request_sample call, which > > > inflates this whole thing by 5 lines of code. > > > > yes thats why i suggested it initially. > > SUINT allows overflow detection simply by #define CHECKED 1 > > and running under ubsan > > > > otherwise an excplicit check is needed to detect such occurances > > You can either > 1. ignore the error in some way that doesn't cause problems > 2. ignore the error in some way that doesn't cause problems in debug > mode > 3. make the error explicit and log it > > Your first patch did 2 (which I find questionable, btw.), your current My first patch should have done 1, why do you think it does not? > patch does 3 - why not do 1? That solution seems safest and has the > smallest footprint. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Sat, 11 Mar 2017 14:50:42 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sat, Mar 11, 2017 at 02:04:25PM +0100, wm4 wrote: > > On Sat, 11 Mar 2017 01:26:33 +0100 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Fri, Mar 10, 2017 at 04:01:36PM +0100, wm4 wrote: > > > > On Fri, 10 Mar 2017 15:24:52 +0100 > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > Fixes: 755/clusterfuzz-testcase-5369072516595712 > > > > > > > > > > See: [FFmpeg-devel] [PATCH 1/2] avcodec/h264_direct: Fix runtime error: signed integer overflow: 2147483647 - -14133 cannot be represented in type 'int' > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > --- > > > > > libavcodec/h264_direct.c | 7 ++++++- > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/libavcodec/h264_direct.c b/libavcodec/h264_direct.c > > > > > index cbb84665b3..66e54479d1 100644 > > > > > --- a/libavcodec/h264_direct.c > > > > > +++ b/libavcodec/h264_direct.c > > > > > @@ -39,7 +39,12 @@ static int get_scale_factor(H264SliceContext *sl, > > > > > int poc, int poc1, int i) > > > > > { > > > > > int poc0 = sl->ref_list[0][i].poc; > > > > > - int td = av_clip_int8(poc1 - poc0); > > > > > + int64_t pocdiff = poc1 - (int64_t)poc0; > > > > > + int td = av_clip_int8(pocdiff); > > > > > + > > > > > + if (pocdiff != (int)pocdiff) > > > > > + avpriv_request_sample(sl->h264->avctx, "pocdiff overflow\n"); > > > > > + > > > > > if (td == 0 || sl->ref_list[0][i].parent->long_ref) { > > > > > return 256; > > > > > } else { > > > > > > > > Hard to image that these poc values aren't bounded by something else, > > > > but I don't know. > > > > > > > > > > > Also the previous patch didn't have this request_sample call, which > > > > inflates this whole thing by 5 lines of code. > > > > > > yes thats why i suggested it initially. > > > SUINT allows overflow detection simply by #define CHECKED 1 > > > and running under ubsan > > > > > > otherwise an excplicit check is needed to detect such occurances > > > > You can either > > 1. ignore the error in some way that doesn't cause problems > > 2. ignore the error in some way that doesn't cause problems in debug > > mode > > 3. make the error explicit and log it > > > > Your first patch did 2 (which I find questionable, btw.), your current > > My first patch should have done 1, why do you think it does not? Well, it still allows the signed overflow, but only in release mode. Or do you want this patch only to make ubsan happy? (If it's not UB, why does ubsan warn?)
On Sat, Mar 11, 2017 at 05:46:31PM +0100, wm4 wrote: > On Sat, 11 Mar 2017 14:50:42 +0100 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Sat, Mar 11, 2017 at 02:04:25PM +0100, wm4 wrote: > > > On Sat, 11 Mar 2017 01:26:33 +0100 > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > On Fri, Mar 10, 2017 at 04:01:36PM +0100, wm4 wrote: > > > > > On Fri, 10 Mar 2017 15:24:52 +0100 > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > > > Fixes: 755/clusterfuzz-testcase-5369072516595712 > > > > > > > > > > > > See: [FFmpeg-devel] [PATCH 1/2] avcodec/h264_direct: Fix runtime error: signed integer overflow: 2147483647 - -14133 cannot be represented in type 'int' > > > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > --- > > > > > > libavcodec/h264_direct.c | 7 ++++++- > > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/libavcodec/h264_direct.c b/libavcodec/h264_direct.c > > > > > > index cbb84665b3..66e54479d1 100644 > > > > > > --- a/libavcodec/h264_direct.c > > > > > > +++ b/libavcodec/h264_direct.c > > > > > > @@ -39,7 +39,12 @@ static int get_scale_factor(H264SliceContext *sl, > > > > > > int poc, int poc1, int i) > > > > > > { > > > > > > int poc0 = sl->ref_list[0][i].poc; > > > > > > - int td = av_clip_int8(poc1 - poc0); > > > > > > + int64_t pocdiff = poc1 - (int64_t)poc0; > > > > > > + int td = av_clip_int8(pocdiff); > > > > > > + > > > > > > + if (pocdiff != (int)pocdiff) > > > > > > + avpriv_request_sample(sl->h264->avctx, "pocdiff overflow\n"); > > > > > > + > > > > > > if (td == 0 || sl->ref_list[0][i].parent->long_ref) { > > > > > > return 256; > > > > > > } else { > > > > > > > > > > Hard to image that these poc values aren't bounded by something else, > > > > > but I don't know. > > > > > > > > > > > > > > Also the previous patch didn't have this request_sample call, which > > > > > inflates this whole thing by 5 lines of code. > > > > > > > > yes thats why i suggested it initially. > > > > SUINT allows overflow detection simply by #define CHECKED 1 > > > > and running under ubsan > > > > > > > > otherwise an excplicit check is needed to detect such occurances > > > > > > You can either > > > 1. ignore the error in some way that doesn't cause problems > > > 2. ignore the error in some way that doesn't cause problems in debug > > > mode > > > 3. make the error explicit and log it > > > > > > Your first patch did 2 (which I find questionable, btw.), your current > > > > My first patch should have done 1, why do you think it does not? > > Well, it still allows the signed overflow, but only in release mode. Or i think you misread the code, the signed overflow is only possible when CHECKED is enabled, its not enabled in release mode. It is enabled in DEBUG mode so ubsan can be used to find such overflows easily while there is no undefined behavior normally or in any default build. > do you want this patch only to make ubsan happy? (If it's not UB, why > does ubsan warn?) [...]
On Sat, 11 Mar 2017 18:26:48 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sat, Mar 11, 2017 at 05:46:31PM +0100, wm4 wrote: > > On Sat, 11 Mar 2017 14:50:42 +0100 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Sat, Mar 11, 2017 at 02:04:25PM +0100, wm4 wrote: > > > > On Sat, 11 Mar 2017 01:26:33 +0100 > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > On Fri, Mar 10, 2017 at 04:01:36PM +0100, wm4 wrote: > > > > > > On Fri, 10 Mar 2017 15:24:52 +0100 > > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > > > > > Fixes: 755/clusterfuzz-testcase-5369072516595712 > > > > > > > > > > > > > > See: [FFmpeg-devel] [PATCH 1/2] avcodec/h264_direct: Fix runtime error: signed integer overflow: 2147483647 - -14133 cannot be represented in type 'int' > > > > > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > > --- > > > > > > > libavcodec/h264_direct.c | 7 ++++++- > > > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/libavcodec/h264_direct.c b/libavcodec/h264_direct.c > > > > > > > index cbb84665b3..66e54479d1 100644 > > > > > > > --- a/libavcodec/h264_direct.c > > > > > > > +++ b/libavcodec/h264_direct.c > > > > > > > @@ -39,7 +39,12 @@ static int get_scale_factor(H264SliceContext *sl, > > > > > > > int poc, int poc1, int i) > > > > > > > { > > > > > > > int poc0 = sl->ref_list[0][i].poc; > > > > > > > - int td = av_clip_int8(poc1 - poc0); > > > > > > > + int64_t pocdiff = poc1 - (int64_t)poc0; > > > > > > > + int td = av_clip_int8(pocdiff); > > > > > > > + > > > > > > > + if (pocdiff != (int)pocdiff) > > > > > > > + avpriv_request_sample(sl->h264->avctx, "pocdiff overflow\n"); > > > > > > > + > > > > > > > if (td == 0 || sl->ref_list[0][i].parent->long_ref) { > > > > > > > return 256; > > > > > > > } else { > > > > > > > > > > > > Hard to image that these poc values aren't bounded by something else, > > > > > > but I don't know. > > > > > > > > > > > > > > > > > Also the previous patch didn't have this request_sample call, which > > > > > > inflates this whole thing by 5 lines of code. > > > > > > > > > > yes thats why i suggested it initially. > > > > > SUINT allows overflow detection simply by #define CHECKED 1 > > > > > and running under ubsan > > > > > > > > > > otherwise an excplicit check is needed to detect such occurances > > > > > > > > You can either > > > > 1. ignore the error in some way that doesn't cause problems > > > > 2. ignore the error in some way that doesn't cause problems in debug > > > > mode > > > > 3. make the error explicit and log it > > > > > > > > Your first patch did 2 (which I find questionable, btw.), your current > > > > > > My first patch should have done 1, why do you think it does not? > > > > Well, it still allows the signed overflow, but only in release mode. Or > > i think you misread the code, the signed overflow is only possible > when CHECKED is enabled, its not enabled in release mode. > It is enabled in DEBUG mode so ubsan can be used to find such overflows > easily while there is no undefined behavior normally or in any default > build. Oh I see. Makes the while thing even stranger. > > > > do you want this patch only to make ubsan happy? (If it's not UB, why > > does ubsan warn?) > > [...]
On Mon, Mar 13, 2017 at 11:39:05AM +0100, wm4 wrote: > On Sat, 11 Mar 2017 18:26:48 +0100 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Sat, Mar 11, 2017 at 05:46:31PM +0100, wm4 wrote: > > > On Sat, 11 Mar 2017 14:50:42 +0100 > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > On Sat, Mar 11, 2017 at 02:04:25PM +0100, wm4 wrote: > > > > > On Sat, 11 Mar 2017 01:26:33 +0100 > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > > > On Fri, Mar 10, 2017 at 04:01:36PM +0100, wm4 wrote: > > > > > > > On Fri, 10 Mar 2017 15:24:52 +0100 > > > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > > > > > > > Fixes: 755/clusterfuzz-testcase-5369072516595712 > > > > > > > > > > > > > > > > See: [FFmpeg-devel] [PATCH 1/2] avcodec/h264_direct: Fix runtime error: signed integer overflow: 2147483647 - -14133 cannot be represented in type 'int' > > > > > > > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > > > --- > > > > > > > > libavcodec/h264_direct.c | 7 ++++++- > > > > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/libavcodec/h264_direct.c b/libavcodec/h264_direct.c > > > > > > > > index cbb84665b3..66e54479d1 100644 > > > > > > > > --- a/libavcodec/h264_direct.c > > > > > > > > +++ b/libavcodec/h264_direct.c > > > > > > > > @@ -39,7 +39,12 @@ static int get_scale_factor(H264SliceContext *sl, > > > > > > > > int poc, int poc1, int i) > > > > > > > > { > > > > > > > > int poc0 = sl->ref_list[0][i].poc; > > > > > > > > - int td = av_clip_int8(poc1 - poc0); > > > > > > > > + int64_t pocdiff = poc1 - (int64_t)poc0; > > > > > > > > + int td = av_clip_int8(pocdiff); > > > > > > > > + > > > > > > > > + if (pocdiff != (int)pocdiff) > > > > > > > > + avpriv_request_sample(sl->h264->avctx, "pocdiff overflow\n"); > > > > > > > > + > > > > > > > > if (td == 0 || sl->ref_list[0][i].parent->long_ref) { > > > > > > > > return 256; > > > > > > > > } else { > > > > > > > > > > > > > > Hard to image that these poc values aren't bounded by something else, > > > > > > > but I don't know. > > > > > > > > > > > > > > > > > > > > Also the previous patch didn't have this request_sample call, which > > > > > > > inflates this whole thing by 5 lines of code. > > > > > > > > > > > > yes thats why i suggested it initially. > > > > > > SUINT allows overflow detection simply by #define CHECKED 1 > > > > > > and running under ubsan > > > > > > > > > > > > otherwise an excplicit check is needed to detect such occurances > > > > > > > > > > You can either > > > > > 1. ignore the error in some way that doesn't cause problems > > > > > 2. ignore the error in some way that doesn't cause problems in debug > > > > > mode > > > > > 3. make the error explicit and log it > > > > > > > > > > Your first patch did 2 (which I find questionable, btw.), your current > > > > > > > > My first patch should have done 1, why do you think it does not? > > > > > > Well, it still allows the signed overflow, but only in release mode. Or > > > > i think you misread the code, the signed overflow is only possible > > when CHECKED is enabled, its not enabled in release mode. > > It is enabled in DEBUG mode so ubsan can be used to find such overflows > > easily while there is no undefined behavior normally or in any default > > build. > > Oh I see. Makes the while thing even stranger. can i apply one of the 2 patches or do you object? if you dont object, which one do you prefer ? [...]
On Mon, 13 Mar 2017 16:02:43 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, Mar 13, 2017 at 11:39:05AM +0100, wm4 wrote: > > On Sat, 11 Mar 2017 18:26:48 +0100 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > On Sat, Mar 11, 2017 at 05:46:31PM +0100, wm4 wrote: > > > > On Sat, 11 Mar 2017 14:50:42 +0100 > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > On Sat, Mar 11, 2017 at 02:04:25PM +0100, wm4 wrote: > > > > > > On Sat, 11 Mar 2017 01:26:33 +0100 > > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > > > > > On Fri, Mar 10, 2017 at 04:01:36PM +0100, wm4 wrote: > > > > > > > > On Fri, 10 Mar 2017 15:24:52 +0100 > > > > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > > > > > > > > > Fixes: 755/clusterfuzz-testcase-5369072516595712 > > > > > > > > > > > > > > > > > > See: [FFmpeg-devel] [PATCH 1/2] avcodec/h264_direct: Fix runtime error: signed integer overflow: 2147483647 - -14133 cannot be represented in type 'int' > > > > > > > > > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > > > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > > > > --- > > > > > > > > > libavcodec/h264_direct.c | 7 ++++++- > > > > > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > diff --git a/libavcodec/h264_direct.c b/libavcodec/h264_direct.c > > > > > > > > > index cbb84665b3..66e54479d1 100644 > > > > > > > > > --- a/libavcodec/h264_direct.c > > > > > > > > > +++ b/libavcodec/h264_direct.c > > > > > > > > > @@ -39,7 +39,12 @@ static int get_scale_factor(H264SliceContext *sl, > > > > > > > > > int poc, int poc1, int i) > > > > > > > > > { > > > > > > > > > int poc0 = sl->ref_list[0][i].poc; > > > > > > > > > - int td = av_clip_int8(poc1 - poc0); > > > > > > > > > + int64_t pocdiff = poc1 - (int64_t)poc0; > > > > > > > > > + int td = av_clip_int8(pocdiff); > > > > > > > > > + > > > > > > > > > + if (pocdiff != (int)pocdiff) > > > > > > > > > + avpriv_request_sample(sl->h264->avctx, "pocdiff overflow\n"); > > > > > > > > > + > > > > > > > > > if (td == 0 || sl->ref_list[0][i].parent->long_ref) { > > > > > > > > > return 256; > > > > > > > > > } else { > > > > > > > > > > > > > > > > Hard to image that these poc values aren't bounded by something else, > > > > > > > > but I don't know. > > > > > > > > > > > > > > > > > > > > > > > Also the previous patch didn't have this request_sample call, which > > > > > > > > inflates this whole thing by 5 lines of code. > > > > > > > > > > > > > > yes thats why i suggested it initially. > > > > > > > SUINT allows overflow detection simply by #define CHECKED 1 > > > > > > > and running under ubsan > > > > > > > > > > > > > > otherwise an excplicit check is needed to detect such occurances > > > > > > > > > > > > You can either > > > > > > 1. ignore the error in some way that doesn't cause problems > > > > > > 2. ignore the error in some way that doesn't cause problems in debug > > > > > > mode > > > > > > 3. make the error explicit and log it > > > > > > > > > > > > Your first patch did 2 (which I find questionable, btw.), your current > > > > > > > > > > My first patch should have done 1, why do you think it does not? > > > > > > > > Well, it still allows the signed overflow, but only in release mode. Or > > > > > > i think you misread the code, the signed overflow is only possible > > > when CHECKED is enabled, its not enabled in release mode. > > > It is enabled in DEBUG mode so ubsan can be used to find such overflows > > > easily while there is no undefined behavior normally or in any default > > > build. > > > > Oh I see. Makes the while thing even stranger. > > can i apply one of the 2 patches or do you object? > if you dont object, which one do you prefer ? I think the second patch is preferable, if it's between those 2. I think it could be improved, but don't want to hold you back further.
On Mon, Mar 13, 2017 at 10:26:25PM +0100, wm4 wrote: > On Mon, 13 Mar 2017 16:02:43 +0100 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Mon, Mar 13, 2017 at 11:39:05AM +0100, wm4 wrote: > > > On Sat, 11 Mar 2017 18:26:48 +0100 > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > On Sat, Mar 11, 2017 at 05:46:31PM +0100, wm4 wrote: > > > > > On Sat, 11 Mar 2017 14:50:42 +0100 > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > > > On Sat, Mar 11, 2017 at 02:04:25PM +0100, wm4 wrote: > > > > > > > On Sat, 11 Mar 2017 01:26:33 +0100 > > > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > > > > > > > On Fri, Mar 10, 2017 at 04:01:36PM +0100, wm4 wrote: > > > > > > > > > On Fri, 10 Mar 2017 15:24:52 +0100 > > > > > > > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > > > > > > > > > > > > > Fixes: 755/clusterfuzz-testcase-5369072516595712 > > > > > > > > > > > > > > > > > > > > See: [FFmpeg-devel] [PATCH 1/2] avcodec/h264_direct: Fix runtime error: signed integer overflow: 2147483647 - -14133 cannot be represented in type 'int' > > > > > > > > > > > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg > > > > > > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > > > > > > --- > > > > > > > > > > libavcodec/h264_direct.c | 7 ++++++- > > > > > > > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > diff --git a/libavcodec/h264_direct.c b/libavcodec/h264_direct.c > > > > > > > > > > index cbb84665b3..66e54479d1 100644 > > > > > > > > > > --- a/libavcodec/h264_direct.c > > > > > > > > > > +++ b/libavcodec/h264_direct.c > > > > > > > > > > @@ -39,7 +39,12 @@ static int get_scale_factor(H264SliceContext *sl, > > > > > > > > > > int poc, int poc1, int i) > > > > > > > > > > { > > > > > > > > > > int poc0 = sl->ref_list[0][i].poc; > > > > > > > > > > - int td = av_clip_int8(poc1 - poc0); > > > > > > > > > > + int64_t pocdiff = poc1 - (int64_t)poc0; > > > > > > > > > > + int td = av_clip_int8(pocdiff); > > > > > > > > > > + > > > > > > > > > > + if (pocdiff != (int)pocdiff) > > > > > > > > > > + avpriv_request_sample(sl->h264->avctx, "pocdiff overflow\n"); > > > > > > > > > > + > > > > > > > > > > if (td == 0 || sl->ref_list[0][i].parent->long_ref) { > > > > > > > > > > return 256; > > > > > > > > > > } else { > > > > > > > > > > > > > > > > > > Hard to image that these poc values aren't bounded by something else, > > > > > > > > > but I don't know. > > > > > > > > > > > > > > > > > > > > > > > > > > Also the previous patch didn't have this request_sample call, which > > > > > > > > > inflates this whole thing by 5 lines of code. > > > > > > > > > > > > > > > > yes thats why i suggested it initially. > > > > > > > > SUINT allows overflow detection simply by #define CHECKED 1 > > > > > > > > and running under ubsan > > > > > > > > > > > > > > > > otherwise an excplicit check is needed to detect such occurances > > > > > > > > > > > > > > You can either > > > > > > > 1. ignore the error in some way that doesn't cause problems > > > > > > > 2. ignore the error in some way that doesn't cause problems in debug > > > > > > > mode > > > > > > > 3. make the error explicit and log it > > > > > > > > > > > > > > Your first patch did 2 (which I find questionable, btw.), your current > > > > > > > > > > > > My first patch should have done 1, why do you think it does not? > > > > > > > > > > Well, it still allows the signed overflow, but only in release mode. Or > > > > > > > > i think you misread the code, the signed overflow is only possible > > > > when CHECKED is enabled, its not enabled in release mode. > > > > It is enabled in DEBUG mode so ubsan can be used to find such overflows > > > > easily while there is no undefined behavior normally or in any default > > > > build. > > > > > > Oh I see. Makes the while thing even stranger. > > > > can i apply one of the 2 patches or do you object? > > if you dont object, which one do you prefer ? > > I think the second patch is preferable, if it's between those 2. I > think it could be improved, but don't want to hold you back further. locally applied thanks [...]
diff --git a/libavcodec/h264_direct.c b/libavcodec/h264_direct.c index cbb84665b3..66e54479d1 100644 --- a/libavcodec/h264_direct.c +++ b/libavcodec/h264_direct.c @@ -39,7 +39,12 @@ static int get_scale_factor(H264SliceContext *sl, int poc, int poc1, int i) { int poc0 = sl->ref_list[0][i].poc; - int td = av_clip_int8(poc1 - poc0); + int64_t pocdiff = poc1 - (int64_t)poc0; + int td = av_clip_int8(pocdiff); + + if (pocdiff != (int)pocdiff) + avpriv_request_sample(sl->h264->avctx, "pocdiff overflow\n"); + if (td == 0 || sl->ref_list[0][i].parent->long_ref) { return 256; } else {
Fixes: 755/clusterfuzz-testcase-5369072516595712 See: [FFmpeg-devel] [PATCH 1/2] avcodec/h264_direct: Fix runtime error: signed integer overflow: 2147483647 - -14133 cannot be represented in type 'int' Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/h264_direct.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)