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

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@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(-)

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@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));
>  }
>  
>  /**
> 
Will apply this tomorrow unless there are objections.

- Andreas