VP8LBitReader: fix remaining ubsan error with large shifts

* make VP8LPrefetchBits() safe wrt past-EOS reads
* set 'BitReader::bits_" to a safe shifting value upon EOS

no visible performance difference on x86

Change-Id: I0a4177928cfa81d5dfc9054b36a686eaa1bf8c65
This commit is contained in:
Pascal Massimino 2015-03-13 19:37:44 +00:00 committed by Gerrit Code Review
parent ec1fb9f8dd
commit 7a01c3c3ec
2 changed files with 15 additions and 7 deletions

View File

@ -83,6 +83,8 @@ void VP8LoadFinalBytes(VP8BitReader* const br) {
br->value_ <<= 8; br->value_ <<= 8;
br->bits_ += 8; br->bits_ += 8;
br->eof_ = 1; br->eof_ = 1;
} else {
br->bits_ = 0; // This is to avoid undefined behaviour with shifts.
} }
} }
@ -159,6 +161,11 @@ void VP8LBitReaderSetBuffer(VP8LBitReader* const br,
br->eos_ = (br->pos_ > br->len_) || VP8LIsEndOfStream(br); br->eos_ = (br->pos_ > br->len_) || VP8LIsEndOfStream(br);
} }
static void VP8LSetEndOfStream(VP8LBitReader* const br) {
br->eos_ = 1;
br->bit_pos_ = 0; // To avoid undefined behaviour with shifts.
}
// If not at EOS, reload up to VP8L_LBITS byte-by-byte // If not at EOS, reload up to VP8L_LBITS byte-by-byte
static void ShiftBytes(VP8LBitReader* const br) { static void ShiftBytes(VP8LBitReader* const br) {
while (br->bit_pos_ >= 8 && br->pos_ < br->len_) { while (br->bit_pos_ >= 8 && br->pos_ < br->len_) {
@ -167,7 +174,9 @@ static void ShiftBytes(VP8LBitReader* const br) {
++br->pos_; ++br->pos_;
br->bit_pos_ -= 8; br->bit_pos_ -= 8;
} }
br->eos_ = VP8LIsEndOfStream(br); if (VP8LIsEndOfStream(br)) {
VP8LSetEndOfStream(br);
}
} }
void VP8LDoFillBitWindow(VP8LBitReader* const br) { void VP8LDoFillBitWindow(VP8LBitReader* const br) {
@ -193,14 +202,13 @@ uint32_t VP8LReadBits(VP8LBitReader* const br, int n_bits) {
assert(n_bits >= 0); assert(n_bits >= 0);
// Flag an error if end_of_stream or n_bits is more than allowed limit. // Flag an error if end_of_stream or n_bits is more than allowed limit.
if (!br->eos_ && n_bits <= VP8L_MAX_NUM_BIT_READ) { if (!br->eos_ && n_bits <= VP8L_MAX_NUM_BIT_READ) {
const uint32_t val = const uint32_t val = VP8LPrefetchBits(br) & kBitMask[n_bits];
(uint32_t)(br->val_ >> br->bit_pos_) & kBitMask[n_bits];
const int new_bits = br->bit_pos_ + n_bits; const int new_bits = br->bit_pos_ + n_bits;
br->bit_pos_ = new_bits; br->bit_pos_ = new_bits;
ShiftBytes(br); ShiftBytes(br);
return val; return val;
} else { } else {
br->eos_ = 1; VP8LSetEndOfStream(br);
return 0; return 0;
} }
} }

View File

@ -107,7 +107,7 @@ int32_t VP8GetSignedValue(VP8BitReader* const br, int num_bits);
// maximum number of bits (inclusive) the bit-reader can handle: // maximum number of bits (inclusive) the bit-reader can handle:
#define VP8L_MAX_NUM_BIT_READ 24 #define VP8L_MAX_NUM_BIT_READ 24
#define VP8L_LBITS 64 // Number of bits prefetched. #define VP8L_LBITS 64 // Number of bits prefetched (= bit-size of vp8l_val_t).
#define VP8L_WBITS 32 // Minimum number of bytes ready after VP8LFillBitWindow. #define VP8L_WBITS 32 // Minimum number of bytes ready after VP8LFillBitWindow.
typedef uint64_t vp8l_val_t; // right now, this bit-reader can only use 64bit. typedef uint64_t vp8l_val_t; // right now, this bit-reader can only use 64bit.
@ -137,14 +137,14 @@ uint32_t VP8LReadBits(VP8LBitReader* const br, int n_bits);
// Return the prefetched bits, so they can be looked up. // Return the prefetched bits, so they can be looked up.
static WEBP_INLINE uint32_t VP8LPrefetchBits(VP8LBitReader* const br) { static WEBP_INLINE uint32_t VP8LPrefetchBits(VP8LBitReader* const br) {
return (uint32_t)(br->val_ >> br->bit_pos_); return (uint32_t)(br->val_ >> (br->bit_pos_ & (VP8L_LBITS - 1)));
} }
// Returns true if there was an attempt at reading bit past the end of // Returns true if there was an attempt at reading bit past the end of
// the buffer. Doesn't set br->eos_ flag. // the buffer. Doesn't set br->eos_ flag.
static WEBP_INLINE int VP8LIsEndOfStream(const VP8LBitReader* const br) { static WEBP_INLINE int VP8LIsEndOfStream(const VP8LBitReader* const br) {
assert(br->pos_ <= br->len_); assert(br->pos_ <= br->len_);
return (br->pos_ == br->len_) && (br->bit_pos_ > VP8L_LBITS); return br->eos_ || ((br->pos_ == br->len_) && (br->bit_pos_ > VP8L_LBITS));
} }
// For jumping over a number of bits in the bit stream when accessed with // For jumping over a number of bits in the bit stream when accessed with