[FFmpeg-devel] [PATCH 1/2] avformat/utils: free existing extradata before trying to allocate a new one

[James Almer]

On 3/6/2018 11:03 PM, James Almer wrote:
> On 3/6/2018 10:47 PM, Michael Niedermayer wrote:
>> On Tue, Mar 06, 2018 at 01:42:36AM -0300, James Almer wrote:
>>> This prevents leaks in the rare cases the function is called when extradata
>>> already exists.
>>>
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>>  libavformat/utils.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>>> index 72531d4185..31340a484b 100644
>>> --- a/libavformat/utils.c
>>> +++ b/libavformat/utils.c
>>> @@ -3245,6 +3245,7 @@ int ff_alloc_extradata(AVCodecParameters *par, int size)
>>>  {
>>>      int ret;
>>>  
>>> +    av_freep(&par->extradata);
>>>      if (size < 0 || size >= INT32_MAX - AV_INPUT_BUFFER_PADDING_SIZE) {
>>>          par->extradata = NULL;
>>>          par->extradata_size = 0;
>>
>> This causes memory corruption
>> ...
>> [mpegts @ 0x7f8c74000a80] PES packet size mismatch
>> *** Error in `./ffplay': double free or corruption (fasttop): 0x00007f8c7402d9c0 ***
>> Aborted (core dumped)
> 
> Is something freeing extradata and leaving a dangling pointer before
> eventually calling ff_alloc_extradata()?
> At least the two calls in mpegts.c don't seem to do that.
> 
>>
>> I think this should not have been applied so quickly, i tested it as soon as i
>> had time and saw it but it was applied already
> 
> Fate passed when i tested it, and i got a positive review from the
> author of the function in question, that's why i pushed it. Sorry.
> 
>>
>> If it helps i can debug the cases i see to find out which calls cause them but
>> someone will still have to review all call sites probably for this change to
>> be safe
> 
> Yes, help would be welcome. Crashes like this probably hint at frees
> leaving dangling pointers across the codebase.

Does the attached patch fix this crash?
-------------- next part --------------
From 1f128144cdd2f212b2ec74b29e2db771ad0977a6 Mon Sep 17 00:00:00 2001
From: James Almer <jamrial at gmail.com>
Date: Tue, 6 Mar 2018 23:26:22 -0300
Subject: [PATCH] avformat/isom: don't free extradata before calling
 ff_get_extradata()

Using av_free() leaves a dangling pointer.

Signed-off-by: James Almer <jamrial at gmail.com>
---
 libavformat/isom.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libavformat/isom.c b/libavformat/isom.c
index 59502a8b3f..2792371c25 100644
--- a/libavformat/isom.c
+++ b/libavformat/isom.c
@@ -526,7 +526,6 @@ FF_ENABLE_DEPRECATION_WARNINGS
         av_log(fc, AV_LOG_TRACE, "Specific MPEG-4 header len=%d\n", len);
         if (!len || (uint64_t)len > (1<<30))
             return AVERROR_INVALIDDATA;
-        av_free(st->codecpar->extradata);
         if ((ret = ff_get_extradata(fc, st->codecpar, pb, len)) < 0)
             return ret;
         if (st->codecpar->codec_id == AV_CODEC_ID_AAC) {
-- 
2.16.2