| Message ID | 20170702022856.16195-3-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
On 2 July 2017 at 03:28, Michael Niedermayer <michael@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 <michael@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.
On Sun, Jul 02, 2017 at 02:24:53PM +0100, Rostislav Pehlivanov wrote: > On 2 July 2017 at 03:28, Michael Niedermayer <michael@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 <michael@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 [...]
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 <michael@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 <michael@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 [...]
Hi, On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer <michael@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 <michael@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 <michael@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? Ronald
On 09.07.2017, at 02:52, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer <michael@niedermayer.cc> > wrote: > >> >> 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? The way C works, you MUST assume any undefined behaviour can at any point (different compiler, compiler options, ...) become exploitable. You can try to justify it with assumptions (but even that is usually very hard, is and will the value really never be used in a condition affecting, however indirectly, a pointer value for example?), but those are just arbitrary assumptions not backed by any standard. If you don't like that, C is the wrong language to use.
On 7/9/17, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer <michael@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 <michael@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 <michael@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?
Hi, On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > On 09.07.2017, at 02:52, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > > On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer > <michael@niedermayer.cc> > > wrote: > > > >> > >> 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? > > The way C works, you MUST assume any undefined behaviour can at any point > [..] become exploitable.[..] If you don't like that, C is the wrong > language to use. I think I've read "the boy who cried wolf" a few too many times to my kids, but the form of this discussion is currently too polarizing/political for my taste. Ronald
On Sun, Jul 09, 2017 at 10:08:50AM -0400, Ronald S. Bultje wrote: > Hi, > > On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> > wrote: > > > On 09.07.2017, at 02:52, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > > > On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer > > <michael@niedermayer.cc> > > > wrote: > > > > > >> > > >> 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? > > > > The way C works, you MUST assume any undefined behaviour can at any point > > [..] become exploitable.[..] If you don't like that, C is the wrong > > language to use. > > > I think I've read "the boy who cried wolf" a few too many times to my kids, > but the form of this discussion is currently too polarizing/political for > my taste. I dont know about polarizing, it quite possibly is but If anyone belives this is just political or theoretical and its safe to trigger undefined behavior if the output is not used, here are some examples found after 30sec of google use where that is not so: https://blogs.msdn.microsoft.com/oldnewthing/20140627-00/?p=633 https://blog.regehr.org/archives/759 This is intended "for the archives" and not intended to pull anyone into a discussion they dont want to be in. [...]
On 09.07.2017, at 16:08, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > Hi, > > On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> > wrote: > >> On 09.07.2017, at 02:52, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: >>> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer >> <michael@niedermayer.cc> >>> wrote: >>> >>>> >>>> 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? >> >> The way C works, you MUST assume any undefined behaviour can at any point >> [..] become exploitable.[..] If you don't like that, C is the wrong >> language to use. > > > I think I've read "the boy who cried wolf" a few too many times to my kids, > but the form of this discussion is currently too polarizing/political for > my taste. That is my reading of the C standard, is that political or even just controversial? I mean of course you can ignore standards (see MPEG-4 ASP, and in some ways that was actually fairly reasonable thing to do at the time), and I don't fix every undefined behaviour case in my code when I can't think of any reasonable solution. So there is a cost-benefit discussion in principle. I believe the cost of not fixing undefined behaviour, just by virtue of going outside what the standard guarantees should be considered fairly high. That is an opinion, but is there any disagreement that undefined behaviour is at least an issue of some degree? If we can agree on that, then the question would only be how much effort/code ugliness is reasonable. There is also the point (which I hope I mentioned in the parts cut out) that just making sure that these cases are not already exploitable right now with the current compiler can in many cases be quite a pain (and does not have tool support), so I think fixing it would often be the lowest-effort method. I'd also like to point out that even ignoring all that, ubsan + fuzzing is a powerful tool to improve code quality, and I would argue than at least some amount of code complexity is justified just for having them work well. And it's also that to my mind the patch did not look THAT bad...
On 7/9/17, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> > wrote: > >> On 09.07.2017, at 02:52, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: >> > On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer >> <michael@niedermayer.cc> >> > wrote: >> > >> >> >> >> 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? >> >> The way C works, you MUST assume any undefined behaviour can at any point >> [..] become exploitable.[..] If you don't like that, C is the wrong >> language to use. > > > I think I've read "the boy who cried wolf" a few too many times to my kids, > but the form of this discussion is currently too polarizing/political for > my taste. So, you stir the pot and then run away. You ignore technical explanations and you call discussion too political. And you "imply" that developers are lairs. Maybe you are not aware what you are going, but I do assure you, it aint pretty. I think you owe some people an apology.
On Sun, 9 Jul 2017 22:03:21 +0200 Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > On 09.07.2017, at 16:08, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > > Hi, > > > > On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> > > wrote: > > > >> On 09.07.2017, at 02:52, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > >>> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer > >> <michael@niedermayer.cc> > >>> wrote: > >>> > >>>> > >>>> 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? > >> > >> The way C works, you MUST assume any undefined behaviour can at any point > >> [..] become exploitable.[..] If you don't like that, C is the wrong > >> language to use. > > > > > > I think I've read "the boy who cried wolf" a few too many times to my kids, > > but the form of this discussion is currently too polarizing/political for > > my taste. > > That is my reading of the C standard, is that political or even just controversial? > I mean of course you can ignore standards (see MPEG-4 ASP, and in some ways that was actually fairly reasonable thing to do at the time), and I don't fix every undefined behaviour case in my code when I can't think of any reasonable solution. > So there is a cost-benefit discussion in principle. > I believe the cost of not fixing undefined behaviour, just by virtue of going outside what the standard guarantees should be considered fairly high. > That is an opinion, but is there any disagreement that undefined behaviour is at least an issue of some degree? > If we can agree on that, then the question would only be how much effort/code ugliness is reasonable. > There is also the point (which I hope I mentioned in the parts cut out) that just making sure that these cases are not already exploitable right now with the current compiler can in many cases be quite a pain (and does not have tool support), so I think fixing it would often be the lowest-effort method. The controversial thing is actually the SUINT nonsense. A type is either signed or unsigned, but not both incompletely intransparent ways. Michael keeps adding them even though many are against it. "SUINTFLOAT" just tops this. Is it a signed int? Unsigned int? A float? Unsigned float??? Come on, this is a huge load of bullshit. > I'd also like to point out that even ignoring all that, ubsan + fuzzing is a powerful tool to improve code quality, and I would argue than at least some amount of code complexity is justified just for having them work well. > And it's also that to my mind the patch did not look THAT bad... A powerful tool for Michael to churn out patches which make the code look worse and more tricky, which without doubt will lead ot more new bugs some time in the future. Sure, security is good, but at this point I'm even wondering what's the point of this. Realistically, you'll have to sandbox ffmpeg anyway if you want some minimal security.
On 7/10/2017 5:37 AM, wm4 wrote: > On Sun, 9 Jul 2017 22:03:21 +0200 > Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > >> On 09.07.2017, at 16:08, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: >>> Hi, >>> >>> On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> >>> wrote: >>> >>>> On 09.07.2017, at 02:52, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: >>>>> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer >>>> <michael@niedermayer.cc> >>>>> wrote: >>>>> >>>>>> >>>>>> 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? >>>> >>>> The way C works, you MUST assume any undefined behaviour can at any point >>>> [..] become exploitable.[..] If you don't like that, C is the wrong >>>> language to use. >>> >>> >>> I think I've read "the boy who cried wolf" a few too many times to my kids, >>> but the form of this discussion is currently too polarizing/political for >>> my taste. >> >> That is my reading of the C standard, is that political or even just controversial? >> I mean of course you can ignore standards (see MPEG-4 ASP, and in some ways that was actually fairly reasonable thing to do at the time), and I don't fix every undefined behaviour case in my code when I can't think of any reasonable solution. >> So there is a cost-benefit discussion in principle. >> I believe the cost of not fixing undefined behaviour, just by virtue of going outside what the standard guarantees should be considered fairly high. >> That is an opinion, but is there any disagreement that undefined behaviour is at least an issue of some degree? >> If we can agree on that, then the question would only be how much effort/code ugliness is reasonable. >> There is also the point (which I hope I mentioned in the parts cut out) that just making sure that these cases are not already exploitable right now with the current compiler can in many cases be quite a pain (and does not have tool support), so I think fixing it would often be the lowest-effort method. > > The controversial thing is actually the SUINT nonsense. A type is > either signed or unsigned, but not both incompletely intransparent > ways. Michael keeps adding them even though many are against it. > > "SUINTFLOAT" just tops this. Is it a signed int? Unsigned int? A float? > Unsigned float??? SUINTFLOAT is float or an integer (SUINT) depending on how you include the template file. Blame the template crap for all the int variants of decoders (aac, ac3, etc). > > Come on, this is a huge load of bullshit. > >> I'd also like to point out that even ignoring all that, ubsan + fuzzing is a powerful tool to improve code quality, and I would argue than at least some amount of code complexity is justified just for having them work well. >> And it's also that to my mind the patch did not look THAT bad... > > A powerful tool for Michael to churn out patches which make the code > look worse and more tricky, which without doubt will lead ot more new > bugs some time in the future. Sure, security is good, but at this > point I'm even wondering what's the point of this. Realistically, > you'll have to sandbox ffmpeg anyway if you want some minimal security. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Mon, 10 Jul 2017 09:29:31 -0300 James Almer <jamrial@gmail.com> wrote: > On 7/10/2017 5:37 AM, wm4 wrote: > > On Sun, 9 Jul 2017 22:03:21 +0200 > > Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > > > >> On 09.07.2017, at 16:08, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > >>> Hi, > >>> > >>> On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> > >>> wrote: > >>> > >>>> On 09.07.2017, at 02:52, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > >>>>> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer > >>>> <michael@niedermayer.cc> > >>>>> wrote: > >>>>> > >>>>>> > >>>>>> 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? > >>>> > >>>> The way C works, you MUST assume any undefined behaviour can at any point > >>>> [..] become exploitable.[..] If you don't like that, C is the wrong > >>>> language to use. > >>> > >>> > >>> I think I've read "the boy who cried wolf" a few too many times to my kids, > >>> but the form of this discussion is currently too polarizing/political for > >>> my taste. > >> > >> That is my reading of the C standard, is that political or even just controversial? > >> I mean of course you can ignore standards (see MPEG-4 ASP, and in some ways that was actually fairly reasonable thing to do at the time), and I don't fix every undefined behaviour case in my code when I can't think of any reasonable solution. > >> So there is a cost-benefit discussion in principle. > >> I believe the cost of not fixing undefined behaviour, just by virtue of going outside what the standard guarantees should be considered fairly high. > >> That is an opinion, but is there any disagreement that undefined behaviour is at least an issue of some degree? > >> If we can agree on that, then the question would only be how much effort/code ugliness is reasonable. > >> There is also the point (which I hope I mentioned in the parts cut out) that just making sure that these cases are not already exploitable right now with the current compiler can in many cases be quite a pain (and does not have tool support), so I think fixing it would often be the lowest-effort method. > > > > The controversial thing is actually the SUINT nonsense. A type is > > either signed or unsigned, but not both incompletely intransparent > > ways. Michael keeps adding them even though many are against it. > > > > "SUINTFLOAT" just tops this. Is it a signed int? Unsigned int? A float? > > Unsigned float??? > > SUINTFLOAT is float or an integer (SUINT) depending on how you include > the template file. > > Blame the template crap for all the int variants of decoders (aac, ac3, > etc). Yeah, I understand how it came to be, but maybe it's not a good idea to combine the various hacks into more combinations of hacks.
On Mon, Jul 10, 2017 at 10:37:46AM +0200, wm4 wrote: > On Sun, 9 Jul 2017 22:03:21 +0200 > Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > > > On 09.07.2017, at 16:08, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > > > Hi, > > > > > > On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> > > > wrote: > > > > > >> On 09.07.2017, at 02:52, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > > >>> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer > > >> <michael@niedermayer.cc> > > >>> wrote: > > >>> > > >>>> > > >>>> 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? > > >> > > >> The way C works, you MUST assume any undefined behaviour can at any point > > >> [..] become exploitable.[..] If you don't like that, C is the wrong > > >> language to use. > > > > > > > > > I think I've read "the boy who cried wolf" a few too many times to my kids, > > > but the form of this discussion is currently too polarizing/political for > > > my taste. > > > > That is my reading of the C standard, is that political or even just controversial? > > I mean of course you can ignore standards (see MPEG-4 ASP, and in some ways that was actually fairly reasonable thing to do at the time), and I don't fix every undefined behaviour case in my code when I can't think of any reasonable solution. > > So there is a cost-benefit discussion in principle. > > I believe the cost of not fixing undefined behaviour, just by virtue of going outside what the standard guarantees should be considered fairly high. > > That is an opinion, but is there any disagreement that undefined behaviour is at least an issue of some degree? > > If we can agree on that, then the question would only be how much effort/code ugliness is reasonable. > > There is also the point (which I hope I mentioned in the parts cut out) that just making sure that these cases are not already exploitable right now with the current compiler can in many cases be quite a pain (and does not have tool support), so I think fixing it would often be the lowest-effort method. > > The controversial thing is actually the SUINT nonsense. A type is > either signed or unsigned, but not both incompletely intransparent > ways. Michael keeps adding them even though many are against it. It is extreemly rude from you to make this claim. When in fact i try my best to respect every maitainer and authors preferrance and never add it when people object. [...]
On Mon, 10 Jul 2017 23:10:35 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, Jul 10, 2017 at 10:37:46AM +0200, wm4 wrote: > > On Sun, 9 Jul 2017 22:03:21 +0200 > > Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > > > > > On 09.07.2017, at 16:08, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > > > > Hi, > > > > > > > > On Sun, Jul 9, 2017 at 4:39 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> > > > > wrote: > > > > > > > >> On 09.07.2017, at 02:52, "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > > > >>> On Sat, Jul 8, 2017 at 5:17 PM, Michael Niedermayer > > > >> <michael@niedermayer.cc> > > > >>> wrote: > > > >>> > > > >>>> > > > >>>> 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? > > > >> > > > >> The way C works, you MUST assume any undefined behaviour can at any point > > > >> [..] become exploitable.[..] If you don't like that, C is the wrong > > > >> language to use. > > > > > > > > > > > > I think I've read "the boy who cried wolf" a few too many times to my kids, > > > > but the form of this discussion is currently too polarizing/political for > > > > my taste. > > > > > > That is my reading of the C standard, is that political or even just controversial? > > > I mean of course you can ignore standards (see MPEG-4 ASP, and in some ways that was actually fairly reasonable thing to do at the time), and I don't fix every undefined behaviour case in my code when I can't think of any reasonable solution. > > > So there is a cost-benefit discussion in principle. > > > I believe the cost of not fixing undefined behaviour, just by virtue of going outside what the standard guarantees should be considered fairly high. > > > That is an opinion, but is there any disagreement that undefined behaviour is at least an issue of some degree? > > > If we can agree on that, then the question would only be how much effort/code ugliness is reasonable. > > > There is also the point (which I hope I mentioned in the parts cut out) that just making sure that these cases are not already exploitable right now with the current compiler can in many cases be quite a pain (and does not have tool support), so I think fixing it would often be the lowest-effort method. > > > > The controversial thing is actually the SUINT nonsense. A type is > > either signed or unsigned, but not both incompletely intransparent > > > ways. Michael keeps adding them even though many are against it. > > It is extreemly rude from you to make this claim. > When in fact i try my best to respect every maitainer and authors > preferrance and never add it when people object. Do you deny that you keep sending patches that add SUINT usage? Do you deny that you know that certain people don't like SUINT?
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]); }
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: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/aacpsdsp_template.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)