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

[Michael Niedermayer]

On Mon, Apr 11, 2022 at 07:56:07PM +0200, Andreas Rheinhardt wrote:
> 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).**

What exact check do you suggest ?
(iam asking as i stumbled across this bug again and so got reminded)

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Modern terrorism, a quick summary: Need oil, start war with country that
has oil, kill hundread thousand in war. Let country fall into chaos,
be surprised about raise of fundamantalists. Drop more bombs, kill more
people, be surprised about them taking revenge and drop even more bombs
and strip your own citizens of their rights and freedoms. to be continued
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220617/7a1d156d/attachment.sig>