On 10/9/2020 7:41 PM, Andreas Rheinhardt wrote: > Andreas Rheinhardt: >> James Almer: >>> On 10/9/2020 1:36 PM, Michael Niedermayer wrote: >>>> On Thu, Oct 08, 2020 at 10:25:11AM -0300, James Almer wrote: >>>>> Allocate it only when needed, and instead of giving it a fixed initial >>>>> size >>>>> that's doubled on each realloc, ensure it's always big enough for the NAL >>>>> currently being parsed. >>>>> >>>>> 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: James Almer <jamr...@gmail.com> >>>>> --- >>>>> libavcodec/h2645_parse.c | 28 ++++++++++------------------ >>>>> 1 file changed, 10 insertions(+), 18 deletions(-) >>>>> >>>>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c >>>>> index 0f98b49fbe..f5c76323c1 100644 >>>>> --- a/libavcodec/h2645_parse.c >>>>> +++ b/libavcodec/h2645_parse.c >>>>> @@ -108,22 +108,20 @@ 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) { >>>>> - nal->skipped_bytes_pos_size *= 2; >>>>> - av_assert0(nal->skipped_bytes_pos_size >= >>>>> nal->skipped_bytes); >>>>> - av_reallocp_array(&nal->skipped_bytes_pos, >>>>> + nal->skipped_bytes++; >>>>> + if (nal->skipped_bytes_pos_size < nal->skipped_bytes) { >>>>> + nal->skipped_bytes_pos_size = length / 3; >>>> >>>> This would allocate a much larger than needed skipped_bytes_pos for >>>> probably nearly all of the real world h264 files to fix an issue with >>>> crafted streams. >>>> >>>> The initial size should be choosen so as to be large enough for real world >>>> streams. If that turns out to be too small then length /3 sounds reasonable >>>> as the first reallocation. >>> >>> At most 1/3 of the bytes in a NAL would be removed by this code, hence >>> length / 3. I could make it length / 16 like in your fix if you prefer, >>> or maybe nal->skipped_bytes * 2 to keep it closer to the current >>> behavior, but real world samples don't have more than a handful of NALs >>> per packet, and not all have escaped bytes that need to be removed (If >>> there are none, nothing will be allocated). >>> >>> I looked at a 2160p hevc sample, and the biggest packet is about 26kb, >>> which assuming it's a single slice NAL, it will be about 8kb for the >>> skipped_bytes_pos buffer with length / 3. >>> >> Your calculation seems to be off by a factor of four because you ignore >> that every byte removed needs an entry of type int*. Furthermore 26kB (I >> presume you meant kB and not kb) seems very, very small for a normal >> file these days; it feels more like 320p streaming or so. >> >> Furthermore, there is a bigger issue with your patch: It can lead to >> quadratic memory consumption for annex B input: Because the length of >> the NAL units is not known in advance, length in the above code refers >> to the length from the beginning of the NAL unit to the end of the >> packet. So this code will allocate gigantic amounts of memory for a >> packet containing lots of small NAL units, each with a single 0x000003. >> This is an issue similar to the one fixed by >> 03b82b3ab9883cef017e513c7d0b3b986b3b3e7b. >> > > How about the following approach which mimics 03b82b3ab: Just like the > RBSP buffer the skipped_bytes_pos buffer is owned by an H2645Packet and > shared between its NALs; each NAL only retains the index of the first > entry in the list that belongs to it and the amount of entries of the > list that belong to it. H2645Packet gets a struct equivalent to > H2645RBSP for it and ff_h2645_extract_rbsp() gets a pointer to this as > new parameter; if it is NULL, it means that the caller does not want to > know the position of the removed 0x03. If not, the buffer is reallocated > as needed (with overallocating, of course). > > decode_nal_unit() as well as hls_slice_data_wpp() in hevcdec.c would > need to be adapted to accept the skipped_bytes_pos buffer via other > newly added function parameters.
Patch welcome. That seems overdesigned to maybe save some memory and I'm not interested in implementing it. > > - Andreas > > PS: Orthogonally to this, one could add a flag to > ff_h2645_packet_split() to disable skipped_bytes_pos altogether. ff_h2645_packet_split() already has two optional functionality arguments, so a third will make the signature really bloated. Maybe they can all be replaced by a single flags argument, and use constants to request small_padding, use_ref and this new one. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
