James Almer: > > > On 4/10/2022 11:14 AM, Michael Niedermayer wrote: >> On Sat, Apr 09, 2022 at 08:56:05PM +0200, Marton Balint wrote: >>> >>> >>> On Wed, 30 Mar 2022, Michael Niedermayer wrote: >>> >>>> On Tue, Mar 29, 2022 at 06:33:06PM -0300, James Almer wrote: >>>>> On 3/29/2022 6:24 PM, Michael Niedermayer wrote: >>>>>> Fixes: memleak >>>>>> Fixes: >>>>>> 45982/clusterfuzz-testcase-minimized-ffmpeg_BSF_PCM_RECHUNK_fuzzer-5562089618407424 >>>>>> >>>>>> >>>>>> Found-by: continuous fuzzing process >>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>>>> Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc> >>>>>> --- >>>>>> libavcodec/pcm_rechunk_bsf.c | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/libavcodec/pcm_rechunk_bsf.c >>>>>> b/libavcodec/pcm_rechunk_bsf.c >>>>>> index 108d9e90b9..3f43934fe9 100644 >>>>>> --- a/libavcodec/pcm_rechunk_bsf.c >>>>>> +++ b/libavcodec/pcm_rechunk_bsf.c >>>>>> @@ -153,6 +153,7 @@ static int rechunk_filter(AVBSFContext *ctx, >>>>>> AVPacket *pkt) >>>>>> } >>>>>> } >>>>>> + av_packet_unref(s->in_pkt); >>>>> >>>>> This looks to me like it revealed a bug in the code above, which is >>>>> meant to >>>>> ensure s->in_pkt will be blank at this point. It should be fixed there >>>>> instead. >>>> >>>> IIRC the problem was a input packet with size 0 >>>> the code seems to assume 0 meaning no packet >>> >>> Is that valid here? The docs says that the encoders can generate 0 sized >>> packets if there is side data in them. However - the PCM rechunk BSF >>> using >>> PCM packets - I am not sure this is intentional here. >> >> where exactly is this written ? >> >> >>> >>> So overall it looks to me that the PCM rechunk BSF should reject 0 sized >>> packets with AVERROR_INVALIDDATA, and the encoder or demuxer which >>> produces >>> the 0 sized packets should be fixed. >> >> There is no encoder or demuxer. There is just the fuzzer which excercies >> the whole space of allowed parameters of the BSFs >> and either such zero packets are valid or they are not. >> if not, then a check could be added to av_bsf_send_packet() that feels a >> bit broad though. >> >> i can add a check to pcm_rechunk_bsf but it feels a bit odd if these are >> valid and just not supposed to come out of the encoders >> >> do you see some problem with these packets ? >> that makes it better to just reject them ? >> >> (error you enountered a packet which makes no difference seems a bit odd >> in its own too. That probably should only be a warning) >> thx >> >> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c >> index 42cc1b5ab0..ae16112285 100644 >> --- a/libavcodec/bsf.c >> +++ b/libavcodec/bsf.c >> @@ -212,6 +212,11 @@ int av_bsf_send_packet(AVBSFContext *ctx, >> AVPacket *pkt) >> return 0; >> } >> + if (pkt->size == 0 && pkt->side_data_elems == 0) { >> + av_log(ctx, AV_LOG_ERROR, "Zero packet is not allowed.\n"); >> + return AVERROR(EINVAL); >> + } > > To make this behave like avcodec_send_packet(), it should instead be > > diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c > index 42cc1b5ab0..01ed9db258 100644 > --- a/libavcodec/bsf.c > +++ b/libavcodec/bsf.c > @@ -205,6 +205,9 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket > *pkt) > FFBSFContext *const bsfi = ffbsfcontext(ctx); > int ret; > > + if (pkt && !pkt->size && pkt->data) > + return AVERROR(EINVAL); > + > if (!pkt || IS_EMPTY(pkt)) { > if (pkt) > av_packet_unref(pkt);
1. None of your patches would actually fix the memleak: Packets with data == NULL, size == 0 and side_data_elems != 0 would still slip through and cause leaks (of the side data). 2. I don't see why the behaviour of avcodec_send_packet should be the model to emulate here. (E.g. av_packet_make_refcounted* creates packets with size == 0 and data set (pointing to a buffer of size AV_INPUT_BUFFER_PADDING_SIZE) when the packet has no data.) 3. avcodec_send_packet ignores its own documentation: Packets with data == NULL, size == 0, but side_data_elems != 0 are not considered eof/flush packets. The reason for this is that avcodec_send_packet offloads the flushing-decision to the underlying BSF and the BSF API has slightly different semantics. IMO we should only accept NULL packets/frames as flush packets as this is the only thing that is always guaranteed to be an intentional flush packet; any more additions to AVPacket might otherwise force us to redefine what a flush packet is. 4. The behaviour of avcodec_send_packet has a weird consequence: Say you get new extradata via an out-of-band mechanism and want to send it to the decoder via packet side-data. Given that it is an out-of-band mechanism you don't have an ordinary packet yet to attach it, so you simply send it via a packet with size 0. But given that packets with data == NULL and size == 0 are documented to be flush packets (they aren't in practice) and flushing is not intended here, you use a packet with size == 0 and data != NULL. And then avcodec_send_packet errors out, forcing you to abandon this approach and attach the side-data to the next real packet (which is sligtly less natural).** - Andreas *: Which is automatically called by av_bsf_send_packet(). **: No, I am not claiming that sending a extradata-only packet to decoders works already. _______________________________________________ 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".
