diff mbox series

[FFmpeg-devel,3/4] avformat/dvdvideodec: Only free allocated buffers

Message ID AS8P250MB07449C56EF9596C6C52AE2E28F5D2@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM
State Accepted
Commit 77b26bf4b63b186cf1ea040d1a0ffa593b6ae700
Headers show
Series [FFmpeg-devel,1/4] avformat/dvdvideodec: Explicitly return 0 on success | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Andreas Rheinhardt March 2, 2024, 3:43 p.m. UTC 
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
How has this slipped through?
Btw: This patchset is based upon code inspection, not on reading
actual files.

 libavformat/dvdvideodec.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Andreas Rheinhardt March 2, 2024, 3:47 p.m. UTC | #1 
Andreas Rheinhardt:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> How has this slipped through?

Answer: AVIOContext starts with an AVClass* that is unset when using
ffio_init_context(). Therefore the av_freep() leads to freeing of a NULL
pointer which does not segfault.

> Btw: This patchset is based upon code inspection, not on reading
> actual files.
> 
>  libavformat/dvdvideodec.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
> index ef2d4e6df4..f2f23affb2 100644
> --- a/libavformat/dvdvideodec.c
> +++ b/libavformat/dvdvideodec.c
> @@ -1202,7 +1202,6 @@ static void dvdvideo_subdemux_close(AVFormatContext *s)
>      DVDVideoDemuxContext *c = s->priv_data;
>  
>      av_freep(&c->mpeg_pb.pub.buffer);
> -    av_freep(&c->mpeg_pb);
>      avformat_close_input(&c->mpeg_ctx);
>  }
>
James Almer March 2, 2024, 3:51 p.m. UTC | #2 
On 3/2/2024 12:47 PM, Andreas Rheinhardt wrote:
> Andreas Rheinhardt:
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> How has this slipped through?
> 
> Answer: AVIOContext starts with an AVClass* that is unset when using
> ffio_init_context(). Therefore the av_freep() leads to freeing of a NULL
> pointer which does not segfault.

Would setting s->av_class to &ff_avio_options in ffio_init_context() 
prevent this from happening again?

> 
>> Btw: This patchset is based upon code inspection, not on reading
>> actual files.
>>
>>   libavformat/dvdvideodec.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
>> index ef2d4e6df4..f2f23affb2 100644
>> --- a/libavformat/dvdvideodec.c
>> +++ b/libavformat/dvdvideodec.c
>> @@ -1202,7 +1202,6 @@ static void dvdvideo_subdemux_close(AVFormatContext *s)
>>       DVDVideoDemuxContext *c = s->priv_data;
>>   
>>       av_freep(&c->mpeg_pb.pub.buffer);
>> -    av_freep(&c->mpeg_pb);
>>       avformat_close_input(&c->mpeg_ctx);
>>   }
>>   
> 
> _______________________________________________
> 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".
Andreas Rheinhardt March 2, 2024, 4:19 p.m. UTC | #3 
James Almer:
> On 3/2/2024 12:47 PM, Andreas Rheinhardt wrote:
>> Andreas Rheinhardt:
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>>> ---
>>> How has this slipped through?
>>
>> Answer: AVIOContext starts with an AVClass* that is unset when using
>> ffio_init_context(). Therefore the av_freep() leads to freeing of a NULL
>> pointer which does not segfault.
> 
> Would setting s->av_class to &ff_avio_options in ffio_init_context()
> prevent this from happening again?
> 

ff_avio_options is not an AVClass, so this is wrong; setting it to
ff_avio_class would also be wrong, because said class is only to be used
with the URLProtocol-based AVIOContexts (the child_next and
child_class_iterate callbacks are designed for this; the former returns
the AVIOContext's opaque, believing it to point to an URLContext, yet in
general it need not point to an AVClass-enabled struct at all).

In fact, ff_avio_options etc. should be moved to avio.c. I'll look into
this.

>>
>>> Btw: This patchset is based upon code inspection, not on reading
>>> actual files.
>>>
>>>   libavformat/dvdvideodec.c | 1 -
>>>   1 file changed, 1 deletion(-)
>>>
>>> diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
>>> index ef2d4e6df4..f2f23affb2 100644
>>> --- a/libavformat/dvdvideodec.c
>>> +++ b/libavformat/dvdvideodec.c
>>> @@ -1202,7 +1202,6 @@ static void
>>> dvdvideo_subdemux_close(AVFormatContext *s)
>>>       DVDVideoDemuxContext *c = s->priv_data;
>>>         av_freep(&c->mpeg_pb.pub.buffer);
>>> -    av_freep(&c->mpeg_pb);
>>>       avformat_close_input(&c->mpeg_ctx);
>>>   }
>>>   
>>
Marth64 March 3, 2024, 2:31 a.m. UTC | #4 
FYI: hls, dashdec do the same av_freep(). Doesn't make this right, but
saying that was where I used as an example (so they should probably fixed
too?)

On Sat, Mar 2, 2024 at 10:17 AM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> James Almer:
> > On 3/2/2024 12:47 PM, Andreas Rheinhardt wrote:
> >> Andreas Rheinhardt:
> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >>> ---
> >>> How has this slipped through?
> >>
> >> Answer: AVIOContext starts with an AVClass* that is unset when using
> >> ffio_init_context(). Therefore the av_freep() leads to freeing of a NULL
> >> pointer which does not segfault.
> >
> > Would setting s->av_class to &ff_avio_options in ffio_init_context()
> > prevent this from happening again?
> >
>
> ff_avio_options is not an AVClass, so this is wrong; setting it to
> ff_avio_class would also be wrong, because said class is only to be used
> with the URLProtocol-based AVIOContexts (the child_next and
> child_class_iterate callbacks are designed for this; the former returns
> the AVIOContext's opaque, believing it to point to an URLContext, yet in
> general it need not point to an AVClass-enabled struct at all).
>
> In fact, ff_avio_options etc. should be moved to avio.c. I'll look into
> this.
>
> >>
> >>> Btw: This patchset is based upon code inspection, not on reading
> >>> actual files.
> >>>
> >>>   libavformat/dvdvideodec.c | 1 -
> >>>   1 file changed, 1 deletion(-)
> >>>
> >>> diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
> >>> index ef2d4e6df4..f2f23affb2 100644
> >>> --- a/libavformat/dvdvideodec.c
> >>> +++ b/libavformat/dvdvideodec.c
> >>> @@ -1202,7 +1202,6 @@ static void
> >>> dvdvideo_subdemux_close(AVFormatContext *s)
> >>>       DVDVideoDemuxContext *c = s->priv_data;
> >>>         av_freep(&c->mpeg_pb.pub.buffer);
> >>> -    av_freep(&c->mpeg_pb);
> >>>       avformat_close_input(&c->mpeg_ctx);
> >>>   }
> >>>
> >>
>
> _______________________________________________
> 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".
>
Marth64 March 3, 2024, 2:32 a.m. UTC | #5 
Nevermind, disregard. They do not, and I misread. Thanks for catching this.

On Sat, Mar 2, 2024 at 8:31 PM Marth64 <marth64@proxyid.net> wrote:

> FYI: hls, dashdec do the same av_freep(). Doesn't make this right, but
> saying that was where I used as an example (so they should probably fixed
> too?)
>
> On Sat, Mar 2, 2024 at 10:17 AM Andreas Rheinhardt <
> andreas.rheinhardt@outlook.com> wrote:
>
>> James Almer:
>> > On 3/2/2024 12:47 PM, Andreas Rheinhardt wrote:
>> >> Andreas Rheinhardt:
>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> >>> ---
>> >>> How has this slipped through?
>> >>
>> >> Answer: AVIOContext starts with an AVClass* that is unset when using
>> >> ffio_init_context(). Therefore the av_freep() leads to freeing of a
>> NULL
>> >> pointer which does not segfault.
>> >
>> > Would setting s->av_class to &ff_avio_options in ffio_init_context()
>> > prevent this from happening again?
>> >
>>
>> ff_avio_options is not an AVClass, so this is wrong; setting it to
>> ff_avio_class would also be wrong, because said class is only to be used
>> with the URLProtocol-based AVIOContexts (the child_next and
>> child_class_iterate callbacks are designed for this; the former returns
>> the AVIOContext's opaque, believing it to point to an URLContext, yet in
>> general it need not point to an AVClass-enabled struct at all).
>>
>> In fact, ff_avio_options etc. should be moved to avio.c. I'll look into
>> this.
>>
>> >>
>> >>> Btw: This patchset is based upon code inspection, not on reading
>> >>> actual files.
>> >>>
>> >>>   libavformat/dvdvideodec.c | 1 -
>> >>>   1 file changed, 1 deletion(-)
>> >>>
>> >>> diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
>> >>> index ef2d4e6df4..f2f23affb2 100644
>> >>> --- a/libavformat/dvdvideodec.c
>> >>> +++ b/libavformat/dvdvideodec.c
>> >>> @@ -1202,7 +1202,6 @@ static void
>> >>> dvdvideo_subdemux_close(AVFormatContext *s)
>> >>>       DVDVideoDemuxContext *c = s->priv_data;
>> >>>         av_freep(&c->mpeg_pb.pub.buffer);
>> >>> -    av_freep(&c->mpeg_pb);
>> >>>       avformat_close_input(&c->mpeg_ctx);
>> >>>   }
>> >>>
>> >>
>>
>> _______________________________________________
>> 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".
>>
>
diff mbox series

Patch

diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
index ef2d4e6df4..f2f23affb2 100644
--- a/libavformat/dvdvideodec.c
+++ b/libavformat/dvdvideodec.c
@@ -1202,7 +1202,6 @@  static void dvdvideo_subdemux_close(AVFormatContext *s)
     DVDVideoDemuxContext *c = s->priv_data;
 
     av_freep(&c->mpeg_pb.pub.buffer);
-    av_freep(&c->mpeg_pb);
     avformat_close_input(&c->mpeg_ctx);
 }