On 7/9/17, Ronald S. Bultje <rsbul...@gmail.com> wrote: > Hi, > > On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer <mich...@niedermayer.cc> > wrote: > >> On Mon, Jul 03, 2017 at 01:37:09AM +0200, Michael Niedermayer wrote: >> > On Sun, Jul 02, 2017 at 02:24:53PM +0100, Rostislav Pehlivanov wrote: >> > > On 2 July 2017 at 03:28, Michael Niedermayer <mich...@niedermayer.cc> >> wrote: >> > > >> > > > Fixes: runtime error: signed integer overflow: 1965219850 + >> > > > 995792909 >> > > > cannot be represented in type 'int' >> > > > Fixes: part of 2096/clusterfuzz-testcase-minimized-4901566068817920 >> > > > >> > > > Found-by: continuous fuzzing process https://github.com/google/oss- >> > > > fuzz/tree/master/projects/ffmpeg >> > > > Signed-off-by >> > > > <https://github.com/google/oss-fuzz/tree/master/projects/ >> ffmpeg%0ASigned-off-by>: >> > > > Michael Niedermayer <mich...@niedermayer.cc> >> > > > --- >> > > > libavcodec/aacpsdsp_template.c | 3 ++- >> > > > 1 file changed, 2 insertions(+), 1 deletion(-) >> > > > >> > > > diff --git a/libavcodec/aacpsdsp_template.c b/libavcodec/aacpsdsp_ >> > > > template.c >> > > > index 9e1a95c1a1..2c0afd4512 100644 >> > > > --- a/libavcodec/aacpsdsp_template.c >> > > > +++ b/libavcodec/aacpsdsp_template.c >> > > > @@ -26,9 +26,10 @@ >> > > > #include "libavutil/attributes.h" >> > > > #include "aacpsdsp.h" >> > > > >> > > > -static void ps_add_squares_c(INTFLOAT *dst, const INTFLOAT >> (*src)[2], int >> > > > n) >> > > > +static void ps_add_squares_c(INTFLOAT *dst_param, const INTFLOAT >> > > > (*src)[2], int n) >> > > > { >> > > > int i; >> > > > + SUINTFLOAT *dst = dst_param; >> > > > for (i = 0; i < n; i++) >> > > > dst[i] += AAC_MADD28(src[i][0], src[i][0], src[i][1], >> src[i][1]); >> > > > } >> > > > >> > > > >> > > What's the issue with just _not_ fixing it here? It only occurs on >> fuzzed >> > > inputs, doesn't crash on any known platform ever, makes the code >> uglier and >> > > why? Because some fuzzer is super pedantic. >> > > Why not fix the fuzzer? Why not just mark this as a false positive, >> since >> > > fixing it is pointless from the standpoint of security (you can't >> exploit >> > > overflows in transforms or functions like this), and all developers >> hate it. >> > >> > Iam not sure you understand the problem. >> > signed integer overflows are undefined behavior, undefined behavior >> > is not allowed in C. >> > >> > >> > Theres also no option to mark anything as false positive. >> > If the tool makes a mistake, the issue needs to be reported to its >> > authors and needs to be fixed. >> > I belive the tool is correct, if someone thinks its not correct, the >> > place to report this to is likely where the clang sanitizers are >> > developed. >> > >> > I do understand that this is annoying but this is how C works. >> > >> > About "doesn't crash on any known platform ever", >> > "you can't exploit overflows in transforms or functions like this" >> > >> > undefined behavior has bitten people with unexpected results in the >> > past. for example "if (a+b < a)" to test for overflow, but because the >> condition >> > can only be true in case of overflow and overflow isnt allowed in C >> > the compiler didnt assemble this the way the human thought. >> > >> > In the case of ps_add_squares_c(), dst[i] has to increase if iam >> > not mistaken. In case of overflow it can decrease but overflow is >> > not allowed so a compiler can prune anything that implies dst[] to be >> > negative. >> > dst[] is used downstream in checks of greater / less and in FFMAX >> > In this code the compiler can assume that the sign bit is clear, >> > what happens when the compilers optimizer has false assumtations >> > i dont know but i know its not good. >> > >> > That said even if no such chain of conditions exist its still invalid >> > code if theres undefined behavior in it >> >> Does anyone object to this patch ? >> Or does anyone have a better idea on how to fix this ? >> if not id like to apply it > > > I think Rostislav's point is: why fix it, if it can only happen with > corrupt input? The before and after situation is identical: garbage in, > garbage out. If the compiler does funny things that makes the garbage > slightly differently bad, is that really so devilishly bad? It's still > garbage. Is anything improved by this?
As I have tried to explain before, you can harden a program by compiling it with `gcc -ftrapv` . In such a program the overflow would trap and in normal case would lead to termination. Do you want your browser to die by a small bit error in a random clip? _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
