[FFmpeg-devel] [PATCH] ensure input buffer padding is always initialized to 0
Sat Apr 11 16:06:07 CEST 2009
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
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
Size: 189 bytes
Desc: Digital signature
More information about the ffmpeg-devel