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

[Michael Niedermayer]

On Sat, Apr 11, 2009 at 03:12:30PM +0200, Reimar D?ffinger wrote:
> 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

no, it does not always free it

but the principle is ok

[...]

> 
> > 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.

the problem i have with it is that you add the zeroing code inside the
individual codecs while i doubt these codecs benefit from it

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090411/b6efe3cb/attachment.pgp>