From 06fef1e9f11a45df809e7cc4522f7174f48a664f Mon Sep 17 00:00:00 2001 From: Andreas Rheinhardt Date: Fri, 31 Jul 2020 08:32:55 +0200 Subject: 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 --- 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)); } /** -- cgit v1.2.3