| Message ID | 20230416222518.21308-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avcodec/pcm_rechunk_bsf: unref packet before putting a new one in | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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@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.
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@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 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 [...]
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@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.
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@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 thx [...]
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@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
On 4/17/2023 2:13 PM, Marton Balint wrote: > > > 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@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. The bsf API accepts zero sized packets as long as pkt->data or pkt->side_data are not NULL. > 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. No, the fuzzer uses tools/target_bsf_fuzzer.c to create arbitrary packets that are passed to the bsf public API. In some cases that apparently includes zero sized packets. > > Regards, > Marton
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); 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(+)