[FFmpeg-devel] [PATCH] Remove flexible array member from Escape 124

Reimar Döffinger Reimar.Doeffinger
Mon Mar 31 20:17:43 CEST 2008


On Mon, Mar 31, 2008 at 11:07:10AM -0700, Eli Friedman wrote:
> Per the thread about my Escape 124 patch ("[FFmpeg-devel] [PATCH]
> Escape 124 (RPL) decoder rev5"), patch to avoid using a flexible array
> member, since apparently it causes issues with older compilers.

It seems mostly cleaner to me, but...

> @@ -83,23 +83,23 @@
>      return 0;
>  }
>  
> -static CodeBook* unpack_codebook(GetBitContext* gb, unsigned depth,
> +static CodeBook unpack_codebook(GetBitContext* gb, unsigned depth,
>                                   unsigned size)

I think it would be better if the function took a CodeBook* argument
instead of having a return value.

> -    if (size >= (INT_MAX - sizeof(CodeBook)) / sizeof(MacroBlock))
> -        return NULL;
> -    cb = av_malloc(size * sizeof(MacroBlock) + sizeof(CodeBook));
> -    if (!cb)
> -        return NULL;
> +    if (size >= INT_MAX / sizeof(MacroBlock))
> +        return cb;
> +    cb.blocks = av_malloc(size ? size * sizeof(MacroBlock) : 1);

Is there any reason why calling av_malloc with 0 should be a problem?
Ah, seems you will have to do the change I suggested before to make this
work...
And too bad that there is no av_calloc, having that kind of overflow
check maintained at some central place would be nicer...

> @@ -265,9 +265,9 @@
>                      cb_size = s->num_superblocks << cb_depth;
>                  }
>              }
> -            av_free(s->codebooks[i]);
> +            av_free(s->codebooks[i].blocks);
>              s->codebooks[i] = unpack_codebook(&gb, cb_depth, cb_size);
> -            if (!s->codebooks[i])
> +            if (!s->codebooks[i].blocks)
>                  return -1;

Having CodeBook* as argument to unpack_codebook also has the advantage
to allow checking the return value and you can do the av_free inside
unpack_codebook as well, following the principle of doing the free as
much as possible at the same place as the alloc.

Greetings,
Reimar D?ffinger




More information about the ffmpeg-devel mailing list