[FFmpeg-devel] [PATCH] cbs_av1: Reject thirty-two zero bits in uvlc code

[Mark Thompson]

The spec allows at least thirty-two zero bits followed by a one to mean
2^32-1, with no constraint on the number of zeroes.  The libaom
reference decoder does not match this, instead reading thirty-two zeroes
but not the following one to mean 2^32-1.  These two interpretations are
incompatible and other implementations may follow one or the other.
Therefore reject thirty-two zeroes because the intended behaviour is not
clear.
---
libaom, dav1d and SVT-AV1 all have the same nonstandard behaviour of stopping at thirty-two zeroes and not reading the one.  gav1 just rejects thirty-two zeroes.

This is also a source of arbitrarily large single syntax elements to hit <https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2023-October/315973.html>.

  libavcodec/cbs_av1.c | 18 +++++++++++++-----
  1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
index 1d9ac5ab44..13c749a25b 100644
--- a/libavcodec/cbs_av1.c
+++ b/libavcodec/cbs_av1.c
@@ -36,7 +36,7 @@ static int cbs_av1_read_uvlc(CodedBitstreamContext *ctx, GetBitContext *gbc,
      CBS_TRACE_READ_START();

      zeroes = 0;
-    while (1) {
+    while (zeroes < 32) {
          if (get_bits_left(gbc) < 1) {
              av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid uvlc code at "
                     "%s: bitstream ended.\n", name);
@@ -49,10 +49,18 @@ static int cbs_av1_read_uvlc(CodedBitstreamContext *ctx, GetBitContext *gbc,
      }

      if (zeroes >= 32) {
-        // Note that the spec allows an arbitrarily large number of
-        // zero bits followed by a one bit in this case, but the
-        // libaom implementation does not support it.
-        value = MAX_UINT_BITS(32);
+        // The spec allows at least thirty-two zero bits followed by a
+        // one to mean 2^32-1, with no constraint on the number of
+        // zeroes.  The libaom reference decoder does not match this,
+        // instead reading thirty-two zeroes but not the following one
+        // to mean 2^32-1.  These two interpretations are incompatible
+        // and other implementations may follow one or the other.
+        // Therefore we reject thirty-two zeroes because the intended
+        // behaviour is not clear.
+        av_log(ctx->log_ctx, AV_LOG_ERROR, "Thirty-two zero bits in "
+               "%s uvlc code: considered invalid due to conflicting "
+               "standard and reference decoder behaviour.\n", name);
+        return AVERROR_INVALIDDATA;
      } else {
          if (get_bits_left(gbc) < zeroes) {
              av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid uvlc code at "
-- 
2.39.2