[FFmpeg-devel] [PATCH 1/2] avcodec/put_bits: Make skip_put_bits() less dangerous

[Andreas Rheinhardt]

Before c63c303a1f2b58677d480505ec93a90f77dd25b5 (the commit which
introduced a typedef for the type of the buffer of a PutBitContext)
skip_put_bits() was as follows:

static inline void skip_put_bits(PutBitContext *s, int n)
{
    s->bit_left -= n;
    s->buf_ptr  -= 4 * (s->bit_left >> 5);
    s->bit_left &= 31;
}

If s->bit_left was negative after the first subtraction, then the next
line will divide this by 32 with rounding towards -inf and multiply by
four; the result will be negative, of course.

The aforementioned commit changed this to:

static inline void skip_put_bits(PutBitContext *s, int n)
{
    s->bit_left -= n;
    s->buf_ptr  -= sizeof(BitBuf) * ((unsigned)s->bit_left / BUF_BITS);
    s->bit_left &= (BUF_BITS - 1);
}

Casting s->bit_left to unsigned meant that the rounding is still towards
-inf; yet the right side is now always positive (it transformed the
arithmetic shift into a logical shift), so that s->buf_ptr will always
be decremented (by about UINT_MAX / 8 unless n is huge) which leads to
segfaults on further usage and is already undefined pointer arithmetic
before that. This can be reproduced with the mpeg4 encoder with the
AV_CODEC_FLAG2_NO_OUTPUT flag set.

Furthermore, the earlier version as well as the new version share
another bug: s->bit_left will be in the range of 0..(BUF_BITS - 1)
afterwards, although the assumption throughout the other PutBitContext
functions is that it is in the range of 1..BUF_BITS. This might lead to
a shift by BUF_BITS in little-endian mode. This has been fixed, too.
The new version is furthermore able to skip zero bits, too.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
---
1. skip_put_bits() is still bad even after this: If one skips so few that
buf_ptr will not change, the non-skipped bits in the buffer will be
wrong in case of a big-endian reader (they would need to be shifted by
the number of bits to be skipped for this to work*). If one skips so much
that buf_ptr does change, all the valid bits in the buffer will not be
output at the place where they should have been output.

2. Since c63c303a1f2b58677d480505ec93a90f77dd25b5 sizeof(BitBuf) is used
in several comparisons like s->buf_end - s->buf_ptr >= sizeof(BitBuf)
where an immediate (of type int) has been used before. This is a
problem if one is already past the end of the buffer, because the left
side will (typically) be converted to size_t. With the current API, this
can only happen when skip_put_bits() and set_put_bits_buffer_size() are
used (or if one modifies the PutBitContext manually). But it would be a
problem if we would add an unchecked version of this API for users that
do their own checks and if said user would want to use padding in a
controlled manner to be able to minimize the amount of checks.

3. Should the "Internal error, put_bits buffer too small" error message
without proper logcontext be removed just like the one in
get_ue_golomb() was in 39c4b788297b7883d833d68fad3707ce50e01472?
(It could of course be used for the av_assert2 message.)

*: For a big-endian writer, the already buffered bits occupy the least
significant bits of the buffer. If they were already put into their
final position, one would need to disallow/special-case writing zero bits
with put_bits() (but it could then automatically be used to write up to
BIT_BUF bits).

 libavcodec/put_bits.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavcodec/put_bits.h b/libavcodec/put_bits.h
index ddd97906b2..3ba9549948 100644
--- a/libavcodec/put_bits.h
+++ b/libavcodec/put_bits.h
@@ -364,13 +364,13 @@ static inline void skip_put_bytes(PutBitContext *s, int n)
 /**
  * Skip the given number of bits.
  * Must only be used if the actual values in the bitstream do not matter.
- * If n is 0 the behavior is undefined.
+ * If n is < 0 the behavior is undefined.
  */
 static inline void skip_put_bits(PutBitContext *s, int n)
 {
-    s->bit_left -= n;
-    s->buf_ptr  -= sizeof(BitBuf) * ((unsigned)s->bit_left / BUF_BITS);
-    s->bit_left &= (BUF_BITS - 1);
+    unsigned bits = BUF_BITS - s->bit_left + n;
+    s->buf_ptr += sizeof(BitBuf) * (bits / BUF_BITS);
+    s->bit_left = BUF_BITS - (bits & (BUF_BITS - 1));
 }
 
 /**
-- 
2.20.1