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 thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Its not that you shouldnt use gotos but rather that you should write readable code and code with gotos often but not always is less readable
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel