[FFmpeg-devel] [PATCH] ensure input buffer padding is always initialized to 0

[Reimar Döffinger]

On Sat, Apr 11, 2009 at 02:48:41PM +0200, Michael Niedermayer wrote:
> On Sat, Apr 11, 2009 at 12:29:17PM +0200, Reimar D?ffinger wrote:
> > Hello,
> > there are quite a few valgrind errors in all kinds of codecs because the
> > padding is not initialized to 0 as required.
> > Attached patch changes this. I have not checked if any of the code is
> > speed-critical enough to justify a more complicated method of doing
> > this, though in those cases av_fast_realloc should not have been used
> > since it involves a memcpy which AFAICT is completely useless in all
> > these cases (the previous data is not relevant).
> 
> > Index: libavcodec/motionpixels.c
> > ===================================================================
> > --- libavcodec/motionpixels.c	(revision 18427)
> > +++ libavcodec/motionpixels.c	(working copy)
> > @@ -298,6 +298,9 @@
> >  
> >      /* le32 bitstream msb first */
> >      mp->bswapbuf = av_fast_realloc(mp->bswapbuf, &mp->bswapbuf_size, buf_size + FF_INPUT_BUFFER_PADDING_SIZE);
> > +    if (!mp->bswapbuf)
> > +        return AVERROR(ENOMEM);
> 
> memleak
> a av_realloc_free() that frees the old buffer on failure might be a 
> good idea ...

Actually, how about a
/**
 * allocates a buffer, reusing the given one if large enough
 *
 * does not preserve the buffer contents and always frees the buffer passed in
 */
void av_fast_malloc(void **ptr, unsigned *size, int min_size)
{
    if (min_size < *size)
        return;
    *size= FFMAX(17*min_size/16 + 32, min_size);
    av_free(*ptr);
    *ptr = av_malloc(*size);
    if (!*ptr) *size = 0;
}

> also, even though mpeg*/h26* can catch the end by
> FF_INPUT_BUFFER_PADDING_SIZE zeros, i doubt somewhat that it works
> with many other decoders ...
> in that sense i think meny of your added memsets() are misleaing
> and in that sense iam not in favor of them, valgrind is buggy if it
> complains about unini stuff that is thrown away

If you want a case-by-case analyses before agreeing to any of them I do
not object, though I'd appreciate if the maintainers took that as a hint
to verify their code if it appears in my patch.
Apart from that, valgrind is not the _reason_ for this, the reason is
that to my knowledge the (external, admittedly this often only uses
the internal) API requires the padding to be zeroed.
Also for all decoders requiring a fixed value for the padding reduces
the possibilities and should allow for better/easier/faster checks.