| Message ID | 20170903102305.8119-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
Hi, On Sun, Sep 3, 2017 at 6:23 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: Timeout > Fixes: 3142/clusterfuzz-testcase-5007853163118592 > > Found-by: continuous fuzzing process https://github.com/google/oss- > fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/snowdec.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/snowdec.c b/libavcodec/snowdec.c > index b74c468ce3..7e07857a44 100644 > --- a/libavcodec/snowdec.c > +++ b/libavcodec/snowdec.c > @@ -183,13 +183,24 @@ static int decode_q_branch(SnowContext *s, int > level, int x, int y){ > int my_context= av_log2(2*FFABS(left->my - top->my)) + > 0*av_log2(2*FFABS(tr->my - top->my)); > > type= get_rac(&s->c, &s->block_state[1 + left->type + top->type]) > ? BLOCK_INTRA : 0; > - > if(type){ > + int ld, cbd, crd; > pred_mv(s, &mx, &my, 0, left, top, tr); > - l += get_symbol(&s->c, &s->block_state[32], 1); > + ld = get_symbol(&s->c, &s->block_state[32], 1); > + if (ld < -255 || ld > 255) { > + av_log(s->avctx, AV_LOG_ERROR, "Invalid ld %d\n", ld); > + return AVERROR_INVALIDDATA; > + } > + l += ld; > if (s->nb_planes > 2) { > - cb+= get_symbol(&s->c, &s->block_state[64], 1); > - cr+= get_symbol(&s->c, &s->block_state[96], 1); > + cbd = get_symbol(&s->c, &s->block_state[64], 1); > + crd = get_symbol(&s->c, &s->block_state[96], 1); > + if (cbd < -255 || cbd > 255 || crd < -255 || crd > 255) { > + av_log(s->avctx, AV_LOG_ERROR, "Invalid cbd %d, crd > %d\n", cbd, crd); > + return AVERROR_INVALIDDATA; > + } > + cb += cbd; > + cr += crd; > } Can you elaborate on how these error messages, which are displayed to the user by default, help the user resolve the likely-to-occur-with-realworld-files situation where a validly-created file doesn't play back? If any part of this sentence is not true, then why is there a message here? Ronald
On 9/3/2017 10:49 AM, Ronald S. Bultje wrote: > Hi, > > On Sun, Sep 3, 2017 at 6:23 AM, Michael Niedermayer <michael@niedermayer.cc> > wrote: > >> Fixes: Timeout >> Fixes: 3142/clusterfuzz-testcase-5007853163118592 >> >> Found-by: continuous fuzzing process https://github.com/google/oss- >> fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/snowdec.c | 19 +++++++++++++++---- >> 1 file changed, 15 insertions(+), 4 deletions(-) >> >> diff --git a/libavcodec/snowdec.c b/libavcodec/snowdec.c >> index b74c468ce3..7e07857a44 100644 >> --- a/libavcodec/snowdec.c >> +++ b/libavcodec/snowdec.c >> @@ -183,13 +183,24 @@ static int decode_q_branch(SnowContext *s, int >> level, int x, int y){ >> int my_context= av_log2(2*FFABS(left->my - top->my)) + >> 0*av_log2(2*FFABS(tr->my - top->my)); >> >> type= get_rac(&s->c, &s->block_state[1 + left->type + top->type]) >> ? BLOCK_INTRA : 0; >> - >> if(type){ >> + int ld, cbd, crd; >> pred_mv(s, &mx, &my, 0, left, top, tr); >> - l += get_symbol(&s->c, &s->block_state[32], 1); >> + ld = get_symbol(&s->c, &s->block_state[32], 1); >> + if (ld < -255 || ld > 255) { >> + av_log(s->avctx, AV_LOG_ERROR, "Invalid ld %d\n", ld); >> + return AVERROR_INVALIDDATA; >> + } >> + l += ld; >> if (s->nb_planes > 2) { >> - cb+= get_symbol(&s->c, &s->block_state[64], 1); >> - cr+= get_symbol(&s->c, &s->block_state[96], 1); >> + cbd = get_symbol(&s->c, &s->block_state[64], 1); >> + crd = get_symbol(&s->c, &s->block_state[96], 1); >> + if (cbd < -255 || cbd > 255 || crd < -255 || crd > 255) { >> + av_log(s->avctx, AV_LOG_ERROR, "Invalid cbd %d, crd >> %d\n", cbd, crd); >> + return AVERROR_INVALIDDATA; >> + } >> + cb += cbd; >> + cr += crd; >> } > > > Can you elaborate on how these error messages, which are displayed to the > user by default, help the user resolve the > likely-to-occur-with-realworld-files situation where a validly-created file > doesn't play back? > > If any part of this sentence is not true, then why is there a message here? > > Ronald Just go straight to the point, please. This fuzzing commit set in the past few months has been way more controversial than it has any right to. Michael: Don't add error messages at any level above debug if they are completely useless and unhelpful for non-developers. And Consider using ff_tlog() as well, so they don't become binary bloat on a non-debug build.
On Sun, Sep 03, 2017 at 11:43:54AM -0300, James Almer wrote: > On 9/3/2017 10:49 AM, Ronald S. Bultje wrote: > > Hi, > > > > On Sun, Sep 3, 2017 at 6:23 AM, Michael Niedermayer <michael@niedermayer.cc> > > wrote: > > > >> Fixes: Timeout > >> Fixes: 3142/clusterfuzz-testcase-5007853163118592 > >> > >> Found-by: continuous fuzzing process https://github.com/google/oss- > >> fuzz/tree/master/projects/ffmpeg > >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> --- > >> libavcodec/snowdec.c | 19 +++++++++++++++---- > >> 1 file changed, 15 insertions(+), 4 deletions(-) > >> > >> diff --git a/libavcodec/snowdec.c b/libavcodec/snowdec.c > >> index b74c468ce3..7e07857a44 100644 > >> --- a/libavcodec/snowdec.c > >> +++ b/libavcodec/snowdec.c > >> @@ -183,13 +183,24 @@ static int decode_q_branch(SnowContext *s, int > >> level, int x, int y){ > >> int my_context= av_log2(2*FFABS(left->my - top->my)) + > >> 0*av_log2(2*FFABS(tr->my - top->my)); > >> > >> type= get_rac(&s->c, &s->block_state[1 + left->type + top->type]) > >> ? BLOCK_INTRA : 0; > >> - > >> if(type){ > >> + int ld, cbd, crd; > >> pred_mv(s, &mx, &my, 0, left, top, tr); > >> - l += get_symbol(&s->c, &s->block_state[32], 1); > >> + ld = get_symbol(&s->c, &s->block_state[32], 1); > >> + if (ld < -255 || ld > 255) { > >> + av_log(s->avctx, AV_LOG_ERROR, "Invalid ld %d\n", ld); > >> + return AVERROR_INVALIDDATA; > >> + } > >> + l += ld; > >> if (s->nb_planes > 2) { > >> - cb+= get_symbol(&s->c, &s->block_state[64], 1); > >> - cr+= get_symbol(&s->c, &s->block_state[96], 1); > >> + cbd = get_symbol(&s->c, &s->block_state[64], 1); > >> + crd = get_symbol(&s->c, &s->block_state[96], 1); > >> + if (cbd < -255 || cbd > 255 || crd < -255 || crd > 255) { > >> + av_log(s->avctx, AV_LOG_ERROR, "Invalid cbd %d, crd > >> %d\n", cbd, crd); > >> + return AVERROR_INVALIDDATA; > >> + } > >> + cb += cbd; > >> + cr += crd; > >> } > > > > > > Can you elaborate on how these error messages, which are displayed to the > > user by default, help the user resolve the > > likely-to-occur-with-realworld-files situation where a validly-created file > > doesn't play back? > > > > If any part of this sentence is not true, then why is there a message here? > > > > Ronald > > Just go straight to the point, please. This fuzzing commit set in the > past few months has been way more controversial than it has any right to. > > Michael: Don't add error messages at any level above debug if they are > completely useless and unhelpful for non-developers. And Consider using We have always printed error messages for the case that an error occured. Its unprofessional to fail decoding a file but not provide any hint as to why a failure occured. If we remove all error messages and just print a generic "failed decoding header" or "failed to decode frame" We would leave users wondering about each error and we would no longer have differentiated bug reports. There would be one very huge bug report about "Error while decoding stream #0:0: Invalid data found when processing input" Because thats all detail the user can get if the message is not in the binary. That bug report then would be marked invalid of course and would help neither users nor developers. Maybe it would be one such report per codec but thats still rather useless. Or there would be one report for every single file with dozends of duplicates, again not what we want. We want the user to do most of the bug reporting work not use the limited developer manpower. Iam not sure why error messages are since about a year or so considered controversial by some developers but not before And why this is directed toward some types of changes and some parts of the codebase but not others. You can look at ronalds vp9*c It containas in fact more error messages than snow does. git grep AV_LOG_ERROR libavcodec/vp9*c vs. git grep AV_LOG_ERROR libavcodec/snow*c These error messages do not substantially differ in what they are about. Its values being out of supported or valid range, allocation failures, and so forth. Same as also this patch here If the community as a whole prefers not to add detailed error messages and just print something like the current generic "Error while decoding stream #0:0: Invalid data found when processing input" Then this should be properly agreed upon and documeted and enforced in everyones code and not just 10% of the changes. Also we may in this case need a team of people doing "bug report analysis and sorting" as users can then no longer group/guess issues similarity based on detailed error messages. It also makes debuging harder as there is less information initially available in a report. And users hitting an issue also cant google with the error message and for example find that it was fixed in a newer version > ff_tlog() as well, so they don't become binary bloat on a non-debug build. I belive english text is negligible size wise in the binaries am i missing something ? Our code is already quite terse and devoid of comments and documentation, error messages do not seem a exception here [...]
On Mon, 4 Sep 2017 at 00:56 Michael Niedermayer <michael@niedermayer.cc> wrote: > We have always printed error messages for the case that an error > occured. > Its unprofessional to fail decoding a file but not provide any > hint as to why a failure occured. > > If we remove all error messages and just print a generic "failed > decoding header" or "failed to decode frame" > We would leave users wondering about each error and we would no longer > have differentiated bug reports. > There would be one very huge bug report about > "Error while decoding stream #0:0: Invalid data found when processing > input" > Because thats all detail the user can get if the message is not in the > binary. > That bug report then would be marked invalid of course and would help > neither users nor developers. > We ask users to report bugs with "--loglevel 99" so this is irrelevant if it's a DEBUG. > Iam not sure why error messages are since about a year or so > considered controversial by some developers but not before > In my case I get a lot of reports from users about ffmpeg spamming logs with obscure warnings. This happens on legal files as per my "SPS/PPS truncation" patch which scares them into thinking the file is broken. The same goes with spamming logs when there is some data corruption with warnings which are very obscure. Regards, Kieran Kunhya
Hi, On Sun, Sep 3, 2017 at 7:54 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > Its unprofessional to fail decoding a file but not provide any > hint as to why a failure occured. > [..] > Our code is already quite terse and devoid of comments and > documentation, error messages do not seem a exception here Both of these comments assume the error message that you're trying to add is in any shape, way or form helpful to end users. Unfortunately, it is not. Imagine that j-b saw the error message (compared to the generic "invalid data") and ask yourself if the utility was any different (again, compared to the generic "invalid data"). If not, then it shouldn't be an AV_LOG_ERROR message displayed to end users. Ronald
On Sun, Sep 03, 2017 at 09:27:53PM -0400, Ronald S. Bultje wrote: > Hi, > > On Sun, Sep 3, 2017 at 7:54 PM, Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > Its unprofessional to fail decoding a file but not provide any > > hint as to why a failure occured. > > > [..] > > > Our code is already quite terse and devoid of comments and > > documentation, error messages do not seem a exception here > > > Both of these comments assume the error message that you're trying to add > is in any shape, way or form helpful to end users. > > Unfortunately, it is not. > > Imagine that j-b saw the error message (compared to the generic "invalid > data") and ask yourself if the utility was any different (again, compared > to the generic "invalid data"). If not, then it shouldn't be an > AV_LOG_ERROR message displayed to end users. To take this example with a generic "invalid data", he knows theres an error and thats it with the added error message he can 1. grep it in git to find out where the error occurs 2. google it to see if its a known problem with a known solution find a patch to fix it, a version that contains that fix, a bug report, a regression, ... 3. find out with git log which commit added it much quicker than through bisect. 4. ask about the specific error on IRC, "is this known issue? should i upload the file?" for example But lets just assume for the sake of the argument, that it is useless, can you take a look at these please: (this is a uncut list of errors from vp9.c) libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Reserved bit set in RGB\n"); libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "RGB not supported in profile %d\n", libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "YUV 4:2:0 not supported in profile %d\n", libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Profile %d color details reserved bit set\n", libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Failed to initialize bitstream reader\n"); libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Invalid frame marker\n"); libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Profile %d is not yet supported\n", avctx->profile); libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Invalid sync code\n"); libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Invalid sync code\n"); libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Not all references are available\n"); libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Failed to initialize decoder for %dx%d @ %d\n", libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Ran out of memory during range coder init\n"); libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, libavcodec/vp9.c- "Ref pixfmt (%s) did not match current frame (%s)", libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, libavcodec/vp9.c- "Invalid ref frame dimensions %dx%d for frame size %dx%d\n", libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Invalid compressed header size\n"); libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Marker bit was set\n"); libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Requested reference %d not available\n", ref); libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, libavcodec/vp9.c- "Failed to allocate block buffers\n"); libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Failed to allocate frame buffer %d\n", i); libavcodec/vp9.c: av_log(avctx, AV_LOG_ERROR, "Failed to allocate frame buffer %d\n", i); Many of these are exactly teh same, technically detailed error messages at AV_LOG_ERROR level about technical details being out of valid range. digging deeper ecd9f57edcb libavcodec/vp9.c (Ronald S. Bultje 2015-09-02 14:35:03 -0400 343) av_log(ctx, AV_LOG_ERROR, "Reserved bit set in RGB\n"); 346ce5da197 libavcodec/vp9.c (Ronald S. Bultje 2015-05-05 09:39:13 -0400 347) av_log(ctx, AV_LOG_ERROR, "RGB not supported in profile %d\n", 346ce5da197 libavcodec/vp9.c (Ronald S. Bultje 2015-05-05 09:39:13 -0400 366) av_log(ctx, AV_LOG_ERROR, "YUV 4:2:0 not supported in profile %d\n", 346ce5da197 libavcodec/vp9.c (Ronald S. Bultje 2015-05-05 09:39:13 -0400 370) av_log(ctx, AV_LOG_ERROR, "Profile %d color details reserved bit set\n", 150c5543ffe libavcodec/vp9.c (Clément Bœsch 2013-11-15 23:26:45 +0100 393) av_log(ctx, AV_LOG_ERROR, "Failed to initialize bitstream reader\n"); 848826f527b libavcodec/vp9.c (Ronald S. Bultje 2013-09-30 23:03:30 -0400 397) av_log(ctx, AV_LOG_ERROR, "Invalid frame marker\n"); 079b7f6eacc libavcodec/vp9.c (James Almer 2015-05-04 18:37:50 -0300 404) av_log(ctx, AV_LOG_ERROR, "Profile %d is not yet supported\n", ctx->profile); 848826f527b libavcodec/vp9.c (Ronald S. Bultje 2013-09-30 23:03:30 -0400 420) av_log(ctx, AV_LOG_ERROR, "Invalid sync code\n"); 848826f527b libavcodec/vp9.c (Ronald S. Bultje 2013-09-30 23:03:30 -0400 436) av_log(ctx, AV_LOG_ERROR, "Invalid sync code\n"); 848826f527b libavcodec/vp9.c (Ronald S. Bultje 2013-09-30 23:03:30 -0400 467) av_log(ctx, AV_LOG_ERROR, "Not all references are available\n"); 1ac89869db0 libavcodec/vp9.c (Ronald S. Bultje 2015-12-01 12:24:05 -0500 635) av_log(ctx, AV_LOG_ERROR, "Failed to initialize decoder for %dx%d @ %d\n", 848826f527b libavcodec/vp9.c (Ronald S. Bultje 2013-09-30 23:03:30 -0400 657) av_log(ctx, AV_LOG_ERROR, "Ran out of memory during range coder init\n"); 585083dd1fc libavcodec/vp9.c (Hendrik Leppkes 2015-12-03 11:10:33 +0100 669) av_log(ctx, AV_LOG_ERROR, 585083dd1fc libavcodec/vp9.c (Hendrik Leppkes 2015-12-03 11:10:33 +0100 678) av_log(ctx, AV_LOG_ERROR, 848826f527b libavcodec/vp9.c (Ronald S. Bultje 2013-09-30 23:03:30 -0400 714) av_log(ctx, AV_LOG_ERROR, "Invalid compressed header size\n"); 848826f527b libavcodec/vp9.c (Ronald S. Bultje 2013-09-30 23:03:30 -0400 722) av_log(ctx, AV_LOG_ERROR, "Marker bit was set\n"); 848826f527b libavcodec/vp9.c (Ronald S. Bultje 2013-09-30 23:03:30 -0400 1282) av_log(ctx, AV_LOG_ERROR, "Requested reference %d not available\n", ref); af63ea7078c libavcodec/vp9.c (Ronald S. Bultje 2014-02-08 08:24:13 -0500 1377) av_log(ctx, AV_LOG_ERROR, 848826f527b libavcodec/vp9.c (Ronald S. Bultje 2013-09-30 23:03:30 -0400 1571) av_log(ctx, AV_LOG_ERROR, "Failed to allocate frame buffer %d\n", i); 848826f527b libavcodec/vp9.c (Ronald S. Bultje 2013-09-30 23:03:30 -0400 1580) av_log(ctx, AV_LOG_ERROR, "Failed to allocate frame buffer %d\n", i); most of them where added by you. Why do you complain about me adding an error message to a error case in code i wrote and maintain while you add basically identical error messages in code you wrote and maintain ? Thanks [...]
On Mon, Sep 04, 2017 at 12:57:32AM +0000, Kieran Kunhya wrote: > On Mon, 4 Sep 2017 at 00:56 Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > We have always printed error messages for the case that an error > > occured. > > Its unprofessional to fail decoding a file but not provide any > > hint as to why a failure occured. > > > > If we remove all error messages and just print a generic "failed > > decoding header" or "failed to decode frame" > > We would leave users wondering about each error and we would no longer > > have differentiated bug reports. > > There would be one very huge bug report about > > "Error while decoding stream #0:0: Invalid data found when processing > > input" > > Because thats all detail the user can get if the message is not in the > > binary. > > That bug report then would be marked invalid of course and would help > > neither users nor developers. > > > > We ask users to report bugs with "--loglevel 99" so this is irrelevant if > it's a DEBUG. yes, true > > > > Iam not sure why error messages are since about a year or so > > considered controversial by some developers but not before > > > > In my case I get a lot of reports from users about ffmpeg spamming logs > with obscure warnings. > This happens on legal files as per my "SPS/PPS truncation" patch which > scares them into thinking the file is broken. > > The same goes with spamming logs when there is some data corruption with > warnings which are very obscure. The messages from libavcodec are by the nature of it quite technical. Does showing these messages to your users have any value for them at all ? AV_LOG_ERROR is documented as /** * Something went wrong and cannot losslessly be recovered. * However, not all future data is affected. */ #define AV_LOG_ERROR 16 This matches the case its used in. I am not against using DEBUG level for detailed error messages, and ERROR for generic ones if thats what more people prefer. But for this to achive any usefull effect it has to be done consistently accross the codebase. ATM we more commonly use AV_LOG_ERROR for these. [...]
Hi, On Mon, Sep 4, 2017 at 5:30 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > But lets just assume for the sake of the argument, that it is useless, > can you take a look at these please: > (this is a uncut list of errors from vp9.c) > But mommy, he did it too. I veto your patch. If it makes you feel better, I give you permission to remove them from vp9.c also. I will not contribute to this discussion any further. You can discuss with me at VDD in person if you wish to discuss this further. Ronald
Hi! > Am 04.09.2017 um 13:48 schrieb "Ronald S. Bultje" <rsbultje@gmail.com>: > > Hi, > > On Mon, Sep 4, 2017 at 5:30 AM, Michael Niedermayer <michael@niedermayer.cc> > wrote: > >> But lets just assume for the sake of the argument, that it is useless, >> can you take a look at these please: >> (this is a uncut list of errors from vp9.c) > > But mommy, he did it too. > > I veto your patch. > > If it makes you feel better, I give you permission to remove them from > vp9.c also. > > I will not contribute to this discussion any further. You can discuss with > me at VDD in person if you wish to discuss this further. Wow. The amount of childish arguments coming from you (as an answer to a technical analysis) is reaching a surprising level. Carl Eugen
On 9/4/2017 6:48 AM, Michael Niedermayer wrote: > On Mon, Sep 04, 2017 at 12:57:32AM +0000, Kieran Kunhya wrote: >> On Mon, 4 Sep 2017 at 00:56 Michael Niedermayer <michael@niedermayer.cc> >> wrote: >> >>> We have always printed error messages for the case that an error >>> occured. >>> Its unprofessional to fail decoding a file but not provide any >>> hint as to why a failure occured. >>> >>> If we remove all error messages and just print a generic "failed >>> decoding header" or "failed to decode frame" >>> We would leave users wondering about each error and we would no longer >>> have differentiated bug reports. >>> There would be one very huge bug report about >>> "Error while decoding stream #0:0: Invalid data found when processing >>> input" >>> Because thats all detail the user can get if the message is not in the >>> binary. >>> That bug report then would be marked invalid of course and would help >>> neither users nor developers. >>> >> >> We ask users to report bugs with "--loglevel 99" so this is irrelevant if >> it's a DEBUG. > > yes, true > > >> >> >>> Iam not sure why error messages are since about a year or so >>> considered controversial by some developers but not before >>> >> >> In my case I get a lot of reports from users about ffmpeg spamming logs >> with obscure warnings. >> This happens on legal files as per my "SPS/PPS truncation" patch which >> scares them into thinking the file is broken. >> >> The same goes with spamming logs when there is some data corruption with >> warnings which are very obscure. > > The messages from libavcodec are by the nature of it quite technical. > Does showing these messages to your users have any value for them at > all ? > > AV_LOG_ERROR is documented as > /** > * Something went wrong and cannot losslessly be recovered. > * However, not all future data is affected. > */ > #define AV_LOG_ERROR 16 > > This matches the case its used in. > > I am not against using DEBUG level for detailed error messages, and > ERROR for generic ones if thats what more people prefer. > But for this to achive any usefull effect it has to be done > consistently accross the codebase. ATM we more commonly use > AV_LOG_ERROR for these. What some people dislike here is not that you're adding new error messages but that said error messages mean nothing even to developers in general. "Failed to allocate a frame size %dx%d", "RGB not supported in profile %d", "Requested reference %d not available", "Profile %d is not yet supported" are concise and informative. You ran out of memory, the file reports invalid parameters, the file is missing data, the file contains features not yet supported. Be it an user or a dev, you can easily realize what went wrong. However, "Invalid cbd %d", crd %d", "Invalid ld %d" mean nothing to pretty much anyone except the person that wrote the relevant code. The three are names for local variables you added as duplicate of another three local variables for the purpose of doing range checks. They are not even labels in the snow spec (assuming there's one), they are just variable names that could as well have been x, y and z. Maybe you could replace them with an error message that mentions something like a block within a frame has out of range or otherwise invalid data/values? Anything that actually informs whoever sees the message that something is wrong in the file and not that some internal variable has an invalid value.
On Mon, Sep 04, 2017 at 11:58:07AM -0300, James Almer wrote: > On 9/4/2017 6:48 AM, Michael Niedermayer wrote: > > On Mon, Sep 04, 2017 at 12:57:32AM +0000, Kieran Kunhya wrote: > >> On Mon, 4 Sep 2017 at 00:56 Michael Niedermayer <michael@niedermayer.cc> > >> wrote: > >> > >>> We have always printed error messages for the case that an error > >>> occured. > >>> Its unprofessional to fail decoding a file but not provide any > >>> hint as to why a failure occured. > >>> > >>> If we remove all error messages and just print a generic "failed > >>> decoding header" or "failed to decode frame" > >>> We would leave users wondering about each error and we would no longer > >>> have differentiated bug reports. > >>> There would be one very huge bug report about > >>> "Error while decoding stream #0:0: Invalid data found when processing > >>> input" > >>> Because thats all detail the user can get if the message is not in the > >>> binary. > >>> That bug report then would be marked invalid of course and would help > >>> neither users nor developers. > >>> > >> > >> We ask users to report bugs with "--loglevel 99" so this is irrelevant if > >> it's a DEBUG. > > > > yes, true > > > > > >> > >> > >>> Iam not sure why error messages are since about a year or so > >>> considered controversial by some developers but not before > >>> > >> > >> In my case I get a lot of reports from users about ffmpeg spamming logs > >> with obscure warnings. > >> This happens on legal files as per my "SPS/PPS truncation" patch which > >> scares them into thinking the file is broken. > >> > >> The same goes with spamming logs when there is some data corruption with > >> warnings which are very obscure. > > > > The messages from libavcodec are by the nature of it quite technical. > > Does showing these messages to your users have any value for them at > > all ? > > > > AV_LOG_ERROR is documented as > > /** > > * Something went wrong and cannot losslessly be recovered. > > * However, not all future data is affected. > > */ > > #define AV_LOG_ERROR 16 > > > > This matches the case its used in. > > > > I am not against using DEBUG level for detailed error messages, and > > ERROR for generic ones if thats what more people prefer. > > But for this to achive any usefull effect it has to be done > > consistently accross the codebase. ATM we more commonly use > > AV_LOG_ERROR for these. > > What some people dislike here is not that you're adding new error > messages but that said error messages mean nothing even to developers in > general. > > "Failed to allocate a frame size %dx%d", "RGB not supported in profile > %d", "Requested reference %d not available", "Profile %d is not yet > supported" are concise and informative. You ran out of memory, the file > reports invalid parameters, the file is missing data, the file contains > features not yet supported. > Be it an user or a dev, you can easily realize what went wrong. > > However, "Invalid cbd %d", crd %d", "Invalid ld %d" mean nothing to > pretty much anyone except the person that wrote the relevant code. The > three are names for local variables you added as duplicate of another > three local variables for the purpose of doing range checks. > They are not even labels in the snow spec (assuming there's one), they > are just variable names that could as well have been x, y and z. > > Maybe you could replace them with an error message that mentions > something like a block within a frame has out of range or otherwise > invalid data/values? Anything that actually informs whoever sees the > message that something is wrong in the file and not that some internal > variable has an invalid value. sure, ill reword so its more clear what is wrong and resubmit Thanks! [...]
diff --git a/libavcodec/snowdec.c b/libavcodec/snowdec.c index b74c468ce3..7e07857a44 100644 --- a/libavcodec/snowdec.c +++ b/libavcodec/snowdec.c @@ -183,13 +183,24 @@ static int decode_q_branch(SnowContext *s, int level, int x, int y){ int my_context= av_log2(2*FFABS(left->my - top->my)) + 0*av_log2(2*FFABS(tr->my - top->my)); type= get_rac(&s->c, &s->block_state[1 + left->type + top->type]) ? BLOCK_INTRA : 0; - if(type){ + int ld, cbd, crd; pred_mv(s, &mx, &my, 0, left, top, tr); - l += get_symbol(&s->c, &s->block_state[32], 1); + ld = get_symbol(&s->c, &s->block_state[32], 1); + if (ld < -255 || ld > 255) { + av_log(s->avctx, AV_LOG_ERROR, "Invalid ld %d\n", ld); + return AVERROR_INVALIDDATA; + } + l += ld; if (s->nb_planes > 2) { - cb+= get_symbol(&s->c, &s->block_state[64], 1); - cr+= get_symbol(&s->c, &s->block_state[96], 1); + cbd = get_symbol(&s->c, &s->block_state[64], 1); + crd = get_symbol(&s->c, &s->block_state[96], 1); + if (cbd < -255 || cbd > 255 || crd < -255 || crd > 255) { + av_log(s->avctx, AV_LOG_ERROR, "Invalid cbd %d, crd %d\n", cbd, crd); + return AVERROR_INVALIDDATA; + } + cb += cbd; + cr += crd; } }else{ if(s->ref_frames > 1)
Fixes: Timeout Fixes: 3142/clusterfuzz-testcase-5007853163118592 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/snowdec.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)