[FFmpeg-devel] [PATCH v3] add put_bits_long to avoid undefined behaviour in put_bits

[Reimar Döffinger]

On Mon, Sep 14, 2009 at 09:03:53PM +0100, M?ns Rullg?rd wrote:
> matthieu castet <castet.matthieu at free.fr> writes:
> > - should we create a put_bits.c for putting only this function ?
> 
> bitstream.c seems fine to me.

That doesn't really work well, because the function is different
depending on whether a LE writer is used or not, so at least a part
must always be in the header.
I suggest attached patch, though I think that
1) mjpegdec should just be changed to write 16 bits per pass (it is just padding),
or actually it should just do flush_put_bits first (pads with 0 up to next byte) and
then use memset.
2) vorbis_enc should be using bytestream functions for most of the stuff
3) mpegenc should possibly be using bytestream, but it IMO certainly should
be using "put_bits_count >> 3" instead of "put_bits_ptr(&pb) - pb.buf"
-------------- next part --------------
Index: libavcodec/mjpegenc.c
===================================================================
--- libavcodec/mjpegenc.c	(revision 20091)
+++ libavcodec/mjpegenc.c	(working copy)
@@ -315,7 +315,7 @@
 
     /* skip put bits */
     for(i=0; i<ff_count-3; i+=4)
-        put_bits(&s->pb, 32, 0);
+        put_bits_long(&s->pb, 32, 0);
     put_bits(&s->pb, (ff_count-i)*8, 0);
     flush_put_bits(&s->pb);
 
Index: libavcodec/put_bits.h
===================================================================
--- libavcodec/put_bits.h	(revision 20091)
+++ libavcodec/put_bits.h	(working copy)
@@ -143,7 +143,7 @@
     int bit_left;
 
     //    printf("put_bits=%d %x\n", n, value);
-    assert(n == 32 || value < (1U << n));
+    assert(n <= 31 && value < (1U << n));
 
     bit_buf = s->bit_buf;
     bit_left = s->bit_left;
@@ -260,6 +260,27 @@
 }
 
 /**
+ * Write up to 62 bits into a bitstream
+ */
+static void put_bits_long(PutBitContext *s, int n, uint64_t value)
+{
+    if(n <= 31) {
+        put_bits(s, n, value);
+    } else {
+        int lo = value & 0x7fffffff;
+        int hi = value >> 31;
+#ifdef ALT_BITSTREAM_WRITER_LE
+        put_bits(s,     31, lo);
+        put_bits(s, n - 31, hi);
+#else
+        put_bits(s, n - 31, hi);
+        put_bits(s,     31, lo);
+#endif
+    }
+
+}
+
+/**
  * Returns the pointer to the byte where the bitstream writer will put
  * the next bit.
  */
Index: libavcodec/alacenc.c
===================================================================
--- libavcodec/alacenc.c	(revision 20091)
+++ libavcodec/alacenc.c	(working copy)
@@ -123,7 +123,7 @@
     put_bits(&s->pbctx, 1,  1);                             // Sample count is in the header
     put_bits(&s->pbctx, 2,  0);                             // FIXME: Wasted bytes field
     put_bits(&s->pbctx, 1,  is_verbatim);                   // Audio block is verbatim
-    put_bits(&s->pbctx, 32, s->avctx->frame_size);          // No. of samples in the frame
+    put_bits_long(&s->pbctx, 32, s->avctx->frame_size);     // No. of samples in the frame
 }
 
 static void calc_predictor_params(AlacEncodeContext *s, int ch)
Index: libavcodec/vorbis_enc.c
===================================================================
--- libavcodec/vorbis_enc.c	(revision 20091)
+++ libavcodec/vorbis_enc.c	(working copy)
@@ -386,7 +386,7 @@
         mant = -mant;
     }
     res |= mant | (exp << 21);
-    put_bits(pb, 32, res);
+    put_bits_long(pb, 32, res);
 }
 
 static void put_codebook_header(PutBitContext *pb, vorbis_enc_codebook *cb)
@@ -532,12 +532,12 @@
     put_bits(&pb, 8, 1); //magic
     for (i = 0; "vorbis"[i]; i++)
         put_bits(&pb, 8, "vorbis"[i]);
-    put_bits(&pb, 32, 0); // version
+    put_bits_long(&pb, 32, 0); // version
     put_bits(&pb,  8, venc->channels);
-    put_bits(&pb, 32, venc->sample_rate);
-    put_bits(&pb, 32, 0); // bitrate
-    put_bits(&pb, 32, 0); // bitrate
-    put_bits(&pb, 32, 0); // bitrate
+    put_bits_long(&pb, 32, venc->sample_rate);
+    put_bits_long(&pb, 32, 0); // bitrate
+    put_bits_long(&pb, 32, 0); // bitrate
+    put_bits_long(&pb, 32, 0); // bitrate
     put_bits(&pb,  4, venc->log2_blocksize[0]);
     put_bits(&pb,  4, venc->log2_blocksize[1]);
     put_bits(&pb,  1, 1); // framing
@@ -552,8 +552,8 @@
     put_bits(&pb, 8, 3); //magic
     for (i = 0; "vorbis"[i]; i++)
         put_bits(&pb, 8, "vorbis"[i]);
-    put_bits(&pb, 32, 0); // vendor length TODO
-    put_bits(&pb, 32, 0); // amount of comments
+    put_bits_long(&pb, 32, 0); // vendor length TODO
+    put_bits_long(&pb, 32, 0); // amount of comments
     put_bits(&pb,  1, 1); // framing
 
     flush_put_bits(&pb);
Index: libavformat/mpegenc.c
===================================================================
--- libavformat/mpegenc.c	(revision 20091)
+++ libavformat/mpegenc.c	(working copy)
@@ -89,7 +89,7 @@
 
     init_put_bits(&pb, buf, 128);
 
-    put_bits(&pb, 32, PACK_START_CODE);
+    put_bits_long(&pb, 32, PACK_START_CODE);
     if (s->is_mpeg2) {
         put_bits(&pb, 2, 0x1);
     } else {
@@ -125,7 +125,7 @@
 
     init_put_bits(&pb, buf, 128);
 
-    put_bits(&pb, 32, SYSTEM_HEADER_START_CODE);
+    put_bits_long(&pb, 32, SYSTEM_HEADER_START_CODE);
     put_bits(&pb, 16, 0);
     put_bits(&pb, 1, 1);