diff mbox series

[FFmpeg-devel,1/3] avcodec/h2645_parse: Limit initial skipped_bytes_pos_size to nal size / 16

Message ID 20201004194143.10582-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/3] avcodec/h2645_parse: Limit initial skipped_bytes_pos_size to nal size / 16
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Michael Niedermayer Oct. 4, 2020, 7:41 p.m. UTC
Fixes: OOM
Fixes: 23817/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_METADATA_fuzzer-6300869057576960

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

Comments

James Almer Oct. 4, 2020, 8:04 p.m. UTC | #1
On 10/4/2020 4:41 PM, Michael Niedermayer wrote:
> Fixes: OOM
> Fixes: 23817/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_METADATA_fuzzer-6300869057576960
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/h2645_parse.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
> index 0f98b49fbe..61105a6eb5 100644
> --- a/libavcodec/h2645_parse.c
> +++ b/libavcodec/h2645_parse.c
> @@ -467,7 +467,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>              memset(pkt->nals + pkt->nals_allocated, 0, sizeof(*pkt->nals));
>  
>              nal = &pkt->nals[pkt->nb_nals];
> -            nal->skipped_bytes_pos_size = 1024; // initial buffer size
> +            nal->skipped_bytes_pos_size = FFMIN(1024, 1+(extract_length>>4)); // initial buffer size

Why is there even an initial buffer? Why not just let
ff_h2645_extract_rbsp() allocate it when needed?

You could even replace the av_reallocp_array() call there that doubles
the buffer size each time with an av_fast_realloc(). Should also help
with memory usage.

>              nal->skipped_bytes_pos = av_malloc_array(nal->skipped_bytes_pos_size, sizeof(*nal->skipped_bytes_pos));
>              if (!nal->skipped_bytes_pos)
>                  return AVERROR(ENOMEM);
>
Michael Niedermayer Oct. 4, 2020, 8:57 p.m. UTC | #2
On Sun, Oct 04, 2020 at 05:04:05PM -0300, James Almer wrote:
> On 10/4/2020 4:41 PM, Michael Niedermayer wrote:
> > Fixes: OOM
> > Fixes: 23817/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_METADATA_fuzzer-6300869057576960
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/h2645_parse.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
> > index 0f98b49fbe..61105a6eb5 100644
> > --- a/libavcodec/h2645_parse.c
> > +++ b/libavcodec/h2645_parse.c
> > @@ -467,7 +467,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
> >              memset(pkt->nals + pkt->nals_allocated, 0, sizeof(*pkt->nals));
> >  
> >              nal = &pkt->nals[pkt->nb_nals];
> > -            nal->skipped_bytes_pos_size = 1024; // initial buffer size
> > +            nal->skipped_bytes_pos_size = FFMIN(1024, 1+(extract_length>>4)); // initial buffer size
> 
> Why is there even an initial buffer? Why not just let
> ff_h2645_extract_rbsp() allocate it when needed?

i wondered that too and assumed it was done that way to avoid spending
cpu cycles on reallocations later



[...]
James Almer Oct. 4, 2020, 9:02 p.m. UTC | #3
On 10/4/2020 5:57 PM, Michael Niedermayer wrote:
> On Sun, Oct 04, 2020 at 05:04:05PM -0300, James Almer wrote:
>> On 10/4/2020 4:41 PM, Michael Niedermayer wrote:
>>> Fixes: OOM
>>> Fixes: 23817/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_METADATA_fuzzer-6300869057576960
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/h2645_parse.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
>>> index 0f98b49fbe..61105a6eb5 100644
>>> --- a/libavcodec/h2645_parse.c
>>> +++ b/libavcodec/h2645_parse.c
>>> @@ -467,7 +467,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>>>              memset(pkt->nals + pkt->nals_allocated, 0, sizeof(*pkt->nals));
>>>  
>>>              nal = &pkt->nals[pkt->nb_nals];
>>> -            nal->skipped_bytes_pos_size = 1024; // initial buffer size
>>> +            nal->skipped_bytes_pos_size = FFMIN(1024, 1+(extract_length>>4)); // initial buffer size
>>
>> Why is there even an initial buffer? Why not just let
>> ff_h2645_extract_rbsp() allocate it when needed?
> 
> i wondered that too and assumed it was done that way to avoid spending
> cpu cycles on reallocations later

Many streams don't need to escape bytes, so for those, allocating
anything at all is a waste. And IMO by using av_fast_realloc() in
ff_h2645_extract_rbsp() there's no need for a big enough initial buffer
either.

> 
> 
> 
> [...]
> 
> 
> _______________________________________________
> 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 Oct. 5, 2020, 4:30 a.m. UTC | #4
On 10/4/2020 6:02 PM, James Almer wrote:
> On 10/4/2020 5:57 PM, Michael Niedermayer wrote:
>> On Sun, Oct 04, 2020 at 05:04:05PM -0300, James Almer wrote:
>>> On 10/4/2020 4:41 PM, Michael Niedermayer wrote:
>>>> Fixes: OOM
>>>> Fixes: 23817/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_METADATA_fuzzer-6300869057576960
>>>>
>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>> ---
>>>>  libavcodec/h2645_parse.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
>>>> index 0f98b49fbe..61105a6eb5 100644
>>>> --- a/libavcodec/h2645_parse.c
>>>> +++ b/libavcodec/h2645_parse.c
>>>> @@ -467,7 +467,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>>>>              memset(pkt->nals + pkt->nals_allocated, 0, sizeof(*pkt->nals));
>>>>  
>>>>              nal = &pkt->nals[pkt->nb_nals];
>>>> -            nal->skipped_bytes_pos_size = 1024; // initial buffer size
>>>> +            nal->skipped_bytes_pos_size = FFMIN(1024, 1+(extract_length>>4)); // initial buffer size
>>>
>>> Why is there even an initial buffer? Why not just let
>>> ff_h2645_extract_rbsp() allocate it when needed?
>>
>> i wondered that too and assumed it was done that way to avoid spending
>> cpu cycles on reallocations later
> 
> Many streams don't need to escape bytes, so for those, allocating
> anything at all is a waste. And IMO by using av_fast_realloc() in
> ff_h2645_extract_rbsp() there's no need for a big enough initial buffer
> either.

On second thought, even if most av_fast_realloc() calls will be no-ops,
they may end up being way too many to the point the current behavior
would be more efficient.

Does moving the allocation of the initial buffer to
ff_h2645_extract_rbsp() also help with this sample? I assume it's one
where it generates an absurd amount of NAL units in the packet, but most
would probably be small enough that they will not really contain bytes
that need to be escaped, and as such not require a skipped_bytes_pos buffer.

Something like

> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
> index 0f98b49fbe..3e59fcaa5b 100644
> --- a/libavcodec/h2645_parse.c
> +++ b/libavcodec/h2645_parse.c
> @@ -108,9 +108,10 @@ int ff_h2645_extract_rbsp(const uint8_t *src, int length,
>                  dst[di++] = 0;
>                  si       += 3;
> 
> -                if (nal->skipped_bytes_pos) {
>                      nal->skipped_bytes++;
>                      if (nal->skipped_bytes_pos_size < nal->skipped_bytes) {
> +                        if (!nal->skipped_bytes_pos_size)
> +                            nal->skipped_bytes_pos_size = 512;
>                          nal->skipped_bytes_pos_size *= 2;
>                          av_assert0(nal->skipped_bytes_pos_size >= nal->skipped_bytes);
>                          av_reallocp_array(&nal->skipped_bytes_pos,
> @@ -123,7 +124,7 @@ int ff_h2645_extract_rbsp(const uint8_t *src, int length,
>                      }
>                      if (nal->skipped_bytes_pos)
>                          nal->skipped_bytes_pos[nal->skipped_bytes-1] = di - 1;
> -                }
> +
>                  continue;
>              } else // next start code
>                  goto nsc;
> @@ -466,12 +467,6 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>              pkt->nals = tmp;
>              memset(pkt->nals + pkt->nals_allocated, 0, sizeof(*pkt->nals));
> 
> -            nal = &pkt->nals[pkt->nb_nals];
> -            nal->skipped_bytes_pos_size = 1024; // initial buffer size
> -            nal->skipped_bytes_pos = av_malloc_array(nal->skipped_bytes_pos_size, sizeof(*nal->skipped_bytes_pos));
> -            if (!nal->skipped_bytes_pos)
> -                return AVERROR(ENOMEM);
> -
>              pkt->nals_allocated = new_size;
>          }
>          nal = &pkt->nals[pkt->nb_nals];
Michael Niedermayer Oct. 5, 2020, 9:43 p.m. UTC | #5
On Mon, Oct 05, 2020 at 01:30:07AM -0300, James Almer wrote:
> On 10/4/2020 6:02 PM, James Almer wrote:
> > On 10/4/2020 5:57 PM, Michael Niedermayer wrote:
> >> On Sun, Oct 04, 2020 at 05:04:05PM -0300, James Almer wrote:
> >>> On 10/4/2020 4:41 PM, Michael Niedermayer wrote:
> >>>> Fixes: OOM
> >>>> Fixes: 23817/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_METADATA_fuzzer-6300869057576960
> >>>>
> >>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>> ---
> >>>>  libavcodec/h2645_parse.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
> >>>> index 0f98b49fbe..61105a6eb5 100644
> >>>> --- a/libavcodec/h2645_parse.c
> >>>> +++ b/libavcodec/h2645_parse.c
> >>>> @@ -467,7 +467,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
> >>>>              memset(pkt->nals + pkt->nals_allocated, 0, sizeof(*pkt->nals));
> >>>>  
> >>>>              nal = &pkt->nals[pkt->nb_nals];
> >>>> -            nal->skipped_bytes_pos_size = 1024; // initial buffer size
> >>>> +            nal->skipped_bytes_pos_size = FFMIN(1024, 1+(extract_length>>4)); // initial buffer size
> >>>
> >>> Why is there even an initial buffer? Why not just let
> >>> ff_h2645_extract_rbsp() allocate it when needed?
> >>
> >> i wondered that too and assumed it was done that way to avoid spending
> >> cpu cycles on reallocations later
> > 
> > Many streams don't need to escape bytes, so for those, allocating
> > anything at all is a waste. And IMO by using av_fast_realloc() in
> > ff_h2645_extract_rbsp() there's no need for a big enough initial buffer
> > either.
> 
> On second thought, even if most av_fast_realloc() calls will be no-ops,
> they may end up being way too many to the point the current behavior
> would be more efficient.
> 
> Does moving the allocation of the initial buffer to
> ff_h2645_extract_rbsp() also help with this sample? I assume it's one
> where it generates an absurd amount of NAL units in the packet, but most
> would probably be small enough that they will not really contain bytes
> that need to be escaped, and as such not require a skipped_bytes_pos buffer.

your variant below works too, yes
the fuzzer testcase is a gazzilion of 1byte NAL units IIRC

thx

[...]
James Almer Oct. 5, 2020, 10:17 p.m. UTC | #6
On 10/5/2020 6:43 PM, Michael Niedermayer wrote:
> On Mon, Oct 05, 2020 at 01:30:07AM -0300, James Almer wrote:
>> On 10/4/2020 6:02 PM, James Almer wrote:
>>> On 10/4/2020 5:57 PM, Michael Niedermayer wrote:
>>>> On Sun, Oct 04, 2020 at 05:04:05PM -0300, James Almer wrote:
>>>>> On 10/4/2020 4:41 PM, Michael Niedermayer wrote:
>>>>>> Fixes: OOM
>>>>>> Fixes: 23817/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_METADATA_fuzzer-6300869057576960
>>>>>>
>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>>> ---
>>>>>>  libavcodec/h2645_parse.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
>>>>>> index 0f98b49fbe..61105a6eb5 100644
>>>>>> --- a/libavcodec/h2645_parse.c
>>>>>> +++ b/libavcodec/h2645_parse.c
>>>>>> @@ -467,7 +467,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>>>>>>              memset(pkt->nals + pkt->nals_allocated, 0, sizeof(*pkt->nals));
>>>>>>  
>>>>>>              nal = &pkt->nals[pkt->nb_nals];
>>>>>> -            nal->skipped_bytes_pos_size = 1024; // initial buffer size
>>>>>> +            nal->skipped_bytes_pos_size = FFMIN(1024, 1+(extract_length>>4)); // initial buffer size
>>>>>
>>>>> Why is there even an initial buffer? Why not just let
>>>>> ff_h2645_extract_rbsp() allocate it when needed?
>>>>
>>>> i wondered that too and assumed it was done that way to avoid spending
>>>> cpu cycles on reallocations later
>>>
>>> Many streams don't need to escape bytes, so for those, allocating
>>> anything at all is a waste. And IMO by using av_fast_realloc() in
>>> ff_h2645_extract_rbsp() there's no need for a big enough initial buffer
>>> either.
>>
>> On second thought, even if most av_fast_realloc() calls will be no-ops,
>> they may end up being way too many to the point the current behavior
>> would be more efficient.
>>
>> Does moving the allocation of the initial buffer to
>> ff_h2645_extract_rbsp() also help with this sample? I assume it's one
>> where it generates an absurd amount of NAL units in the packet, but most
>> would probably be small enough that they will not really contain bytes
>> that need to be escaped, and as such not require a skipped_bytes_pos buffer.
> 
> your variant below works too, yes

What do you think about setting instead nal->skipped_bytes_pos_size to
length / 3 when it needs to be resized? The max possible amount of bytes
it will skip/strip per NAL is one third of the total NAL size.

It's probably better than my previous suggestion, as it still removes
the initial buffer outside of ff_h2645_extract_rbsp() and also avoids
the unconditional check for nal->skipped_bytes_pos_size == 0 and does
not duplicate the buffer's size on each realloc.

> the fuzzer testcase is a gazzilion of 1byte NAL units IIRC
> 
> 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".
>
diff mbox series

Patch

diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
index 0f98b49fbe..61105a6eb5 100644
--- a/libavcodec/h2645_parse.c
+++ b/libavcodec/h2645_parse.c
@@ -467,7 +467,7 @@  int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
             memset(pkt->nals + pkt->nals_allocated, 0, sizeof(*pkt->nals));
 
             nal = &pkt->nals[pkt->nb_nals];
-            nal->skipped_bytes_pos_size = 1024; // initial buffer size
+            nal->skipped_bytes_pos_size = FFMIN(1024, 1+(extract_length>>4)); // initial buffer size
             nal->skipped_bytes_pos = av_malloc_array(nal->skipped_bytes_pos_size, sizeof(*nal->skipped_bytes_pos));
             if (!nal->skipped_bytes_pos)
                 return AVERROR(ENOMEM);