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.rheinha...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
