diff mbox

[FFmpeg-devel] avcodec/cbs_h2645: Fail on all 0 NAL units

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

Commit Message

Michael Niedermayer Dec. 12, 2019, 5:43 p.m. UTC
Fixes: assertion failure
Fixes: 19286/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_REDUNDANT_PPS_fuzzer-5707990724509696

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

Comments

Andriy Gelman Dec. 12, 2019, 10:15 p.m. UTC | #1
On Thu, 12. Dec 18:43, Michael Niedermayer wrote:
> Fixes: assertion failure
> Fixes: 19286/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_REDUNDANT_PPS_fuzzer-5707990724509696
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/cbs_h2645.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> index 5f71d80584..5e0f4a9d98 100644
> --- a/libavcodec/cbs_h2645.c
> +++ b/libavcodec/cbs_h2645.c
> @@ -568,7 +568,8 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>          // Remove trailing zeroes.
>          while (size > 0 && nal->data[size - 1] == 0)
>              --size;
> -        av_assert0(size > 0);

> +        if (size == 0)
> +            return AVERROR_INVALIDDATA;

I suppose there could still be some valid nal units in this packet list.

Can you just continue in the loop and not insert this particular nal unit into
frag?
James Almer Dec. 12, 2019, 10:52 p.m. UTC | #2
On 12/12/2019 7:15 PM, Andriy Gelman wrote:
> On Thu, 12. Dec 18:43, Michael Niedermayer wrote:
>> Fixes: assertion failure
>> Fixes: 19286/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_REDUNDANT_PPS_fuzzer-5707990724509696
>>
>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>>  libavcodec/cbs_h2645.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>> index 5f71d80584..5e0f4a9d98 100644
>> --- a/libavcodec/cbs_h2645.c
>> +++ b/libavcodec/cbs_h2645.c
>> @@ -568,7 +568,8 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>>          // Remove trailing zeroes.
>>          while (size > 0 && nal->data[size - 1] == 0)
>>              --size;
>> -        av_assert0(size > 0);
> 
>> +        if (size == 0)
>> +            return AVERROR_INVALIDDATA;
> 
> I suppose there could still be some valid nal units in this packet list.
> 
> Can you just continue in the loop and not insert this particular nal unit into
> frag? 

This is preferable than aborting, yes. If it can't be skipped in
ff_h2645_packet_split(), the place I'd consider proper for this (cbs is
not the only user of that function), then it should be skipped here.
Michael Niedermayer Dec. 13, 2019, 5:10 p.m. UTC | #3
On Thu, Dec 12, 2019 at 05:15:12PM -0500, Andriy Gelman wrote:
> On Thu, 12. Dec 18:43, Michael Niedermayer wrote:
> > Fixes: assertion failure
> > Fixes: 19286/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_REDUNDANT_PPS_fuzzer-5707990724509696
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/cbs_h2645.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> > index 5f71d80584..5e0f4a9d98 100644
> > --- a/libavcodec/cbs_h2645.c
> > +++ b/libavcodec/cbs_h2645.c
> > @@ -568,7 +568,8 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
> >          // Remove trailing zeroes.
> >          while (size > 0 && nal->data[size - 1] == 0)
> >              --size;
> > -        av_assert0(size > 0);
> 
> > +        if (size == 0)
> > +            return AVERROR_INVALIDDATA;
> 
> I suppose there could still be some valid nal units in this packet list.
> 
> Can you just continue in the loop and not insert this particular nal unit into
> frag? 

sure, will do that

thx

[...]
diff mbox

Patch

diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
index 5f71d80584..5e0f4a9d98 100644
--- a/libavcodec/cbs_h2645.c
+++ b/libavcodec/cbs_h2645.c
@@ -568,7 +568,8 @@  static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
         // Remove trailing zeroes.
         while (size > 0 && nal->data[size - 1] == 0)
             --size;
-        av_assert0(size > 0);
+        if (size == 0)
+            return AVERROR_INVALIDDATA;
 
         ref = (nal->data == nal->raw_data) ? frag->data_ref
                                            : packet->rbsp.rbsp_buffer_ref;