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

[Andreas Rheinhardt]

Limin Wang:
> On Fri, Oct 11, 2019 at 06:23:00AM +0000, Andreas Rheinhardt wrote:
>> lance.lmwang at gmail.com:
>>> From: Limin Wang <lance.lmwang at gmail.com>
>>>
>>> Signed-off-by: Limin Wang <lance.lmwang at 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