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

[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);