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 <mich...@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 <mich...@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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB "Nothing to hide" only works if the folks in power share the values of you and everyone you know entirely and always will -- Tom Scott
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
