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

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

[Michael Niedermayer]

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);
         ret = ff_bsf_get_packet_ref(ctx, s->in_pkt);
         if (ret == AVERROR_EOF && s->out_pkt->size) {
             if (s->pad) {
-- 
2.17.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[James Almer]

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) {
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1483 bytes --]

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


[...]
-- 
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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Marton Balint]

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
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3210 bytes --]

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);


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

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[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);
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Marton Balint]

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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3829 bytes --]

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

[...]
-- 
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


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Andreas Rheinhardt]

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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 6264 bytes --]

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

[...]
-- 
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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".