Bound exp-Golomb prefix k < 30 in dec_vlc_read_kparam0() and dec_vlc_read_1bit_read()

[fkyslov]

Summary

Bounds the exp-Golomb prefix length k < 30 and accumulates symbol in u32 in dec_vlc_read_kparam0() and dec_vlc_read_1bit_read() (src/oapv_vlc.c) to prevent signed/unsigned integer overflow and shift undefined behavior on malformed bitstreams with long zero-prefix runs.

Testing

• Verified all 24 ctest unit/conformance tests pass with AddressSanitizer and UndefinedBehaviorSanitizer (-fsanitize=address,signed-integer-overflow,unsigned-integer-overflow,shift,bounds).
• Verified against malformed VLC bitstreams.

[kpchoi]

Firstly, Regarding 679 and 719 of your patch,

The in-loop k checks added to dec_vlc_read_kparam0() and dec_vlc_read_1bit_read() are not needed. These loops cannot run forever, even on a malformed bitstream.

• At the end of the buffer, BSR_FLUSH_1BYTE() does not read past it. It fills code with all 1-bits (~(u64)0) and sets is_eob.
• The next BSR_READ_1BIT() then returns 1, so flag becomes true and the loop exits through break.
• Inside these loops k is only incremented and never used as a shift count. The existing check after the loop rejects too large a k before it is used.

The error is detected later instead (deferred error checking). The callers test BSR_IS_UNEXPECTED_EOB() after parsing the coefficients and return OAPV_ERR_MALFORMED_BITSTREAM. This is intentional. These two functions are the k = 0 paths and are the ones called most often in coefficient decoding, so a per-bit check in their loops would slow down decoding.

Comments explaining this will be added near these loops.

[kpchoi]

The oapv_clip3() change to the KPARAM_* macros is not needed either. Their arguments are never negative, and OAPV_KPARAM_*_MIN is 0, so the lower clamp can never take effect:

• KPARAM_DC() gets abs_dc_diff, which is at least 1 at that point.
• KPARAM_AC() gets level, which is at least 1.
• KPARAM_RUN() gets run, which has already been range-checked to [0, OAPV_BLK_D - scan_pos_offset].
• On the encoder side, only absolute values are passed.

These macros are evaluated for every coefficient. In VLC decoding even one extra branch or compare per coefficient noticeably affects decoding speed, so oapv_clip3() is intentionally not used here.

[kpchoi]

Changing symbol from int to u32 is not needed for correctness. The additions are already done in unsigned arithmetic (int + u32, int + u64), so there is no signed overflow. With the k check after the loop, the result always fits in int.

Still, the change is fine to keep as a cosmetic change. It matches dec_vlc_read(), which already uses u32 symbol, and it keeps code reviewers and coding agents from mistaking this pattern for a signed overflow.

[kpchoi]

This is a correct comparison. thanks.

[kpchoi]

This line is strongly suggested to be removed because of the reason commented.

[kpchoi]

This line is strongly suggested to be removed because of the reason commented.

[kpchoi]

This 3 line are not required to be changed because of the reason commented.

[fkyslov]

Updated per review feedback: removed the in-loop oapv_assert_rv(k < 30, -1) checks and reverted the redundant oapv_clip3 macro changes in src/oapv_vlc.h, keeping only the post-loop oapv_assert_rv(k < 30, -1) and u32 symbol in dec_vlc_read_kparam0() and dec_vlc_read_1bit_read().

[kpchoi]

Great. thanks.