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

[Michael Niedermayer]

On Sun, Apr 10, 2022 at 11:46:24AM -0300, James Almer wrote:
> 
> 
> 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

if we reject these allready then it should be fine for bsfs too
will apply your suggestion with you as author after testing

thx

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

"You are 36 times more likely to die in a bathtub than at the hands of a
terrorist. Also, you are 2.5 times more likely to become a president and
2 times more likely to become an astronaut, than to die in a terrorist
attack." -- Thoughty2

-------------- 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/20220411/9cc2f62f/attachment.sig>