[FFmpeg-devel] [PATCH] avcodec/get_bits: Don't let number of bits_left become invalid

[Andreas Rheinhardt]

When using the checked cached bitstream reader, it can currently
happen that the GetBitContext's number of bits_left becomes wrong,
namely off by UINT_MAX due to wraparound (if it were an int, it
would become negative).

The reason for this is that reading new input is performed through
show_bits() which does not update get_vlc2() if one is already
past the end of input*, so that the following skip_remaining() may
remove more bits than are actually present (it seems to have been
designed as if it were only to be called if there are enough bits
left). (The issue could be reproduced with
fraps-v5-bouncing-balls-partial.avi from the FATE suite if fraps
were modified to use a checked bitstream reader.)

At the moment, this does not seem to cause any issues:
The only place where bits_left is used as in the right
operand of a shift is when refilling and this doesn't
happen if one is already at the end. Furthermore, get_bits_count()
and get_bits_left() return proper values, because they return
an int and so being off by UINT_MAX doesn't matter.
Some functions that check whether the cache has enough bits left
will directly read from the cache.

This commit nevertheless intends to fix this by adding a variant
of refill_32() that always adds 32 bits to bits_left, but reports
if it has added fake bits (not read from the bitstream),
so that they can be compensated for. To do so, the first thing
get_vlc2() does is ensuring that at least 32 bits are in the cache
(even if fake bits), so that all the following operations
can be performed on the cache (the earlier code potentially
refilled the cache at every reading stage).

This is also beneficial for code size. In the following,
Clang means Clang 14 and GCC GCC 11.2:

           | Clang old | Clang new | GCC old | GCC new
_________________________________________________________
fraps      |    f84    |    f04    |   c9e   |   b7e
magicyuv   |   1f2f    |   1ecf    |  1b1b   |  1a49
mvha       |    c8a    |    bfa    |  11f4   |  1168
photocd    |   12d5    |   12f9    |  15ca   |  15aa
sheervideo |  14f2a    |  11778    | 103d3   | 10339
utvideodec |   35a1    |   34d1    |  3037   |  2ea7

(None of the above files (which are all files that use
the cached bitstream reader) call get_vlc2() with a max depth
of one; in this case, saving refill_32() wins size-wise.
This is unfortunately not true in case of max_depth == 1,
where the added check leads to a slight codesize increase.)

It is unfortunately not beneficial for speed in the majority
of cases, probably because of the additional branch that
is added to the common case in which the result is obtained
after only one lookup.

*: The unchecked bitstream reader does not have this check
and is therefore unaffected by this issue.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
---
get_xbits() also suffers from the problem that it skips bits
that need not be present (no user of get_xbits() uses the cached
bitstream reader).
I think that there is another potential issue with the cached bitstream
reader: Checking for whether an overread already happened
may not work, namely if the buffer length is a multiple of four
and != 4. In this case get_bits_left() will return 0 even after an
overread. This could be fixed by modifying the overread check
s->index >> 3 >= s->buffer_end - s->buffer to ">" (this is similar
to using size_in_bits_plus8 in the ordinary GetBitContext).

 libavcodec/get_bits.h | 77 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 67 insertions(+), 10 deletions(-)

diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h
index 16f8af5107..d48ff111a4 100644
--- a/libavcodec/get_bits.h
+++ b/libavcodec/get_bits.h
@@ -227,13 +227,8 @@ static inline int get_bits_count(const GetBitContext *s)
 }
 
 #if CACHED_BITSTREAM_READER
-static inline void refill_32(GetBitContext *s, int is_le)
+static av_always_inline void refill_32_internal(GetBitContext *s, int is_le)
 {
-#if !UNCHECKED_BITSTREAM_READER
-    if (s->index >> 3 >= s->buffer_end - s->buffer)
-        return;
-#endif
-
     if (is_le)
         s->cache = (uint64_t)AV_RL32(s->buffer + (s->index >> 3)) << s->bits_left | s->cache;
     else
@@ -242,6 +237,52 @@ static inline void refill_32(GetBitContext *s, int is_le)
     s->bits_left += 32;
 }
 
+/**
+ * If the end of input has not been reached yet, refill the GetBitContext
+ * with 32 bits read from the bitstream.
+ * Otherwise just pretend that it read 32 zeroes. One can then use
+ * show_val()/get_val()/skip_remaining() (which require enough cache bits
+ * to be available) as if there were at least 32 bits available;
+ * all other functions not solely based upon these are forbidden.
+ * Lateron one has to use fixup_fake with the return value of this function
+ * to restore the GetBitContext.
+ *
+ * This function is internal to the GetBit-API; access from users is forbidden.
+ *
+ * @return The amount of fake bits added.
+ */
+static inline unsigned refill_32_fake(GetBitContext *s, int is_le)
+{
+#if !UNCHECKED_BITSTREAM_READER
+    if (s->index >> 3 >= s->buffer_end - s->buffer) {
+        s->bits_left += 32;
+        return 32;
+    }
+#endif
+    refill_32_internal(s, is_le);
+    return 0;
+}
+
+static inline void fixup_fake(GetBitContext *s, unsigned fake_bits)
+{
+#if !UNCHECKED_BITSTREAM_READER
+    if (s->bits_left <= fake_bits)
+        s->bits_left  = 0;
+    else
+        s->bits_left -= fake_bits;
+#endif
+}
+
+static inline void refill_32(GetBitContext *s, int is_le)
+{
+#if !UNCHECKED_BITSTREAM_READER
+    if (s->index >> 3 >= s->buffer_end - s->buffer)
+        return;
+#endif
+    refill_32_internal(s, is_le);
+    return;
+}
+
 static inline void refill_64(GetBitContext *s, int is_le)
 {
 #if !UNCHECKED_BITSTREAM_READER
@@ -773,6 +814,7 @@ static inline const uint8_t *align_get_bits(GetBitContext *s)
         SKIP_BITS(name, gb, n);                                 \
     } while (0)
 
+#if CACHED_BITSTREAM_READER
 /* Return the LUT element for the given bitstream configuration. */
 static inline int set_idx(GetBitContext *s, int code, int *n, int *nb_bits,
                           const VLCElem *table)
@@ -780,11 +822,12 @@ static inline int set_idx(GetBitContext *s, int code, int *n, int *nb_bits,
     unsigned idx;
 
     *nb_bits = -*n;
-    idx = show_bits(s, *nb_bits) + code;
+    idx = show_val(s, *nb_bits) + code;
     *n = table[idx].len;
 
     return table[idx].sym;
 }
+#endif
 
 /**
  * Parse a vlc code.
@@ -800,9 +843,21 @@ static av_always_inline int get_vlc2(GetBitContext *s, const VLCElem *table,
 {
 #if CACHED_BITSTREAM_READER
     int nb_bits;
-    unsigned idx = show_bits(s, bits);
-    int code = table[idx].sym;
-    int n    = table[idx].len;
+    unsigned fake_bits = 0;
+    unsigned idx;
+    int n, code;
+
+    /* We only support VLC tables whose codelengths are bounded by 32. */
+    if (s->bits_left < 32)
+#ifdef BITSTREAM_READER_LE
+        fake_bits = refill_32_fake(s, 1);
+#else
+        fake_bits = refill_32_fake(s, 0);
+#endif
+
+    idx = show_val(s, bits);
+    code = table[idx].sym;
+    n   = table[idx].len;
 
     if (max_depth > 1 && n < 0) {
         skip_remaining(s, bits);
@@ -814,6 +869,8 @@ static av_always_inline int get_vlc2(GetBitContext *s, const VLCElem *table,
     }
     skip_remaining(s, n);
 
+    fixup_fake(s, fake_bits);
+
     return code;
 #else
     int code;
-- 
2.34.1