diff mbox series

[FFmpeg-devel,1/2] avdevice/avfoundation: fix memleak

Message ID tencent_97F4E1DC5AB2355F811C34993E2F56DD9D09@qq.com
State Accepted
Commit 6b708592fa8c80a8c65b063b25bb496920d1eda0
Headers show
Series [FFmpeg-devel,1/2] avdevice/avfoundation: fix memleak | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Zhao Zhili Feb. 4, 2022, 4:19 p.m. UTC 
---
 libavdevice/avfoundation.m | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Thilo Borgmann Feb. 8, 2022, 12:47 p.m. UTC | #1 
Am 04.02.22 um 17:19 schrieb Zhao Zhili:
> ---
>   libavdevice/avfoundation.m | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
> index 0cd6e646d5..2078c4879c 100644
> --- a/libavdevice/avfoundation.m
> +++ b/libavdevice/avfoundation.m
> @@ -106,6 +106,7 @@ typedef struct
>       int             audio_device_index;
>       int             audio_stream_index;
>   
> +    char            *url;
>       char            *video_filename;
>       char            *audio_filename;
>   
> @@ -299,6 +300,7 @@ static void destroy_context(AVFContext* ctx)
>       ctx->avf_delegate    = NULL;
>       ctx->avf_audio_delegate = NULL;
>   
> +    av_freep(&ctx->url);
>       av_freep(&ctx->audio_buffer);

Why carry it in the context instead of adding the missing av_freep() in parse_device_name() ?

-Thilo
Andreas Rheinhardt Feb. 8, 2022, 12:50 p.m. UTC | #2 
Thilo Borgmann:
> Am 04.02.22 um 17:19 schrieb Zhao Zhili:
>> ---
>>   libavdevice/avfoundation.m | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
>> index 0cd6e646d5..2078c4879c 100644
>> --- a/libavdevice/avfoundation.m
>> +++ b/libavdevice/avfoundation.m
>> @@ -106,6 +106,7 @@ typedef struct
>>       int             audio_device_index;
>>       int             audio_stream_index;
>>   +    char            *url;
>>       char            *video_filename;
>>       char            *audio_filename;
>>   @@ -299,6 +300,7 @@ static void destroy_context(AVFContext* ctx)
>>       ctx->avf_delegate    = NULL;
>>       ctx->avf_audio_delegate = NULL;
>>   +    av_freep(&ctx->url);
>>       av_freep(&ctx->audio_buffer);
> 
> Why carry it in the context instead of adding the missing av_freep() in
> parse_device_name() ?
> 

Because video_filename and audio_filename point into it.

- Andreas
Thilo Borgmann Feb. 8, 2022, 1:03 p.m. UTC | #3 
Am 08.02.22 um 13:50 schrieb Andreas Rheinhardt:
> Thilo Borgmann:
>> Am 04.02.22 um 17:19 schrieb Zhao Zhili:
>>> ---
>>>    libavdevice/avfoundation.m | 10 ++++++----
>>>    1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
>>> index 0cd6e646d5..2078c4879c 100644
>>> --- a/libavdevice/avfoundation.m
>>> +++ b/libavdevice/avfoundation.m
>>> @@ -106,6 +106,7 @@ typedef struct
>>>        int             audio_device_index;
>>>        int             audio_stream_index;
>>>    +    char            *url;
>>>        char            *video_filename;
>>>        char            *audio_filename;
>>>    @@ -299,6 +300,7 @@ static void destroy_context(AVFContext* ctx)
>>>        ctx->avf_delegate    = NULL;
>>>        ctx->avf_audio_delegate = NULL;
>>>    +    av_freep(&ctx->url);
>>>        av_freep(&ctx->audio_buffer);
>>
>> Why carry it in the context instead of adding the missing av_freep() in
>> parse_device_name() ?
>>
> 
> Because video_filename and audio_filename point into it.

Wondering if we couldn't skip av_strdup() and operate on s->url directly then.

-Thilo
Andreas Rheinhardt Feb. 8, 2022, 1:05 p.m. UTC | #4 
Thilo Borgmann:
> Am 08.02.22 um 13:50 schrieb Andreas Rheinhardt:
>> Thilo Borgmann:
>>> Am 04.02.22 um 17:19 schrieb Zhao Zhili:
>>>> ---
>>>>    libavdevice/avfoundation.m | 10 ++++++----
>>>>    1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
>>>> index 0cd6e646d5..2078c4879c 100644
>>>> --- a/libavdevice/avfoundation.m
>>>> +++ b/libavdevice/avfoundation.m
>>>> @@ -106,6 +106,7 @@ typedef struct
>>>>        int             audio_device_index;
>>>>        int             audio_stream_index;
>>>>    +    char            *url;
>>>>        char            *video_filename;
>>>>        char            *audio_filename;
>>>>    @@ -299,6 +300,7 @@ static void destroy_context(AVFContext* ctx)
>>>>        ctx->avf_delegate    = NULL;
>>>>        ctx->avf_audio_delegate = NULL;
>>>>    +    av_freep(&ctx->url);
>>>>        av_freep(&ctx->audio_buffer);
>>>
>>> Why carry it in the context instead of adding the missing av_freep() in
>>> parse_device_name() ?
>>>
>>
>> Because video_filename and audio_filename point into it.
> 
> Wondering if we couldn't skip av_strdup() and operate on s->url directly
> then.
> 

This would trash s->url.

- Andreas
Zhao Zhili Feb. 19, 2022, 8:45 a.m. UTC | #5 
> On Feb 8, 2022, at 9:05 PM, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 
> Thilo Borgmann:
>> Am 08.02.22 um 13:50 schrieb Andreas Rheinhardt:
>>> Thilo Borgmann:
>>>> Am 04.02.22 um 17:19 schrieb Zhao Zhili:
>>>>> ---
>>>>>    libavdevice/avfoundation.m | 10 ++++++----
>>>>>    1 file changed, 6 insertions(+), 4 deletions(-)
>>>>> 
>>>>> diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
>>>>> index 0cd6e646d5..2078c4879c 100644
>>>>> --- a/libavdevice/avfoundation.m
>>>>> +++ b/libavdevice/avfoundation.m
>>>>> @@ -106,6 +106,7 @@ typedef struct
>>>>>        int             audio_device_index;
>>>>>        int             audio_stream_index;
>>>>>    +    char            *url;
>>>>>        char            *video_filename;
>>>>>        char            *audio_filename;
>>>>>    @@ -299,6 +300,7 @@ static void destroy_context(AVFContext* ctx)
>>>>>        ctx->avf_delegate    = NULL;
>>>>>        ctx->avf_audio_delegate = NULL;
>>>>>    +    av_freep(&ctx->url);
>>>>>        av_freep(&ctx->audio_buffer);
>>>> 
>>>> Why carry it in the context instead of adding the missing av_freep() in
>>>> parse_device_name() ?
>>>> 
>>> 
>>> Because video_filename and audio_filename point into it.
>> 
>> Wondering if we couldn't skip av_strdup() and operate on s->url directly
>> then.
>> 
> 
> This would trash s->url.

Ping. Can this patch set be applied?

> 
> - Andreas
> _______________________________________________
> 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".
Thilo Borgmann Feb. 22, 2022, 12:12 p.m. UTC | #6 
Am 19.02.22 um 09:45 schrieb "zhilizhao(赵志立)":
> 
> 
>> On Feb 8, 2022, at 9:05 PM, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
>>
>> Thilo Borgmann:
>>> Am 08.02.22 um 13:50 schrieb Andreas Rheinhardt:
>>>> Thilo Borgmann:
>>>>> Am 04.02.22 um 17:19 schrieb Zhao Zhili:
>>>>>> ---
>>>>>>     libavdevice/avfoundation.m | 10 ++++++----
>>>>>>     1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
>>>>>> index 0cd6e646d5..2078c4879c 100644
>>>>>> --- a/libavdevice/avfoundation.m
>>>>>> +++ b/libavdevice/avfoundation.m
>>>>>> @@ -106,6 +106,7 @@ typedef struct
>>>>>>         int             audio_device_index;
>>>>>>         int             audio_stream_index;
>>>>>>     +    char            *url;
>>>>>>         char            *video_filename;
>>>>>>         char            *audio_filename;
>>>>>>     @@ -299,6 +300,7 @@ static void destroy_context(AVFContext* ctx)
>>>>>>         ctx->avf_delegate    = NULL;
>>>>>>         ctx->avf_audio_delegate = NULL;
>>>>>>     +    av_freep(&ctx->url);
>>>>>>         av_freep(&ctx->audio_buffer);
>>>>>
>>>>> Why carry it in the context instead of adding the missing av_freep() in
>>>>> parse_device_name() ?
>>>>>
>>>>
>>>> Because video_filename and audio_filename point into it.
>>>
>>> Wondering if we couldn't skip av_strdup() and operate on s->url directly
>>> then.
>>>
>>
>> This would trash s->url.
> 
> Ping. Can this patch set be applied?

Pushed, thanks!

-Thilo
diff mbox series

Patch

diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
index 0cd6e646d5..2078c4879c 100644
--- a/libavdevice/avfoundation.m
+++ b/libavdevice/avfoundation.m
@@ -106,6 +106,7 @@  typedef struct
     int             audio_device_index;
     int             audio_stream_index;
 
+    char            *url;
     char            *video_filename;
     char            *audio_filename;
 
@@ -299,6 +300,7 @@  static void destroy_context(AVFContext* ctx)
     ctx->avf_delegate    = NULL;
     ctx->avf_audio_delegate = NULL;
 
+    av_freep(&ctx->url);
     av_freep(&ctx->audio_buffer);
 
     pthread_mutex_destroy(&ctx->frame_lock);
@@ -311,14 +313,14 @@  static void destroy_context(AVFContext* ctx)
 static void parse_device_name(AVFormatContext *s)
 {
     AVFContext *ctx = (AVFContext*)s->priv_data;
-    char *tmp = av_strdup(s->url);
+    ctx->url = av_strdup(s->url);
     char *save;
 
-    if (tmp[0] != ':') {
-        ctx->video_filename = av_strtok(tmp,  ":", &save);
+    if (ctx->url[0] != ':') {
+        ctx->video_filename = av_strtok(ctx->url,  ":", &save);
         ctx->audio_filename = av_strtok(NULL, ":", &save);
     } else {
-        ctx->audio_filename = av_strtok(tmp,  ":", &save);
+        ctx->audio_filename = av_strtok(ctx->url,  ":", &save);
     }
 }