[FFmpeg-devel] [PATCH] avcodec/pcm_rechunk_bsf: unref packet before putting a new one in

[Andreas Rheinhardt]

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 <michael at 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.