[FFmpeg-devel] [PATCH] fix for malloc error (roundup issues 2480, 2479)

[Kostya Shishkov]

On 5 January 2011 17:22, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Mon, Jan 03, 2011 at 01:01:17PM -0500, Daniel Kang wrote:
>> I am a Google Code-In student, and as part of a task, I have zzufed
>> several files. It seems for extraordinary large total frames, ffmpeg
>> fails on mallocating memory. Both bugs have been reproduced, so it is
>> not only a bug on my box.
>>
>> The patches attached fix the issues locally. Another solution is to move
>> the check into av_malloc, but that may create extra overhead. I am not
>> sure what the best solution may be.
>>
>> Are there any suggestions?
>
>> ?ape.c | ? ?2 +-
>> ?1 file changed, 1 insertion(+), 1 deletion(-)
>> f806ab16204e4200c3b29ce617532b8e4250eb11 ?ape_malloc_check.diff
>> From 035683a38496569c9b79e2421682607c678a0a8b Mon Sep 17 00:00:00 2001
>> From: Daniel Kang <daniel.d.kang at gmail.com>
>> Date: Sun, 2 Jan 2011 20:42:07 -0500
>> Subject: [PATCH] Sanity check to see if malloc returns the right size.
>>
>> ---
>> ?libavformat/ape.c | ? ?2 +-
>> ?1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/libavformat/ape.c b/libavformat/ape.c
>> index 91acf72..225f00b 100644
>> --- a/libavformat/ape.c
>> +++ b/libavformat/ape.c
>> @@ -247,7 +247,7 @@ static int ape_read_header(AVFormatContext * s, AVFormatParameters * ap)
>> ? ? ? ? ?return -1;
>> ? ? ?}
>> ? ? ?ape->frames ? ? ? = av_malloc(ape->totalframes * sizeof(APEFrame));
>> - ? ?if(!ape->frames)
>> + ? ?if(!ape->frames || sizeof(ape->frames) != ape->totalframes * sizeof(APEFrame))
>> ? ? ? ? ?return AVERROR(ENOMEM);
>
> sizeof(ape->frames) is the size of the type of ape->frames, if thats a pointer
> it would be sizeof(*void) aka 4 or 8 bytes normally.
> if its an array like int frames[123] then it would be the size in bytes of
> that array thus 4*128 normally in that case.
>
> *malloc() dont allocate less then requested.
> what can happen with code like av_malloc(ape->totalframes * sizeof(APEFrame));
> is that the multiply overflows, like 0x7FFFFFFF * 10 doesnt fit in 32bit int
> but it seems this is alraedy checked for in the line above:
>
> ? ?if(ape->totalframes > UINT_MAX / sizeof(APEFrame)){
> ? ? ? ?av_log(s, AV_LOG_ERROR, "Too many frames: %d\n", ape->totalframes);
> ? ? ? ?return -1;
> ? ?}

There's also a check on seektable/num frames mismatch but it will only
print warning.
I'll look at it tomorrow.

> [...]
> --
> Michael ? ? GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB