[FFmpeg-devel,4/5] avcodec/alac: Check for bps of 0

Submitted by Michael Niedermayer on Aug. 8, 2019, 11:23 p.m.

Details

Message ID 20190808232350.16076-4-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Aug. 8, 2019, 11:23 p.m.
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(-)

Comments

Paul B Mahol Aug. 9, 2019, 7:34 a.m.
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".
James Almer Aug. 23, 2019, 2:20 p.m.
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.

>      }
>
Michael Niedermayer Aug. 25, 2019, 7:32 a.m.
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


[...]
Paul B Mahol Aug. 25, 2019, 7:37 a.m.
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".
James Almer Aug. 25, 2019, 3:06 p.m.
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".
>
Michael Niedermayer Aug. 26, 2019, 8:55 a.m.
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

[...]
Baptiste Coudurier Aug. 26, 2019, 5:45 p.m.
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
Paul B Mahol Aug. 26, 2019, 5:48 p.m.
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".

Patch hide | download patch | download mbox

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;
     }