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

[Michael Niedermayer]

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);
+    }
+
     if (bsfi->eof) {
         av_log(ctx, AV_LOG_ERROR, "A non-NULL packet sent after an EOF.\n");
         return AVERROR(EINVAL);


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

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- 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/20220410/891a6437/attachment.sig>