[FFmpeg-devel] [PATCH] Fix DV uninitialized reads

[Reimar Döffinger]

On Tue, Sep 22, 2009 at 12:19:29AM -0700, Baptiste Coudurier wrote:
> Hi Reimar,
> 
> On 09/21/2009 05:40 AM, Reimar D?ffinger wrote:
> > Hello,
> > I think this fixes the uninitialized data in the DV encoder that causes
> > sporadic "make test" failures, at least valgrind complains no longer.
> > Quick measurements with "time" indicate a slowdown by about 0.8%.
> > regression test values for the encoded files changes (memset to 0
> > instead of 0xff might avoid that though), but the decoded data
> > stays the same - so at least for the cases "make test" covers it is
> > correct.
> 
> I assume 0xff is correct according to specs, right ?

I don't think the specs say anything about it (except for the header
DIFs), but since the current code pads the header and audio DIFs by 0xff
it is consistent... (though flush_put_bits will pad with 0, so it is not
that consistent).

> > Index: libavcodec/dv.c
> > ===================================================================
> > --- libavcodec/dv.c     (revision 19948)
> > +++ libavcodec/dv.c     (working copy)
> > @@ -1102,8 +1102,14 @@
> >               av_log(NULL, AV_LOG_ERROR, "ac bitstream overflow\n");
> >       }
> >
> > -    for (j=0; j<5*s->sys->bpm; j++)
> > +    for (j=0; j<5*s->sys->bpm; j++) {
> > +       int pos, size;
> >          flush_put_bits(&pbs[j]);
> > +       pos = put_bits_count(&pbs[j])>>  3;
> > +       size = pbs[j].size_in_bits>>  3;
> 
> Maybe s->sys->block_sizes[j] makes more sense ?

Not in my opinion. Like this it is clear that it simply pads the
remainder of the put_bit context. If you use s->sys->block_sizes[j]
the code is only understandable to someone who knows that this put_bit
context is somehow related to the block sizes.

> > +       if (pos<  size)
> > +           memset(pbs[j].buf + pos, 0xff, size - pos);
> 
> Is it worth to check for pos < size ?

For speed? No. But if for some reason ever the decoder writes beyond the
buffer end without the check it would try to overwrite all memory with
0xff. If the cost is minimal (just a if) I prefer to make such
behaviour that makes a smaller security issue into a even larger impossible.