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

[Reimar Döffinger]

On Tue, Sep 29, 2009 at 11:24:53AM -0700, Baptiste Coudurier wrote:
> On 9/29/09 4:52 AM, Reimar D?ffinger wrote:
> > On Mon, Sep 21, 2009 at 02:40:51PM +0200, 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.
> >
> > Ok, a different version.
> > It uses s->sys->block_sizes even though I think it pointlessly bloats
> > and complexifies the code, it memsets to 0xff still because the
> > regression tests change anyway and the spec does not say anything about
> > which value to use, and it returns -1 if the bitstream wrote too far,
> > even though the return value is not checked anywhere.
> 
> Humm, yet it bloats the code indeed, sorry, commit this patch with the 
> old check against size_in_bits.

The bloat is partially due to other changes (like checking overread on
bit-level, which I just realize is pointless), it is basically only the %
s->sys->bpm it adds (though I find that very ugly).
Which variant do you want, this one for example?:

Index: libavcodec/dv.c
===================================================================
--- libavcodec/dv.c     (revision 20081)
+++ libavcodec/dv.c     (working copy)
@@ -1102,8 +1102,17 @@
             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;
+       int size = pbs[j].size_in_bits >> 3;
        flush_put_bits(&pbs[j]);
+       pos = put_bits_count(&pbs[j]) >> 3;
+       if (pos > size) {
+           av_log(NULL, AV_LOG_ERROR, "bitstream written beyond buffer size\n");
+           return -1;
+       }
+       memset(pbs[j].buf + pos, 0xff, size - pos);
+    }
 
     return 0;
 }