[FFmpeg-soc] [soc]: r5744 - mms/mmst.c

[Ronald S. Bultje]

Hi Bobby,

On Apr 8, 2010, at 8:52 PM, Bobby Bingham <uhmmmm at gmail.com> wrote:
> On Thu, 8 Apr 2010 12:06:14 -0400
> "Ronald S. Bultje" <rsbultje at gmail.com> wrote:
>>
>> On Thu, Apr 8, 2010 at 12:02 PM, spyfeng <subversion at mplayerhq.hu>  
>> wrote:
>>>                             mms->asf_header = av_realloc(mms- 
>>> >asf_header,
>>>                                               mms->asf_header_size
>>>                                               + mms->pkt_buf_len);
>>> -                            if (!mms->asf_header)
>>> +                            if (!mms->asf_header) {
>>> +                                av_freep(&mms->asf_header);
>>>                                 return AVERROR(ENOMEM);
>>> +                            }
>>>                             memcpy(mms->asf_header + mms- 
>>> >asf_header_size,
>>>                                                  mms->pkt_read_ptr,
>>>                                                  mms->pkt_buf_len);
>>
>> You now this won't work right? :-). You're assigning NULL to a  
>> pointer
>> and then free'ing it. You'll want to save the return value of
>> av_realloc() into a new pointer, check the value, free()
>> mms->asf_header if it failed, and else overwrite mms->asf_header.
>
> I haven't looked, but I can imagine that memory leaks arising from  
> this
> case aren't all that rare, and it seems a bit unwieldy to need a  
> second
> pointer to store the return value every time you call av_realloc.  I
> realize it follows the C realloc interface as is, but maybe a wrapper
> like this could be useful?
>
> int av_realloc2(void **ptr, size_t size)
> {
>    void *newptr = av_realloc(*ptr, size);
>    if(newptr || size == 0) {
>        *ptr = newptr;
>        return 0;
>    }
>    return AVERROR(ENOMEM);
> }

We generally don't like to change behaviour of libc wrappers, but see  
man reallocf also.

Ronald