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

[Michael Niedermayer]

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;
    }


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

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110105/72fecde7/attachment.pgp>