[libopenapv] Fix signed overflow in VLC decoding#213
Conversation
Add bounds checks to prevent signed integer overflow during VLC decoding. 1. In dec_vlc_read_1bit_read, limit the number of leading zeros (k) to < 30 to prevent overflow when calculating symbol and suffix. 2. In dec_vlc_read, limit k to < 31 in the exp-golomb loop to prevent signed overflow in the shift operation (1 << k). 3. Update KPARAM_AC macro in oapv_vlc.h to use oapv_clip3 instead of oapv_min, ensuring the returned parameter is always >= 0. This prevents a crash in media.swcodec when processing malformed bitstreams with excessive zeros. Bug: 495077878 Test: manual verification with stagefright and PoC Flag: EXEMPT BUGFIX Change-Id: Ia44c26d6a012681d94873bb1bcd372008c8b5bda
|
@kpchoi please review |
| } | ||
| } | ||
|
|
||
| oapv_assert_rv(k < 32, -1); /* prevent too large (impossible) k value */ |
There was a problem hiding this comment.
This code can support similar assert operation without significant performance drop.
There was a problem hiding this comment.
Could you please elaborate? assert is needed to prevent more than 30 leading zeroes to be read as reading more than 30 zeroes would lead to overflow.
There was a problem hiding this comment.
One conditional brach in VLC code leads significant performance drop. and already the assert is outside of the loop.
There was a problem hiding this comment.
Yes, there is an assert outside the loop, but the value of "k" is increasing inside the loop. There will be a performance drop, correct, please recommend a better approach.
|
|
||
| #define KPARAM_DC(level) oapv_min((level)>>1, OAPV_KPARAM_DC_MAX) | ||
| #define KPARAM_AC(level) oapv_min((level)>>2, OAPV_KPARAM_AC_MAX) | ||
| #define KPARAM_AC(level) oapv_clip3(OAPV_KPARAM_AC_MIN, OAPV_KPARAM_AC_MAX, (level)>>2) |
There was a problem hiding this comment.
Level is always positive and monotonically increasing. Camparing with min value is not required.
There was a problem hiding this comment.
Unless it overflows. This code prevents crash (because of MIN_INT) in case of (unintended overflow)
There was a problem hiding this comment.
It is difficult to understand why it can prevent crash. as I mentioned, level is always positive. and it is limited to some value.
2 participants
Add bounds checking to BSW_FLUSH_4BYTE and BSW_FLUSH_8BYTE macros in oapv_vlc.c to prevent writing past the end of the bitstream buffer during VLC encoding.
Add bounds checking in enc_frame in oapv.c to ensure the cumulative tile bitstream size does not exceed the target bitstream buffer end before copying tile bitstreams.
Change-Id: Ib40bc500096b6fda93e5802d97b306e4320ba6eb