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

[Marton Balint]

On Mon, 17 Apr 2023, Michael Niedermayer wrote:

> On Mon, Apr 17, 2023 at 08:28:37AM -0300, James Almer wrote:
>> On 4/17/2023 8:26 AM, Michael Niedermayer wrote:
>>> On Sun, Apr 16, 2023 at 07:57:00PM -0300, James Almer wrote:
>>>> On 4/16/2023 7:25 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 108d9e90b99..3f43934fe9a 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 could be pointing to a bug in the above code, and unreffing here like
>>>> this would result in data loss.
>>>>
>>>> Does the following change also fix the memleak?
>>>>
>>>>> diff --git a/libavcodec/pcm_rechunk_bsf.c b/libavcodec/pcm_rechunk_bsf.c
>>>>> index 108d9e90b9..032f914916 100644
>>>>> --- a/libavcodec/pcm_rechunk_bsf.c
>>>>> +++ b/libavcodec/pcm_rechunk_bsf.c
>>>>> @@ -151,7 +151,8 @@ static int rechunk_filter(AVBSFContext *ctx, AVPacket *pkt)
>>>>>                   av_packet_move_ref(pkt, s->in_pkt);
>>>>>                   return send_packet(s, nb_samples, pkt);
>>>>>               }
>>>>> -        }
>>>>> +        } else
>>>>> +            av_packet_unref(s->in_pkt);
>>>>>
>>>>>           ret = ff_bsf_get_packet_ref(ctx, s->in_pkt);
>>>>>           if (ret == AVERROR_EOF && s->out_pkt->size) {
>>>>
>>>> If it does then a zero sized packet with either only side data or that went
>>>> through the av_packet_make_refcounted() in av_bsf_send_packet() that left it
>>>> with an allocated padding buffer was fed to this bsf.
>>>
>>> yes this works too
>>> and this memleak is open since a year, its the 2nd time i submited this
>>
>> Yes, i had a sense of deja vu.
>>
>>> patch, last time the discussions didnt lead to any consensus on what to do
>>> I hope this time some fix from someone ends up in git
>>>
>>> thx
>>
>> Just apply the suggested change i made above.
>
> ok

That is fine by me as well to fix the leak.

However I still wonder if it is valid to pass empty packets around. AFAIK 
the only documented case is when some final side data is passed at the end 
of the encoding, and these fuzzing issues are typically not that, but 
that some demuxer generates 0-sized packets for corrupt files.

Regards,
Marton