From b37b0179c5a1fbecab13c7cd847b864d61da23c7 Mon Sep 17 00:00:00 2001 From: Pascal Massimino Date: Fri, 20 Nov 2015 09:12:48 +0000 Subject: [PATCH] fix for issue #275: don't compare to out-of-bound pointers the original change triggered several internal API modifs. This is to ensure that we're never computing pointer that can possibly wrap around, or differences between pointers that can overflow. no observed speed difference Change-Id: I9c94dda38d94fecc010305e4ad12f13b8fda5380 --- src/dec/idec.c | 11 +++++++---- src/dec/vp8.c | 24 +++++++++++++----------- src/utils/bit_reader.c | 18 ++++++++++++++---- src/utils/bit_reader.h | 6 +++++- src/utils/bit_reader_inl.h | 2 +- 5 files changed, 40 insertions(+), 21 deletions(-) diff --git a/src/dec/idec.c b/src/dec/idec.c index d6ac3ed8..abafb9f3 100644 --- a/src/dec/idec.c +++ b/src/dec/idec.c @@ -130,8 +130,12 @@ static void DoRemap(WebPIDecoder* const idec, ptrdiff_t offset) { VP8RemapBitReader(&dec->br_, offset); } } - assert(last_part >= 0); - dec->parts_[last_part].buf_end_ = mem->buf_ + mem->end_; + { + const uint8_t* const last_start = dec->parts_[last_part].buf_; + assert(last_part >= 0); + VP8BitReaderSetBuffer(&dec->parts_[last_part], last_start, + mem->buf_ + mem->end_ - last_start); + } if (NeedCompressedAlpha(idec)) { ALPHDecoder* const alph_dec = dec->alph_dec_; dec->alpha_data_ += offset; @@ -375,8 +379,7 @@ static VP8StatusCode CopyParts0Data(WebPIDecoder* const idec) { } memcpy(part0_buf, br->buf_, part_size); mem->part0_buf_ = part0_buf; - br->buf_ = part0_buf; - br->buf_end_ = part0_buf + part_size; + VP8BitReaderSetBuffer(br, part0_buf, part_size); } else { // Else: just keep pointers to the partition #0's data in dec_->br_. } diff --git a/src/dec/vp8.c b/src/dec/vp8.c index 22228ab5..d89eb1c5 100644 --- a/src/dec/vp8.c +++ b/src/dec/vp8.c @@ -190,25 +190,27 @@ static VP8StatusCode ParsePartitions(VP8Decoder* const dec, const uint8_t* sz = buf; const uint8_t* buf_end = buf + size; const uint8_t* part_start; - int last_part; - int p; + size_t size_left = size; + size_t last_part; + size_t p; dec->num_parts_ = 1 << VP8GetValue(br, 2); last_part = dec->num_parts_ - 1; - part_start = buf + last_part * 3; - if (buf_end < part_start) { + if (size < 3 * last_part) { // we can't even read the sizes with sz[]! That's a failure. return VP8_STATUS_NOT_ENOUGH_DATA; } + part_start = buf + last_part * 3; + size_left -= last_part * 3; for (p = 0; p < last_part; ++p) { - const uint32_t psize = sz[0] | (sz[1] << 8) | (sz[2] << 16); - const uint8_t* part_end = part_start + psize; - if (part_end > buf_end) part_end = buf_end; - VP8InitBitReader(dec->parts_ + p, part_start, part_end); - part_start = part_end; + size_t psize = sz[0] | (sz[1] << 8) | (sz[2] << 16); + if (psize > size_left) psize = size_left; + VP8InitBitReader(dec->parts_ + p, part_start, psize); + part_start += psize; + size_left -= psize; sz += 3; } - VP8InitBitReader(dec->parts_ + last_part, part_start, buf_end); + VP8InitBitReader(dec->parts_ + last_part, part_start, size_left); return (part_start < buf_end) ? VP8_STATUS_OK : VP8_STATUS_SUSPENDED; // Init is ok, but there's not enough data } @@ -325,7 +327,7 @@ int VP8GetHeaders(VP8Decoder* const dec, VP8Io* const io) { } br = &dec->br_; - VP8InitBitReader(br, buf, buf + frm_hdr->partition_length_); + VP8InitBitReader(br, buf, frm_hdr->partition_length_); buf += frm_hdr->partition_length_; buf_size -= frm_hdr->partition_length_; diff --git a/src/utils/bit_reader.c b/src/utils/bit_reader.c index a268c646..cd265321 100644 --- a/src/utils/bit_reader.c +++ b/src/utils/bit_reader.c @@ -20,17 +20,26 @@ //------------------------------------------------------------------------------ // VP8BitReader +void VP8BitReaderSetBuffer(VP8BitReader* const br, + const uint8_t* const start, + size_t size) { + br->buf_ = start; + br->buf_end_ = start + size; + br->buf_max_ = + (size >= sizeof(lbit_t)) ? start + size - sizeof(lbit_t) + 1 + : start; +} + void VP8InitBitReader(VP8BitReader* const br, - const uint8_t* const start, const uint8_t* const end) { + const uint8_t* const start, size_t size) { assert(br != NULL); assert(start != NULL); - assert(start <= end); + assert(size < (1u << 31)); // limit ensured by format and upstream checks br->range_ = 255 - 1; - br->buf_ = start; - br->buf_end_ = end; br->value_ = 0; br->bits_ = -8; // to load the very first 8bits br->eof_ = 0; + VP8BitReaderSetBuffer(br, start, size); VP8LoadNewBytes(br); } @@ -38,6 +47,7 @@ void VP8RemapBitReader(VP8BitReader* const br, ptrdiff_t offset) { if (br->buf_ != NULL) { br->buf_ += offset; br->buf_end_ += offset; + br->buf_max_ += offset; } } diff --git a/src/utils/bit_reader.h b/src/utils/bit_reader.h index 4ebbe539..0fc62d33 100644 --- a/src/utils/bit_reader.h +++ b/src/utils/bit_reader.h @@ -74,12 +74,16 @@ struct VP8BitReader { // read buffer const uint8_t* buf_; // next byte to be read const uint8_t* buf_end_; // end of read buffer + const uint8_t* buf_max_; // max packed-read position on buffer int eof_; // true if input is exhausted }; // Initialize the bit reader and the boolean decoder. void VP8InitBitReader(VP8BitReader* const br, - const uint8_t* const start, const uint8_t* const end); + const uint8_t* const start, size_t size); +// Sets the working read buffer. +void VP8BitReaderSetBuffer(VP8BitReader* const br, + const uint8_t* const start, size_t size); // Update internal pointers to displace the byte buffer by the // relative offset 'offset'. diff --git a/src/utils/bit_reader_inl.h b/src/utils/bit_reader_inl.h index 1f79c941..37215702 100644 --- a/src/utils/bit_reader_inl.h +++ b/src/utils/bit_reader_inl.h @@ -58,7 +58,7 @@ void VP8LoadFinalBytes(VP8BitReader* const br); static WEBP_INLINE void VP8LoadNewBytes(VP8BitReader* const br) { assert(br != NULL && br->buf_ != NULL); // Read 'BITS' bits at a time if possible. - if (br->buf_ + sizeof(lbit_t) <= br->buf_end_) { + if (br->buf_ < br->buf_max_) { // convert memory type to register type (with some zero'ing!) bit_t bits; #if defined(WEBP_FORCE_ALIGNED)