[FFmpeg-devel,1/2] avcodec/snowdec: Check intra block dc differences.

Submitted by Michael Niedermayer on Nov. 15, 2017, 8:17 p.m.

Details

Message ID 20171115201716.2924-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Nov. 15, 2017, 8:17 p.m.
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(-)

Comments

Ronald S. Bultje Nov. 15, 2017, 8:26 p.m.
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
Carl Eugen Hoyos Nov. 15, 2017, 10:38 p.m.
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
Michael Niedermayer Nov. 16, 2017, 12:02 a.m.
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.


[...]
Ronald S. Bultje Nov. 16, 2017, 3:06 a.m.
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
Carl Eugen Hoyos Nov. 16, 2017, 3:15 a.m.
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
Paul B Mahol Nov. 16, 2017, 10:49 a.m.
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.
Ronald S. Bultje Nov. 16, 2017, 11:26 a.m.
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
Michael Niedermayer Nov. 16, 2017, 4:50 p.m.
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.

[...]
Ronald S. Bultje Nov. 16, 2017, 6:21 p.m.
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
Michael Niedermayer Nov. 16, 2017, 9:41 p.m.
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

[...]
Kieran Kunhya Nov. 16, 2017, 10:09 p.m.
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
Ronald S. Bultje Nov. 17, 2017, 12:47 a.m.
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
Michael Niedermayer Nov. 17, 2017, 1:43 a.m.
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

[...]
James Almer Nov. 17, 2017, 2:13 a.m.
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...
Michael Niedermayer Nov. 17, 2017, 3:14 p.m.
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

[...]
Ronald S. Bultje Nov. 17, 2017, 3:28 p.m.
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
Nicolas George Nov. 17, 2017, 6:24 p.m.
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,
Michael Niedermayer Nov. 17, 2017, 7:06 p.m.
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

[...]
Carl Eugen Hoyos Nov. 17, 2017, 10:15 p.m.
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

Patch hide | download patch | download mbox

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)