Bound exp-Golomb prefix k < 30 in dec_vlc_read()

[fkyslov]

Summary

Bounds the post-loop exp-Golomb prefix check k < 30 and explicitly casts return (int)symbol; in dec_vlc_read() (src/oapv_vlc.c) so symbol cannot exceed INT_MAX or return a negative int when adding the k-bit suffix.

Testing

• Verified all 24 ctest unit/conformance tests pass with AddressSanitizer and UndefinedBehaviorSanitizer.

[kpchoi]

The in-loop change cannot be accepted. The original symbol += 1u << (k & 31); was implemented this way on purpose, and the comment removed by this PR explains why.

This loop runs for every prefix bit in coefficient decoding, which is the hottest path of the decoder. An oapv_assert_rv(k < 30, -1) inside it adds a branch per bit and slows down decoding. The (k & 31) mask gives the same safety without a branch:

• For a valid bitstream k stays below 32, so the mask is a no-op and the decoded value is the same as in the APV spec.
• For a malformed bitstream it keeps the shift count in [0, 31], so there is no shift-count UB. Such a symbol is then rejected by the k check after the loop.

So the mask and its comment should be kept, and the in-loop assert should not be added.

This part of the code has been reported repeatedly, most likely by coding agents that flag the missing in-loop check. To prevent this, comments explaining why the check is intentionally left out of these loops and how a malformed bitstream is rejected have been added in #297.

[fkyslov]

We have reverted the in-loop change in this PR so that the PR only includes the oapv_assert_rv(k < 30, -1) and return (int)symbol; changes that we agree on.

However, to share the context on why the in-loop symbol += 1u << (k & 31); was flagged: while u32 wraparound is well-defined in standard C, Android builds libopenapv with sanitize: { integer_overflow: true } in Android.bp, which enables Clang's -fsanitize=unsigned-integer-overflow in addition to signed-integer-overflow (treating unsigned wraparound as a fatal abort in sanitized/fuzzing builds).

On a malformed bitstream with 32+ zero bits in the prefix, k reaches 31 inside the while (1) loop and symbol += 1u << (k & 31) wraps u32 (2147483680 + 2147483648), triggering a UBSan abort before the post-loop k < 30 check is reached:

src/oapv_vlc.c:780:24: runtime error: unsigned integer overflow: 2147483680 + 2147483648 cannot be represented in type 'unsigned int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/oapv_vlc.c:780:24

If you are open to avoiding that u32 wraparound without adding any branch to the hot loop, dec_vlc_read() can use the same structure as dec_vlc_read_kparam0() and dec_vlc_read_1bit_read() — doing only k++; inside the loop and adding the closed-form geometric series sum after oapv_assert_rv(k < 30, -1):

• When parse_exp_golomb == 1 (initial prefix bits 01 with initial parameter k0), (2u << k0) + sum_{j=k0..k-1} (1u << j) = (1u << k0) + (1u << k).
• Setting symbol = 1u << k; when parse_exp_golomb == 1 (or symbol = (u32)(1 + flag) << k; when flag == 0 and only symbol = 1u << k; on the parse_exp_golomb path), keeping only k++; in the loop, and computing symbol += 1u << k; after oapv_assert_rv(k < 30, -1) requires zero in-loop branch, removes the per-bit shift/add from the loop body, and never wraps u32.

[kpchoi]

The oapv_assert_rv(k >= 0 && k < 30, -1) check at the entry of dec_vlc_read() is not needed either. k does not come from the bitstream. It is always a decoder-side value set by OAPV_KPARAM_* or KPARAM_*(), so it is always in [0, OAPV_KPARAM_DC_MAX] (0 to 5). The check can never fail, and it would only add a branch to every call on the coefficient decoding path.

[fkyslov]

Agreed — removed the entry oapv_assert_rv(k >= 0 && k < 30, -1) check.

[kpchoi]

This change is reasonable.

[kpchoi]

fine.