| Message ID | 20220329212459.23440-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/pcm_rechunk_bsf: unref packet before putting a new one in | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
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@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. > ret = ff_bsf_get_packet_ref(ctx, s->in_pkt); > if (ret == AVERROR_EOF && s->out_pkt->size) { > if (s->pad) {
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@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 I can check more explicitly for that after ff_bsf_get_packet_ref() and unref there, its more code through thx [...]
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@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. 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. Regards, Marton > > I can check more explicitly for that after ff_bsf_get_packet_ref() > and unref there, its more code through > > thx > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Its not that you shouldnt use gotos but rather that you should write > readable code and code with gotos often but not always is less readable >
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@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); [...]
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@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);
On Sun, 10 Apr 2022, 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@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 ? In the docs of av_bsf_send_packet() there is a reference to packet with no data but side data only. Also in the docs of AVPacket there is this: Encoders are allowed to output empty packets, with no compressed data, containing only side data (e.g. to update some stream parameters at the end of encoding). Regards, Marton
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@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 [...]
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@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).** - Andreas *: Which is automatically called by av_bsf_send_packet(). **: No, I am not claiming that sending a extradata-only packet to decoders works already.
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@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 [...]
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); ret = ff_bsf_get_packet_ref(ctx, s->in_pkt); if (ret == AVERROR_EOF && s->out_pkt->size) { if (s->pad) {
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@niedermayer.cc> --- libavcodec/pcm_rechunk_bsf.c | 1 + 1 file changed, 1 insertion(+)