diff mbox series

[FFmpeg-devel,2/2] avcodec/wavpack: Prevent frame format from being wrong

Message ID 20200320000336.3702-2-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/2] avcodec/cbs_jpeg: Fix infinite loop in cbs_jpeg_split_fragment()
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Michael Niedermayer March 20, 2020, 12:03 a.m. UTC
Fixes: out of array access
Fixes: 21193/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5125168956702720

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/wavpack.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Paul B Mahol March 20, 2020, 8:54 a.m. UTC | #1
lgtm

On 3/20/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
> Fixes: out of array access
> Fixes:
> 21193/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5125168956702720
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/wavpack.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
> index b27262b94e..e9c870e41e 100644
> --- a/libavcodec/wavpack.c
> +++ b/libavcodec/wavpack.c
> @@ -1488,6 +1488,7 @@ static int wavpack_decode_block(AVCodecContext *avctx,
> int block_no,
>
>          /* get output buffer */
>          wc->curr_frame.f->nb_samples = s->samples;
> +        wc->curr_frame.f->format     = avctx->sample_fmt;
>          if ((ret = ff_thread_get_buffer(avctx, &wc->curr_frame,
> AV_GET_BUFFER_FLAG_REF)) < 0)
>              return ret;
>
> --
> 2.17.1
>
> _______________________________________________
> 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".
Anton Khirnov March 20, 2020, 9:18 a.m. UTC | #2
Quoting Michael Niedermayer (2020-03-20 01:03:36)
> Fixes: out of array access
> Fixes: 21193/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5125168956702720
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/wavpack.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
> index b27262b94e..e9c870e41e 100644
> --- a/libavcodec/wavpack.c
> +++ b/libavcodec/wavpack.c
> @@ -1488,6 +1488,7 @@ static int wavpack_decode_block(AVCodecContext *avctx, int block_no,
>  
>          /* get output buffer */
>          wc->curr_frame.f->nb_samples = s->samples;
> +        wc->curr_frame.f->format     = avctx->sample_fmt;

How does this have any effect? curr_frame.f should now be clean and get
initialized from avctx->sample_fmt.
David Bryant March 20, 2020, 3:59 p.m. UTC | #3
On 3/20/20 1:54 AM, Paul B Mahol wrote:
> lgtm

Looks good to me too, sorry about that!


>
> On 3/20/20, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> Fixes: out of array access
>> Fixes:
>> 21193/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5125168956702720
>>
>> Found-by: continuous fuzzing process
>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>>  libavcodec/wavpack.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
>> index b27262b94e..e9c870e41e 100644
>> --- a/libavcodec/wavpack.c
>> +++ b/libavcodec/wavpack.c
>> @@ -1488,6 +1488,7 @@ static int wavpack_decode_block(AVCodecContext *avctx,
>> int block_no,
>>
>>          /* get output buffer */
>>          wc->curr_frame.f->nb_samples = s->samples;
>> +        wc->curr_frame.f->format     = avctx->sample_fmt;
>>          if ((ret = ff_thread_get_buffer(avctx, &wc->curr_frame,
>> AV_GET_BUFFER_FLAG_REF)) < 0)
>>              return ret;
>>
>> --
>> 2.17.1
>>
>> _______________________________________________
>> 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".
> _______________________________________________
> 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 March 20, 2020, 8:50 p.m. UTC | #4
On Fri, Mar 20, 2020 at 10:18:49AM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2020-03-20 01:03:36)
> > Fixes: out of array access
> > Fixes: 21193/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5125168956702720
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/wavpack.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
> > index b27262b94e..e9c870e41e 100644
> > --- a/libavcodec/wavpack.c
> > +++ b/libavcodec/wavpack.c
> > @@ -1488,6 +1488,7 @@ static int wavpack_decode_block(AVCodecContext *avctx, int block_no,
> >  
> >          /* get output buffer */
> >          wc->curr_frame.f->nb_samples = s->samples;
> > +        wc->curr_frame.f->format     = avctx->sample_fmt;
> 
> How does this have any effect? curr_frame.f should now be clean and get
> initialized from avctx->sample_fmt.

IIRC
The format changes between frames, so the struct is still set to the one
from the previous frame and that overrides the use of the avctx value

setting it to NONE (here or somewhere else) should work too.

thx

[...]
Anton Khirnov March 23, 2020, 4:49 p.m. UTC | #5
Quoting Michael Niedermayer (2020-03-20 21:50:18)
> On Fri, Mar 20, 2020 at 10:18:49AM +0100, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2020-03-20 01:03:36)
> > > Fixes: out of array access
> > > Fixes: 21193/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5125168956702720
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/wavpack.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
> > > index b27262b94e..e9c870e41e 100644
> > > --- a/libavcodec/wavpack.c
> > > +++ b/libavcodec/wavpack.c
> > > @@ -1488,6 +1488,7 @@ static int wavpack_decode_block(AVCodecContext *avctx, int block_no,
> > >  
> > >          /* get output buffer */
> > >          wc->curr_frame.f->nb_samples = s->samples;
> > > +        wc->curr_frame.f->format     = avctx->sample_fmt;
> > 
> > How does this have any effect? curr_frame.f should now be clean and get
> > initialized from avctx->sample_fmt.
> 
> IIRC
> The format changes between frames, so the struct is still set to the one
> from the previous frame and that overrides the use of the avctx value
> 
> setting it to NONE (here or somewhere else) should work too.

ff_thread_release_buffer() is called on that frame immediately before,
which should reset it to defaults (setting format to FMT_NONE).
David Bryant March 24, 2020, 12:08 a.m. UTC | #6
On 3/23/20 9:49 AM, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2020-03-20 21:50:18)
>> On Fri, Mar 20, 2020 at 10:18:49AM +0100, Anton Khirnov wrote:
>>> Quoting Michael Niedermayer (2020-03-20 01:03:36)
>>>> Fixes: out of array access
>>>> Fixes: 21193/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5125168956702720
>>>>
>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>> ---
>>>>  libavcodec/wavpack.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
>>>> index b27262b94e..e9c870e41e 100644
>>>> --- a/libavcodec/wavpack.c
>>>> +++ b/libavcodec/wavpack.c
>>>> @@ -1488,6 +1488,7 @@ static int wavpack_decode_block(AVCodecContext *avctx, int block_no,
>>>>  
>>>>          /* get output buffer */
>>>>          wc->curr_frame.f->nb_samples = s->samples;
>>>> +        wc->curr_frame.f->format     = avctx->sample_fmt;
>>> How does this have any effect? curr_frame.f should now be clean and get
>>> initialized from avctx->sample_fmt.
>> IIRC
>> The format changes between frames, so the struct is still set to the one
>> from the previous frame and that overrides the use of the avctx value
>>
>> setting it to NONE (here or somewhere else) should work too.
> ff_thread_release_buffer() is called on that frame immediately before,
> which should reset it to defaults (setting format to FMT_NONE).
>
I don't think the format should change between frames, so I don't understand how the format is getting set to a wacky value.

Would it be possible for me the get the triggering test case and try this myself? I searched and couldn't find it, so I assume
it's not public yet. I assume that just decoding the file should trigger the assertion, right?

Thanks!
Michael Niedermayer March 24, 2020, 12:37 a.m. UTC | #7
On Mon, Mar 23, 2020 at 05:08:23PM -0700, David Bryant wrote:
> On 3/23/20 9:49 AM, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2020-03-20 21:50:18)
> >> On Fri, Mar 20, 2020 at 10:18:49AM +0100, Anton Khirnov wrote:
> >>> Quoting Michael Niedermayer (2020-03-20 01:03:36)
> >>>> Fixes: out of array access
> >>>> Fixes: 21193/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5125168956702720
> >>>>
> >>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>> ---
> >>>>  libavcodec/wavpack.c | 1 +
> >>>>  1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
> >>>> index b27262b94e..e9c870e41e 100644
> >>>> --- a/libavcodec/wavpack.c
> >>>> +++ b/libavcodec/wavpack.c
> >>>> @@ -1488,6 +1488,7 @@ static int wavpack_decode_block(AVCodecContext *avctx, int block_no,
> >>>>  
> >>>>          /* get output buffer */
> >>>>          wc->curr_frame.f->nb_samples = s->samples;
> >>>> +        wc->curr_frame.f->format     = avctx->sample_fmt;
> >>> How does this have any effect? curr_frame.f should now be clean and get
> >>> initialized from avctx->sample_fmt.
> >> IIRC
> >> The format changes between frames, so the struct is still set to the one
> >> from the previous frame and that overrides the use of the avctx value
> >>
> >> setting it to NONE (here or somewhere else) should work too.
> > ff_thread_release_buffer() is called on that frame immediately before,
> > which should reset it to defaults (setting format to FMT_NONE).
> >
> I don't think the format should change between frames, so I don't understand how the format is getting set to a wacky value.

wavpack_decode_frame() sets the format from flags read for each frame. 


> 
> Would it be possible for me the get the triggering test case and try this myself? I searched and couldn't find it, so I assume
> it's not public yet. I assume that just decoding the file should trigger the assertion, right?

sample sent privatly

thanks

[...]
Michael Niedermayer March 24, 2020, 12:41 a.m. UTC | #8
On Mon, Mar 23, 2020 at 05:49:06PM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2020-03-20 21:50:18)
> > On Fri, Mar 20, 2020 at 10:18:49AM +0100, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2020-03-20 01:03:36)
> > > > Fixes: out of array access
> > > > Fixes: 21193/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5125168956702720
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavcodec/wavpack.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
> > > > index b27262b94e..e9c870e41e 100644
> > > > --- a/libavcodec/wavpack.c
> > > > +++ b/libavcodec/wavpack.c
> > > > @@ -1488,6 +1488,7 @@ static int wavpack_decode_block(AVCodecContext *avctx, int block_no,
> > > >  
> > > >          /* get output buffer */
> > > >          wc->curr_frame.f->nb_samples = s->samples;
> > > > +        wc->curr_frame.f->format     = avctx->sample_fmt;
> > > 
> > > How does this have any effect? curr_frame.f should now be clean and get
> > > initialized from avctx->sample_fmt.
> > 
> > IIRC
> > The format changes between frames, so the struct is still set to the one
> > from the previous frame and that overrides the use of the avctx value
> > 
> > setting it to NONE (here or somewhere else) should work too.
> 
> ff_thread_release_buffer() is called on that frame immediately before,
> which should reset it to defaults (setting format to FMT_NONE).

ff_thread_release_buffer() does not reset it in all cases
Ill send an alternative patch, which does the reset in the get side
on failure. We already have some reset code for dimensions there.

thx

[...]
Anton Khirnov March 24, 2020, 9:34 a.m. UTC | #9
Quoting Michael Niedermayer (2020-03-24 01:41:24)
> On Mon, Mar 23, 2020 at 05:49:06PM +0100, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2020-03-20 21:50:18)
> > > On Fri, Mar 20, 2020 at 10:18:49AM +0100, Anton Khirnov wrote:
> > > > Quoting Michael Niedermayer (2020-03-20 01:03:36)
> > > > > Fixes: out of array access
> > > > > Fixes: 21193/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5125168956702720
> > > > > 
> > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > ---
> > > > >  libavcodec/wavpack.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
> > > > > index b27262b94e..e9c870e41e 100644
> > > > > --- a/libavcodec/wavpack.c
> > > > > +++ b/libavcodec/wavpack.c
> > > > > @@ -1488,6 +1488,7 @@ static int wavpack_decode_block(AVCodecContext *avctx, int block_no,
> > > > >  
> > > > >          /* get output buffer */
> > > > >          wc->curr_frame.f->nb_samples = s->samples;
> > > > > +        wc->curr_frame.f->format     = avctx->sample_fmt;
> > > > 
> > > > How does this have any effect? curr_frame.f should now be clean and get
> > > > initialized from avctx->sample_fmt.
> > > 
> > > IIRC
> > > The format changes between frames, so the struct is still set to the one
> > > from the previous frame and that overrides the use of the avctx value
> > > 
> > > setting it to NONE (here or somewhere else) should work too.
> > 
> > ff_thread_release_buffer() is called on that frame immediately before,
> > which should reset it to defaults (setting format to FMT_NONE).
> 
> ff_thread_release_buffer() does not reset it in all cases

Then I would say that is the bug that should be fixed.
ff_thread_release_buffer() should have the same semantics as
av_frame_unref() and always reset the frame to a clean state.
diff mbox series

Patch

diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
index b27262b94e..e9c870e41e 100644
--- a/libavcodec/wavpack.c
+++ b/libavcodec/wavpack.c
@@ -1488,6 +1488,7 @@  static int wavpack_decode_block(AVCodecContext *avctx, int block_no,
 
         /* get output buffer */
         wc->curr_frame.f->nb_samples = s->samples;
+        wc->curr_frame.f->format     = avctx->sample_fmt;
         if ((ret = ff_thread_get_buffer(avctx, &wc->curr_frame, AV_GET_BUFFER_FLAG_REF)) < 0)
             return ret;