diff mbox

[FFmpeg-devel,v1,5/6] avformat/rl2: use av_freep instead of av_free to avoid invalid access if alloc failed

Message ID 20191011061444.4988-5-lance.lmwang@gmail.com
State New
Headers show

Commit Message

Lance Wang Oct. 11, 2019, 6:14 a.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/rl2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Andreas Rheinhardt Oct. 11, 2019, 6:23 a.m. UTC | #1 
lance.lmwang@gmail.com:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavformat/rl2.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/rl2.c b/libavformat/rl2.c
> index d847d9aaa8..3d38ffe8ba 100644
> --- a/libavformat/rl2.c
> +++ b/libavformat/rl2.c
> @@ -163,9 +163,9 @@ static av_cold int rl2_read_header(AVFormatContext *s)
>      chunk_offset = av_malloc(frame_count * sizeof(uint32_t));
>  
>      if(!chunk_size || !audio_size || !chunk_offset){
> -        av_free(chunk_size);
> -        av_free(audio_size);
> -        av_free(chunk_offset);
> +        av_freep(&chunk_size);
> +        av_freep(&audio_size);
> +        av_freep(&chunk_offset);
>          return AVERROR(ENOMEM);
>      }
>  
What invalid accesses are you talking about? You are just setting
local variables to NULL (in addition to freeing them) and you do this
immediately before leaving the function which ends their lifetime
anyway. So I don't really know how this should help prevent invalid
accesses.

- Andreas
Lance Wang Oct. 11, 2019, 6:33 a.m. UTC | #2 
On Fri, Oct 11, 2019 at 06:23:00AM +0000, Andreas Rheinhardt wrote:
> lance.lmwang@gmail.com:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavformat/rl2.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavformat/rl2.c b/libavformat/rl2.c
> > index d847d9aaa8..3d38ffe8ba 100644
> > --- a/libavformat/rl2.c
> > +++ b/libavformat/rl2.c
> > @@ -163,9 +163,9 @@ static av_cold int rl2_read_header(AVFormatContext *s)
> >      chunk_offset = av_malloc(frame_count * sizeof(uint32_t));
> >  
> >      if(!chunk_size || !audio_size || !chunk_offset){
> > -        av_free(chunk_size);
> > -        av_free(audio_size);
> > -        av_free(chunk_offset);
> > +        av_freep(&chunk_size);
> > +        av_freep(&audio_size);
> > +        av_freep(&chunk_offset);
> >          return AVERROR(ENOMEM);
> >      }
> >  
> What invalid accesses are you talking about? You are just setting
> local variables to NULL (in addition to freeing them) and you do this
> immediately before leaving the function which ends their lifetime
> anyway. So I don't really know how this should help prevent invalid
> accesses.

If one of chunk_size or audio_size or chunk_offset is NULL, it'll cause
av_free(NULL) which it's invalid access.

> 
> - 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".
Andreas Rheinhardt Oct. 11, 2019, 6:42 a.m. UTC | #3 
Limin Wang:
> On Fri, Oct 11, 2019 at 06:23:00AM +0000, Andreas Rheinhardt wrote:
>> lance.lmwang@gmail.com:
>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>
>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>>> ---
>>>  libavformat/rl2.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libavformat/rl2.c b/libavformat/rl2.c
>>> index d847d9aaa8..3d38ffe8ba 100644
>>> --- a/libavformat/rl2.c
>>> +++ b/libavformat/rl2.c
>>> @@ -163,9 +163,9 @@ static av_cold int rl2_read_header(AVFormatContext *s)
>>>      chunk_offset = av_malloc(frame_count * sizeof(uint32_t));
>>>  
>>>      if(!chunk_size || !audio_size || !chunk_offset){
>>> -        av_free(chunk_size);
>>> -        av_free(audio_size);
>>> -        av_free(chunk_offset);
>>> +        av_freep(&chunk_size);
>>> +        av_freep(&audio_size);
>>> +        av_freep(&chunk_offset);
>>>          return AVERROR(ENOMEM);
>>>      }
>>>  
>> What invalid accesses are you talking about? You are just setting
>> local variables to NULL (in addition to freeing them) and you do this
>> immediately before leaving the function which ends their lifetime
>> anyway. So I don't really know how this should help prevent invalid
>> accesses.
> 
> If one of chunk_size or audio_size or chunk_offset is NULL, it'll cause
> av_free(NULL) which it's invalid access.
> 
The part about invalid access is totally wrong. From the documentation
of av_free: "ptr = NULL is explicitly allowed." It's the same with the
ordinary free() function of the C standard library ("If ptr is a null
pointer, no action occurs.").
Moreover, if av_free(ptr) were dangerous if ptr == NULL, then so is
av_freep(&ptr) -- because av_freep will call av_free(ptr) internally
(without any check whether ptr is NULL).

- Andreas
Lance Wang Oct. 11, 2019, 7:01 a.m. UTC | #4 
On Fri, Oct 11, 2019 at 06:42:00AM +0000, Andreas Rheinhardt wrote:
> Limin Wang:
> > On Fri, Oct 11, 2019 at 06:23:00AM +0000, Andreas Rheinhardt wrote:
> >> lance.lmwang@gmail.com:
> >>> From: Limin Wang <lance.lmwang@gmail.com>
> >>>
> >>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>> ---
> >>>  libavformat/rl2.c | 6 +++---
> >>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/libavformat/rl2.c b/libavformat/rl2.c
> >>> index d847d9aaa8..3d38ffe8ba 100644
> >>> --- a/libavformat/rl2.c
> >>> +++ b/libavformat/rl2.c
> >>> @@ -163,9 +163,9 @@ static av_cold int rl2_read_header(AVFormatContext *s)
> >>>      chunk_offset = av_malloc(frame_count * sizeof(uint32_t));
> >>>  
> >>>      if(!chunk_size || !audio_size || !chunk_offset){
> >>> -        av_free(chunk_size);
> >>> -        av_free(audio_size);
> >>> -        av_free(chunk_offset);
> >>> +        av_freep(&chunk_size);
> >>> +        av_freep(&audio_size);
> >>> +        av_freep(&chunk_offset);
> >>>          return AVERROR(ENOMEM);
> >>>      }
> >>>  
> >> What invalid accesses are you talking about? You are just setting
> >> local variables to NULL (in addition to freeing them) and you do this
> >> immediately before leaving the function which ends their lifetime
> >> anyway. So I don't really know how this should help prevent invalid
> >> accesses.
> > 
> > If one of chunk_size or audio_size or chunk_offset is NULL, it'll cause
> > av_free(NULL) which it's invalid access.
> > 
> The part about invalid access is totally wrong. From the documentation
> of av_free: "ptr = NULL is explicitly allowed." It's the same with the
> ordinary free() function of the C standard library ("If ptr is a null
> pointer, no action occurs.").
> Moreover, if av_free(ptr) were dangerous if ptr == NULL, then so is
> av_freep(&ptr) -- because av_freep will call av_free(ptr) internally
> (without any check whether ptr is NULL).

Sorry, it's true, I was mislead by the code which it check the pointer before
av_free. Please ignore the patch.


> 
> - 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".
diff mbox

Patch

diff --git a/libavformat/rl2.c b/libavformat/rl2.c
index d847d9aaa8..3d38ffe8ba 100644
--- a/libavformat/rl2.c
+++ b/libavformat/rl2.c
@@ -163,9 +163,9 @@  static av_cold int rl2_read_header(AVFormatContext *s)
     chunk_offset = av_malloc(frame_count * sizeof(uint32_t));
 
     if(!chunk_size || !audio_size || !chunk_offset){
-        av_free(chunk_size);
-        av_free(audio_size);
-        av_free(chunk_offset);
+        av_freep(&chunk_size);
+        av_freep(&audio_size);
+        av_freep(&chunk_offset);
         return AVERROR(ENOMEM);
     }