| Message ID | 20190808232350.16076-4-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 9, 2019 at 1:26 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int' > Fixes: > 15764/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5102101203517440 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by > <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>: > Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/alac.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/alac.c b/libavcodec/alac.c > index 6086e2caa8..1196925aa7 100644 > --- a/libavcodec/alac.c > +++ b/libavcodec/alac.c > @@ -250,7 +250,7 @@ static int decode_element(AVCodecContext *avctx, > AVFrame *frame, int ch_index, > > alac->extra_bits = get_bits(&alac->gb, 2) << 3; > bps = alac->sample_size - alac->extra_bits + channels - 1; > - if (bps > 32U) { > + if (bps > 32 || bps < 1) { > avpriv_report_missing_feature(avctx, "bps %d", bps); > return AVERROR_PATCHWELCOME; > } > -- > 2.22.0 > I'm not sure reporting for missing feature in this case (bps == 0) is valid. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 8/8/2019 8:23 PM, Michael Niedermayer wrote: > Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int' > Fixes: 15764/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5102101203517440 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/alac.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/alac.c b/libavcodec/alac.c > index 6086e2caa8..1196925aa7 100644 > --- a/libavcodec/alac.c > +++ b/libavcodec/alac.c > @@ -250,7 +250,7 @@ static int decode_element(AVCodecContext *avctx, AVFrame *frame, int ch_index, > > alac->extra_bits = get_bits(&alac->gb, 2) << 3; > bps = alac->sample_size - alac->extra_bits + channels - 1; > - if (bps > 32U) { > + if (bps > 32 || bps < 1) { > avpriv_report_missing_feature(avctx, "bps %d", bps); > return AVERROR_PATCHWELCOME; bps 0 (or negative) is obviously a broken file, so asking for a sample is pointless. Just return invalid data in those cases, and leave this check for > 32. > } >
On Fri, Aug 23, 2019 at 11:20:48AM -0300, James Almer wrote: > On 8/8/2019 8:23 PM, Michael Niedermayer wrote: > > Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int' > > Fixes: 15764/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5102101203517440 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/alac.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavcodec/alac.c b/libavcodec/alac.c > > index 6086e2caa8..1196925aa7 100644 > > --- a/libavcodec/alac.c > > +++ b/libavcodec/alac.c > > @@ -250,7 +250,7 @@ static int decode_element(AVCodecContext *avctx, AVFrame *frame, int ch_index, > > > > alac->extra_bits = get_bits(&alac->gb, 2) << 3; > > bps = alac->sample_size - alac->extra_bits + channels - 1; > > - if (bps > 32U) { > > + if (bps > 32 || bps < 1) { > > avpriv_report_missing_feature(avctx, "bps %d", bps); > > return AVERROR_PATCHWELCOME; > > bps 0 (or negative) is obviously a broken file, id say very likely a broken file, yes > so asking for a sample > is pointless. Just return invalid data in those cases, and leave this > check for > 32. thats a few lines more code, for an error code and different/no message its a bit difficult to guess where people prefer the extra code to be correct and where they prefer somewhat incorrect solutions to minimize fuzzer found bugfixes. but yes, will post a new patch for this. thx [...]
On Sun, Aug 25, 2019 at 9:33 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Aug 23, 2019 at 11:20:48AM -0300, James Almer wrote: > > On 8/8/2019 8:23 PM, Michael Niedermayer wrote: > > > Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int' > > > Fixes: > 15764/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5102101203517440 > > > > > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/alac.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/libavcodec/alac.c b/libavcodec/alac.c > > > index 6086e2caa8..1196925aa7 100644 > > > --- a/libavcodec/alac.c > > > +++ b/libavcodec/alac.c > > > @@ -250,7 +250,7 @@ static int decode_element(AVCodecContext *avctx, > AVFrame *frame, int ch_index, > > > > > > alac->extra_bits = get_bits(&alac->gb, 2) << 3; > > > bps = alac->sample_size - alac->extra_bits + channels - 1; > > > - if (bps > 32U) { > > > + if (bps > 32 || bps < 1) { > > > avpriv_report_missing_feature(avctx, "bps %d", bps); > > > return AVERROR_PATCHWELCOME; > > > > bps 0 (or negative) is obviously a broken file, > > id say very likely a broken file, yes > > > > so asking for a sample > > is pointless. Just return invalid data in those cases, and leave this > > check for > 32. > > thats a few lines more code, for an error code and different/no message > its a bit difficult to guess where people prefer the extra code to be > correct and where they prefer somewhat incorrect solutions to minimize > fuzzer found bugfixes. > If you dislike what people prefer when reviewing, perhaps you should stop sending patches :-) > > but yes, will post a new patch for this. > > thx > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Any man who breaks a law that conscience tells him is unjust and willingly > accepts the penalty by staying in jail in order to arouse the conscience > of > the community on the injustice of the law is at that moment expressing the > very highest respect for law. - Martin Luther King Jr > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 8/25/2019 4:32 AM, Michael Niedermayer wrote: > On Fri, Aug 23, 2019 at 11:20:48AM -0300, James Almer wrote: >> On 8/8/2019 8:23 PM, Michael Niedermayer wrote: >>> Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int' >>> Fixes: 15764/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5102101203517440 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/alac.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/alac.c b/libavcodec/alac.c >>> index 6086e2caa8..1196925aa7 100644 >>> --- a/libavcodec/alac.c >>> +++ b/libavcodec/alac.c >>> @@ -250,7 +250,7 @@ static int decode_element(AVCodecContext *avctx, AVFrame *frame, int ch_index, >>> >>> alac->extra_bits = get_bits(&alac->gb, 2) << 3; >>> bps = alac->sample_size - alac->extra_bits + channels - 1; >>> - if (bps > 32U) { >>> + if (bps > 32 || bps < 1) { >>> avpriv_report_missing_feature(avctx, "bps %d", bps); >>> return AVERROR_PATCHWELCOME; >> >> bps 0 (or negative) is obviously a broken file, > > id say very likely a broken file, yes > > >> so asking for a sample >> is pointless. Just return invalid data in those cases, and leave this >> check for > 32. > > thats a few lines more code, for an error code and different/no message > its a bit difficult to guess where people prefer the extra code to be > correct and where they prefer somewhat incorrect solutions to minimize > fuzzer found bugfixes. I don't know who this was aimed to, but afaik i don't ask or don't intend to ask for "incorrect solutions" for fuzzer found issues. I simply stated that asking for a sample in a situation where it's known that the sample is simply a broken/corrupt one makes no sense and can end up wasting a user's time for bothering to submit it. The extra lines to properly abort in such cases are justified. > > but yes, will post a new patch for this. > > thx > > > [...] > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On Sun, Aug 25, 2019 at 12:06:30PM -0300, James Almer wrote: > On 8/25/2019 4:32 AM, Michael Niedermayer wrote: > > On Fri, Aug 23, 2019 at 11:20:48AM -0300, James Almer wrote: > >> On 8/8/2019 8:23 PM, Michael Niedermayer wrote: > >>> Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int' > >>> Fixes: 15764/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5102101203517440 > >>> > >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>> --- > >>> libavcodec/alac.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/libavcodec/alac.c b/libavcodec/alac.c > >>> index 6086e2caa8..1196925aa7 100644 > >>> --- a/libavcodec/alac.c > >>> +++ b/libavcodec/alac.c > >>> @@ -250,7 +250,7 @@ static int decode_element(AVCodecContext *avctx, AVFrame *frame, int ch_index, > >>> > >>> alac->extra_bits = get_bits(&alac->gb, 2) << 3; > >>> bps = alac->sample_size - alac->extra_bits + channels - 1; > >>> - if (bps > 32U) { > >>> + if (bps > 32 || bps < 1) { > >>> avpriv_report_missing_feature(avctx, "bps %d", bps); > >>> return AVERROR_PATCHWELCOME; > >> > >> bps 0 (or negative) is obviously a broken file, > > > > id say very likely a broken file, yes > > > > > >> so asking for a sample > >> is pointless. Just return invalid data in those cases, and leave this > >> check for > 32. > > > > thats a few lines more code, for an error code and different/no message > > its a bit difficult to guess where people prefer the extra code to be > > correct and where they prefer somewhat incorrect solutions to minimize > > fuzzer found bugfixes. > > I don't know who this was aimed to, but afaik i don't ask or don't > intend to ask for "incorrect solutions" for fuzzer found issues. no, of course not, but in some reviews people complained along the lines of there being too much fuzzer found fixes/checks. > I simply stated that asking for a sample in a situation where it's known > that the sample is simply a broken/corrupt one makes no sense and can > end up wasting a user's time for bothering to submit it. The extra lines > to properly abort in such cases are justified. yes, your request was fine, sorry if it appeared that i meant something else Thanks [...]
Hi Paul, > On Aug 25, 2019, at 12:37 AM, Paul B Mahol <onemda@gmail.com> wrote: > > On Sun, Aug 25, 2019 at 9:33 AM Michael Niedermayer <michael@niedermayer.cc> > wrote: > >> On Fri, Aug 23, 2019 at 11:20:48AM -0300, James Almer wrote: >>> On 8/8/2019 8:23 PM, Michael Niedermayer wrote: >>>> Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int' >>>> Fixes: >> 15764/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5102101203517440 >>>> >>>> Found-by: continuous fuzzing process >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>>> --- >>>> libavcodec/alac.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/libavcodec/alac.c b/libavcodec/alac.c >>>> index 6086e2caa8..1196925aa7 100644 >>>> --- a/libavcodec/alac.c >>>> +++ b/libavcodec/alac.c >>>> @@ -250,7 +250,7 @@ static int decode_element(AVCodecContext *avctx, >> AVFrame *frame, int ch_index, >>>> >>>> alac->extra_bits = get_bits(&alac->gb, 2) << 3; >>>> bps = alac->sample_size - alac->extra_bits + channels - 1; >>>> - if (bps > 32U) { >>>> + if (bps > 32 || bps < 1) { >>>> avpriv_report_missing_feature(avctx, "bps %d", bps); >>>> return AVERROR_PATCHWELCOME; >>> >>> bps 0 (or negative) is obviously a broken file, >> >> id say very likely a broken file, yes >> >> >>> so asking for a sample >>> is pointless. Just return invalid data in those cases, and leave this >>> check for > 32. >> >> thats a few lines more code, for an error code and different/no message >> its a bit difficult to guess where people prefer the extra code to be >> correct and where they prefer somewhat incorrect solutions to minimize >> fuzzer found bugfixes. >> > > If you dislike what people prefer when reviewing, perhaps you should stop > sending patches :-) This remark sounds rude and disrespectful to me. Please refrain from making remarks like this one in the future. — Baptiste
On Mon, Aug 26, 2019 at 7:45 PM Baptiste Coudurier < baptiste.coudurier@gmail.com> wrote: > Hi Paul, > > > > On Aug 25, 2019, at 12:37 AM, Paul B Mahol <onemda@gmail.com> wrote: > > > > On Sun, Aug 25, 2019 at 9:33 AM Michael Niedermayer > <michael@niedermayer.cc> > > wrote: > > > >> On Fri, Aug 23, 2019 at 11:20:48AM -0300, James Almer wrote: > >>> On 8/8/2019 8:23 PM, Michael Niedermayer wrote: > >>>> Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int' > >>>> Fixes: > >> > 15764/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5102101203517440 > >>>> > >>>> Found-by: continuous fuzzing process > >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>>> --- > >>>> libavcodec/alac.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/libavcodec/alac.c b/libavcodec/alac.c > >>>> index 6086e2caa8..1196925aa7 100644 > >>>> --- a/libavcodec/alac.c > >>>> +++ b/libavcodec/alac.c > >>>> @@ -250,7 +250,7 @@ static int decode_element(AVCodecContext *avctx, > >> AVFrame *frame, int ch_index, > >>>> > >>>> alac->extra_bits = get_bits(&alac->gb, 2) << 3; > >>>> bps = alac->sample_size - alac->extra_bits + channels - 1; > >>>> - if (bps > 32U) { > >>>> + if (bps > 32 || bps < 1) { > >>>> avpriv_report_missing_feature(avctx, "bps %d", bps); > >>>> return AVERROR_PATCHWELCOME; > >>> > >>> bps 0 (or negative) is obviously a broken file, > >> > >> id say very likely a broken file, yes > >> > >> > >>> so asking for a sample > >>> is pointless. Just return invalid data in those cases, and leave this > >>> check for > 32. > >> > >> thats a few lines more code, for an error code and different/no message > >> its a bit difficult to guess where people prefer the extra code to be > >> correct and where they prefer somewhat incorrect solutions to minimize > >> fuzzer found bugfixes. > >> > > > > If you dislike what people prefer when reviewing, perhaps you should stop > > sending patches :-) > > This remark sounds rude and disrespectful to me. > Please refrain from making remarks like this one in the future. > I do not see how this can be rude and disrespectful to you or anyone else. > > — > Baptiste > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavcodec/alac.c b/libavcodec/alac.c index 6086e2caa8..1196925aa7 100644 --- a/libavcodec/alac.c +++ b/libavcodec/alac.c @@ -250,7 +250,7 @@ static int decode_element(AVCodecContext *avctx, AVFrame *frame, int ch_index, alac->extra_bits = get_bits(&alac->gb, 2) << 3; bps = alac->sample_size - alac->extra_bits + channels - 1; - if (bps > 32U) { + if (bps > 32 || bps < 1) { avpriv_report_missing_feature(avctx, "bps %d", bps); return AVERROR_PATCHWELCOME; }
Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int' Fixes: 15764/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5102101203517440 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/alac.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)