| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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); >
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 [...]
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". >
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];
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 [...]
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 --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);
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(-)