| Message ID | 20171115201716.2924-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
Hi, On Wed, Nov 15, 2017 at 3:17 PM, 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 727e908fb5..77ffe7f594 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_DEBUG, "Invalid (Out of range) > intra luma block DC difference %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_DEBUG, "Invalid (Out of > range) intra chroma block DC difference %d, %d\n", cbd, crd); > + return AVERROR_INVALIDDATA; > + } Please remove the error messages. Ronald
2017-11-15 21:26 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > Hi, > > On Wed, Nov 15, 2017 at 3:17 PM, 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 727e908fb5..77ffe7f594 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_DEBUG, "Invalid (Out of range) >> intra luma block DC difference %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_DEBUG, "Invalid (Out of >> range) intra chroma block DC difference %d, %d\n", cbd, crd); >> + return AVERROR_INVALIDDATA; >> + } > > > Please remove the error messages. How is the user supposed to know why decoding failed? How is a developer answering on the user mailing list supposed to know what the issue is? (Are you planning to work on snow?) Carl Eugen
On Wed, Nov 15, 2017 at 03:26:42PM -0500, Ronald S. Bultje wrote: > Hi, > > On Wed, Nov 15, 2017 at 3:17 PM, 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 727e908fb5..77ffe7f594 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_DEBUG, "Invalid (Out of range) > > intra luma block DC difference %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_DEBUG, "Invalid (Out of > > range) intra chroma block DC difference %d, %d\n", cbd, crd); > > + return AVERROR_INVALIDDATA; > > + } > > > Please remove the error messages. We had this discussion multiple times already. I would prefer to keep an error message as its important in bug reporting and to maintain and debug this code which iam maintainer and author of. Some similar previous discussion for example: http://ffmpeg.org/pipermail/ffmpeg-devel/2017-September/216499.html To repeat from the thread above: "Iam happy to follow what the community prefers." It seems you dont want to poll the community Is your politly worded request meant litterally just as a suggestion (which i can ignore) ? or did you intend this to be a veto ? Which i would of course respect even though iam not sure you have veto power over maintainer and author. On top of that, this is part of a security fix for an issue that will be made (automatically) public soon. [...]
Hi, On Wed, Nov 15, 2017 at 7:02 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > On Wed, Nov 15, 2017 at 03:26:42PM -0500, Ronald S. Bultje wrote: > > Hi, > > > > On Wed, Nov 15, 2017 at 3:17 PM, 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 727e908fb5..77ffe7f594 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_DEBUG, "Invalid (Out of range) > > > intra luma block DC difference %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_DEBUG, "Invalid (Out of > > > range) intra chroma block DC difference %d, %d\n", cbd, crd); > > > + return AVERROR_INVALIDDATA; > > > + } > > > > > > Please remove the error messages. > > We had this discussion multiple times already. > I would prefer to keep an error message as its important in bug > reporting and to maintain and debug this code which iam maintainer and > author of. > > Some similar previous discussion for example: > http://ffmpeg.org/pipermail/ffmpeg-devel/2017-September/216499.html > > To repeat from the thread above: > "Iam happy to follow what the community prefers." > > It seems you dont want to poll the community > > Is your politly worded request meant litterally > just as a suggestion (which i can ignore) ? > or did you intend this to be a veto ? > Which i would of course respect even though iam not sure you have veto > power over maintainer and author. > > On top of that, this is part of a security fix for an issue that will > be made (automatically) public soon. So, commit it without the error message? I really don't see the issue. Is the lack of error message the security issue? Ronald
2017-11-16 4:06 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>:
> So, commit it without the error message? I really don't see the issue.
As explained, the issue is that without an error message, it
is impossible to parse any related bug report.
Carl Eugen
On 11/16/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-11-16 4:06 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > >> So, commit it without the error message? I really don't see the issue. > > As explained, the issue is that without an error message, it > is impossible to parse any related bug report. > > Carl Eugen Like you will get any useful bug report for snow anyway.
Hi, On Wed, Nov 15, 2017 at 10:15 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-11-16 4:06 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > > > So, commit it without the error message? I really don't see the issue. > > As explained, the issue is that without an error message, it > is impossible to parse any related bug report. We've been OK with that situation so far. Since it only happens for fuzzed files, it's OK to continue going like that. Ronald
On Thu, Nov 16, 2017 at 06:26:06AM -0500, Ronald S. Bultje wrote: > Hi, > > On Wed, Nov 15, 2017 at 10:15 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > wrote: > > > 2017-11-16 4:06 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > > > > > So, commit it without the error message? I really don't see the issue. > > > > As explained, the issue is that without an error message, it > > is impossible to parse any related bug report. > > > We've been OK with that situation so far. Since it only happens for fuzzed > files, it's OK to continue going like that. Thats not the case, the snow spec contains no limit in the place where we need to check. Its a natural and expected limit so likely all files will be within that but a file outside would still be arguably valid. So a valid file could potentially be outside this range and the maintainer (that being me) need to know about this. Please dont see every change that originated from a fuzzer generated report as only related to fuzzed files. [...]
Hi, On Thu, Nov 16, 2017 at 11:50 AM, Michael Niedermayer < michael@niedermayer.cc> wrote: > On Thu, Nov 16, 2017 at 06:26:06AM -0500, Ronald S. Bultje wrote: > > Hi, > > > > On Wed, Nov 15, 2017 at 10:15 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > > wrote: > > > > > 2017-11-16 4:06 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > > > > > > > So, commit it without the error message? I really don't see the > issue. > > > > > > As explained, the issue is that without an error message, it > > > is impossible to parse any related bug report. > > > > > > We've been OK with that situation so far. Since it only happens for > fuzzed > > files, it's OK to continue going like that. > > Thats not the case, the snow spec contains no limit in the place where > we need to check. Its a natural and expected limit so likely all files > will be within that but a file outside would still be arguably valid. > > So a valid file could potentially be outside this range and the > maintainer (that being me) need to know about this. > > Please dont see every change that originated from a fuzzer generated > report as only related to fuzzed files. We are re-hashing old arguments here. I'm not really interested in that. My review comment is and remains: please remove the log msg. Otherwise, the patch is perfectly fine. Ronald
On Thu, Nov 16, 2017 at 01:21:19PM -0500, Ronald S. Bultje wrote: > Hi, > > On Thu, Nov 16, 2017 at 11:50 AM, Michael Niedermayer < > michael@niedermayer.cc> wrote: > > > On Thu, Nov 16, 2017 at 06:26:06AM -0500, Ronald S. Bultje wrote: > > > Hi, > > > > > > On Wed, Nov 15, 2017 at 10:15 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > > > wrote: > > > > > > > 2017-11-16 4:06 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > > > > > > > > > So, commit it without the error message? I really don't see the > > issue. > > > > > > > > As explained, the issue is that without an error message, it > > > > is impossible to parse any related bug report. > > > > > > > > > We've been OK with that situation so far. Since it only happens for > > fuzzed > > > files, it's OK to continue going like that. > > > > Thats not the case, the snow spec contains no limit in the place where > > we need to check. Its a natural and expected limit so likely all files > > will be within that but a file outside would still be arguably valid. > > > > So a valid file could potentially be outside this range and the > > maintainer (that being me) need to know about this. > > > > Please dont see every change that originated from a fuzzer generated > > report as only related to fuzzed files. > > > We are re-hashing old arguments here. I'm not really interested in that. > > My review comment is and remains: please remove the log msg. Otherwise, the > patch is perfectly fine. Thank you for your review comment. please awnser my question, if this is just a suggestion or a veto, so we can move forward. Its not clear from your wording to me if you belive you have authority over other maintainers or not. Normally developers withdraw a comment if the maintainer disagrees or one would start some poll to find out what the majority preferres and create a rule from that for all (which i suggested already) yet you just repeat the same comment. It feels impolite if i would just go ahead and push the patch without confirming that this is just a suggestion and not some kind of veto. So please clarify this Thanks [...]
On Thu, 16 Nov 2017 at 18:21 Ronald S. Bultje <rsbultje@gmail.com> wrote: > We are re-hashing old arguments here. I'm not really interested in that. > > My review comment is and remains: please remove the log msg. Otherwise, the > patch is perfectly fine. > I agree with Ronald's argument. I don't care about this particular instance however since Snow is not of interest to me. Kieran
Hi, On Thu, Nov 16, 2017 at 4:41 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > On Thu, Nov 16, 2017 at 01:21:19PM -0500, Ronald S. Bultje wrote: > > Hi, > > > > On Thu, Nov 16, 2017 at 11:50 AM, Michael Niedermayer < > > michael@niedermayer.cc> wrote: > > > > > On Thu, Nov 16, 2017 at 06:26:06AM -0500, Ronald S. Bultje wrote: > > > > Hi, > > > > > > > > On Wed, Nov 15, 2017 at 10:15 PM, Carl Eugen Hoyos < > ceffmpeg@gmail.com> > > > > wrote: > > > > > > > > > 2017-11-16 4:06 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > > > > > > > > > > > So, commit it without the error message? I really don't see the > > > issue. > > > > > > > > > > As explained, the issue is that without an error message, it > > > > > is impossible to parse any related bug report. > > > > > > > > > > > > We've been OK with that situation so far. Since it only happens for > > > fuzzed > > > > files, it's OK to continue going like that. > > > > > > Thats not the case, the snow spec contains no limit in the place where > > > we need to check. Its a natural and expected limit so likely all files > > > will be within that but a file outside would still be arguably valid. > > > > > > So a valid file could potentially be outside this range and the > > > maintainer (that being me) need to know about this. > > > > > > Please dont see every change that originated from a fuzzer generated > > > report as only related to fuzzed files. > > > > > > We are re-hashing old arguments here. I'm not really interested in that. > > > > > My review comment is and remains: please remove the log msg. Otherwise, > the > > patch is perfectly fine. > > Thank you for your review comment. > > please awnser my question, if this is just a suggestion or a > veto, so we can move forward. Its not clear from your wording to me > if you belive you have authority over other maintainers or not. I'm sorry, what? Authority? Are you kidding me? Amend the patch, stop being difficult. This whole discussion is ridiculous. I thought you were on a deadline for a remote root code execution exploit or something? Ronald
On Thu, Nov 16, 2017 at 07:47:55PM -0500, Ronald S. Bultje wrote: > Hi, > > On Thu, Nov 16, 2017 at 4:41 PM, Michael Niedermayer <michael@niedermayer.cc > > wrote: > > > On Thu, Nov 16, 2017 at 01:21:19PM -0500, Ronald S. Bultje wrote: > > > Hi, > > > > > > On Thu, Nov 16, 2017 at 11:50 AM, Michael Niedermayer < > > > michael@niedermayer.cc> wrote: > > > > > > > On Thu, Nov 16, 2017 at 06:26:06AM -0500, Ronald S. Bultje wrote: > > > > > Hi, > > > > > > > > > > On Wed, Nov 15, 2017 at 10:15 PM, Carl Eugen Hoyos < > > ceffmpeg@gmail.com> > > > > > wrote: > > > > > > > > > > > 2017-11-16 4:06 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > > > > > > > > > > > > > So, commit it without the error message? I really don't see the > > > > issue. > > > > > > > > > > > > As explained, the issue is that without an error message, it > > > > > > is impossible to parse any related bug report. > > > > > > > > > > > > > > > We've been OK with that situation so far. Since it only happens for > > > > fuzzed > > > > > files, it's OK to continue going like that. > > > > > > > > Thats not the case, the snow spec contains no limit in the place where > > > > we need to check. Its a natural and expected limit so likely all files > > > > will be within that but a file outside would still be arguably valid. > > > > > > > > So a valid file could potentially be outside this range and the > > > > maintainer (that being me) need to know about this. > > > > > > > > Please dont see every change that originated from a fuzzer generated > > > > report as only related to fuzzed files. > > > > > > > > > We are re-hashing old arguments here. I'm not really interested in that. > > > > > > > > My review comment is and remains: please remove the log msg. Otherwise, > > the > > > patch is perfectly fine. > > > > Thank you for your review comment. > > > > please awnser my question, if this is just a suggestion or a > > veto, so we can move forward. Its not clear from your wording to me > > if you belive you have authority over other maintainers or not. > > > I'm sorry, what? Authority? Are you kidding me? Amend the patch, stop being > difficult. This whole discussion is ridiculous. I thought you were on a > deadline for a remote root code execution exploit or something? The patch should be commited theres no question about this. Why should i amend it and remove the error message which help me to maintain the code ? Do you realize how ridiculous your request is ? This is code i wrote, code i maintain, you dont work on this code or have anything to do with it. And these error messages help me maintain the code. I think you should relax a bit and jump over your shadow and let me maintain my code instead of threads like this. ill remove the messages so this gets resolved but its harder to maintain this way so this means fewer usefull contributions from me and lower code quality. I dont think thats the ideal outcome ... Thanks [...]
On 11/16/2017 10:43 PM, Michael Niedermayer wrote: > On Thu, Nov 16, 2017 at 07:47:55PM -0500, Ronald S. Bultje wrote: >> Hi, >> >> On Thu, Nov 16, 2017 at 4:41 PM, Michael Niedermayer <michael@niedermayer.cc >>> wrote: >> >>> On Thu, Nov 16, 2017 at 01:21:19PM -0500, Ronald S. Bultje wrote: >>>> Hi, >>>> >>>> On Thu, Nov 16, 2017 at 11:50 AM, Michael Niedermayer < >>>> michael@niedermayer.cc> wrote: >>>> >>>>> On Thu, Nov 16, 2017 at 06:26:06AM -0500, Ronald S. Bultje wrote: >>>>>> Hi, >>>>>> >>>>>> On Wed, Nov 15, 2017 at 10:15 PM, Carl Eugen Hoyos < >>> ceffmpeg@gmail.com> >>>>>> wrote: >>>>>> >>>>>>> 2017-11-16 4:06 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: >>>>>>> >>>>>>>> So, commit it without the error message? I really don't see the >>>>> issue. >>>>>>> >>>>>>> As explained, the issue is that without an error message, it >>>>>>> is impossible to parse any related bug report. >>>>>> >>>>>> >>>>>> We've been OK with that situation so far. Since it only happens for >>>>> fuzzed >>>>>> files, it's OK to continue going like that. >>>>> >>>>> Thats not the case, the snow spec contains no limit in the place where >>>>> we need to check. Its a natural and expected limit so likely all files >>>>> will be within that but a file outside would still be arguably valid. >>>>> >>>>> So a valid file could potentially be outside this range and the >>>>> maintainer (that being me) need to know about this. >>>>> >>>>> Please dont see every change that originated from a fuzzer generated >>>>> report as only related to fuzzed files. >>>> >>>> >>>> We are re-hashing old arguments here. I'm not really interested in that. >>> >>>> >>>> My review comment is and remains: please remove the log msg. Otherwise, >>> the >>>> patch is perfectly fine. >>> >>> Thank you for your review comment. >>> >>> please awnser my question, if this is just a suggestion or a >>> veto, so we can move forward. Its not clear from your wording to me >>> if you belive you have authority over other maintainers or not. >> >> >> I'm sorry, what? Authority? Are you kidding me? Amend the patch, stop being >> difficult. This whole discussion is ridiculous. I thought you were on a >> deadline for a remote root code execution exploit or something? > > The patch should be commited theres no question about this. > Why should i amend it and remove the error message which help me to > maintain the code ? > > Do you realize how ridiculous your request is ? > This is code i wrote, code i maintain, you dont work on this code > or have anything to do with it. And these error messages help me > maintain the code. > > I think you should relax a bit and jump over your shadow and let me > maintain my code instead of threads like this. > > ill remove the messages so this gets resolved but its harder to maintain > this way so this means fewer usefull contributions from me and lower > code quality. > I dont think thats the ideal outcome ... > > Thanks Use ff_dlog() for the log message. It'll still be enabled on debug builds but not on release builds. Ronald said he's ok with it for this case on IRC. Unless this was suggested before and the idea discarded/rejected for some reason...
On Thu, Nov 16, 2017 at 11:13:27PM -0300, James Almer wrote: > On 11/16/2017 10:43 PM, Michael Niedermayer wrote: > > On Thu, Nov 16, 2017 at 07:47:55PM -0500, Ronald S. Bultje wrote: > >> Hi, > >> > >> On Thu, Nov 16, 2017 at 4:41 PM, Michael Niedermayer <michael@niedermayer.cc > >>> wrote: > >> > >>> On Thu, Nov 16, 2017 at 01:21:19PM -0500, Ronald S. Bultje wrote: > >>>> Hi, > >>>> > >>>> On Thu, Nov 16, 2017 at 11:50 AM, Michael Niedermayer < > >>>> michael@niedermayer.cc> wrote: > >>>> > >>>>> On Thu, Nov 16, 2017 at 06:26:06AM -0500, Ronald S. Bultje wrote: > >>>>>> Hi, > >>>>>> > >>>>>> On Wed, Nov 15, 2017 at 10:15 PM, Carl Eugen Hoyos < > >>> ceffmpeg@gmail.com> > >>>>>> wrote: > >>>>>> > >>>>>>> 2017-11-16 4:06 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>: > >>>>>>> > >>>>>>>> So, commit it without the error message? I really don't see the > >>>>> issue. > >>>>>>> > >>>>>>> As explained, the issue is that without an error message, it > >>>>>>> is impossible to parse any related bug report. > >>>>>> > >>>>>> > >>>>>> We've been OK with that situation so far. Since it only happens for > >>>>> fuzzed > >>>>>> files, it's OK to continue going like that. > >>>>> > >>>>> Thats not the case, the snow spec contains no limit in the place where > >>>>> we need to check. Its a natural and expected limit so likely all files > >>>>> will be within that but a file outside would still be arguably valid. > >>>>> > >>>>> So a valid file could potentially be outside this range and the > >>>>> maintainer (that being me) need to know about this. > >>>>> > >>>>> Please dont see every change that originated from a fuzzer generated > >>>>> report as only related to fuzzed files. > >>>> > >>>> > >>>> We are re-hashing old arguments here. I'm not really interested in that. > >>> > >>>> > >>>> My review comment is and remains: please remove the log msg. Otherwise, > >>> the > >>>> patch is perfectly fine. > >>> > >>> Thank you for your review comment. > >>> > >>> please awnser my question, if this is just a suggestion or a > >>> veto, so we can move forward. Its not clear from your wording to me > >>> if you belive you have authority over other maintainers or not. > >> > >> > >> I'm sorry, what? Authority? Are you kidding me? Amend the patch, stop being > >> difficult. This whole discussion is ridiculous. I thought you were on a > >> deadline for a remote root code execution exploit or something? > > > > The patch should be commited theres no question about this. > > Why should i amend it and remove the error message which help me to > > maintain the code ? > > > > Do you realize how ridiculous your request is ? > > This is code i wrote, code i maintain, you dont work on this code > > or have anything to do with it. And these error messages help me > > maintain the code. > > > > I think you should relax a bit and jump over your shadow and let me > > maintain my code instead of threads like this. > > > > ill remove the messages so this gets resolved but its harder to maintain > > this way so this means fewer usefull contributions from me and lower > > code quality. > > I dont think thats the ideal outcome ... > > > > Thanks > > Use ff_dlog() for the log message. It'll still be enabled on debug > builds but not on release builds. Ronald said he's ok with it for this > case on IRC. > > Unless this was suggested before and the idea discarded/rejected for > some reason... ff_dlog() is not enabled in user builds so it wont result in an error message in a bug report. Its also not enabled when you use --enable-debug so very few developers would see it either. That is in effect ff_dlog() is the same as removing the log message I also do not know if the build would be generally useable with all ff_dlog() enabled or too noisy or too slow. error message have a low volume, they are only shown when an error occurs. debug messages and ff_dlog() is high volume there alot of stuff displayed for valid files. As a maintainer If a file fails with an error i want to know what error it is. Why is this so hard or controversal ? Thanks [...]
Hi, On Fri, Nov 17, 2017 at 10:14 AM, Michael Niedermayer < michael@niedermayer.cc> wrote: > As a maintainer If a file fails with an error i want to know > what error it is. Why is this so hard or controversal ? Because *it doesn't happen* for real files. But you don't want to listen. You still -after all these years- think that if you believe something, then it must be true. How can this project be collaborative if any time we disagree, you just dig in your heels in the sand and start this same old argument where "you are maintainer" and therefore whatever you believe is true, and whatever you say is final? :-( Ronald
Le septidi 27 brumaire, an CCXXVI, Ronald S. Bultje a écrit :
> Because *it doesn't happen* for real files.
Then why do you want it gone?
Regards,
On Fri, Nov 17, 2017 at 10:28:49AM -0500, Ronald S. Bultje wrote: > Hi, > > On Fri, Nov 17, 2017 at 10:14 AM, Michael Niedermayer < > michael@niedermayer.cc> wrote: > > > As a maintainer If a file fails with an error i want to know > > what error it is. Why is this so hard or controversal ? > > > Because *it doesn't happen* for real files. > > But you don't want to listen. So you know exactly which error conditions will never happen ? And thats considering all the odd files we saw over the years, hacked and modified encoders and hacked and modified specs and drafts on which some encoders are based which we have seen files from. And you expect that every time we consider adding an error message someone should do this analysis ? And after we have ommitted hundreads of error messages this way with all the time that costs. the 10kb of space saved is worth more than the hundread hours spend on this analysis ? and none of these predictions of non occurance in real world files will be wrong ? In no significant number of cases will bugs be missed because they are just vissible through one of these ommited error messages ? The above sounds unreasonable if that is your position. But please correct me if that is not your position. It seemed to follow from a selective ommision of error messages > You still -after all these years- think that > if you believe something, then it must be true. How can this project be > collaborative if any time we disagree, you just dig in your heels in the > sand and start this same old argument where "you are maintainer" and > therefore whatever you believe is true, and whatever you say is final? Something in your reasoning is a bit unreasonable here. I do not remember that you ever backed down from an argument. And you say i am diging my heels in ? and that after i agreed to remove the messages yesterday so this can move forward and this bug can be fixed. About "collaborative" i must repeat what i said multiple times now "Iam happy to follow what the community prefers." How can this be anti-collaborative !?!?!?! We can do what i suggested many times already, poll the community and follow the communities preferrance on error messages. Thanks [...]
2017-11-17 1:47 GMT+01:00 Ronald S. Bultje <rsbultje@gmail.com>:
> stop being difficult.
Yes please!
I have no idea what's driving you but please stop this childish
attitude: You haven't answered to a bug report since an eternity,
so you simply cannot claim that error messages are unneeded.
Please stop this behaviour, Carl Eugen
diff --git a/libavcodec/snowdec.c b/libavcodec/snowdec.c index 727e908fb5..77ffe7f594 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_DEBUG, "Invalid (Out of range) intra luma block DC difference %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_DEBUG, "Invalid (Out of range) intra chroma block DC difference %d, %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(-)