From c19333173ab5b3a7db155169f162af2939f36df3 Mon Sep 17 00:00:00 2001 From: Pascal Massimino Date: Wed, 1 Aug 2012 00:37:24 -0700 Subject: [PATCH] extra size check for security no speed diff observed by removing the test before calling BitWriterResize(). + remove some unnecessary memset() in VP8LBitWriter + fix mixed code/variable-decl in BIG_ENDIAN mode Change-Id: I36be61f83d10a43e4682b680c2dae0e494da4218 --- src/utils/bit_writer.c | 74 ++++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 31 deletions(-) diff --git a/src/utils/bit_writer.c b/src/utils/bit_writer.c index fcb0a3c9..671159ca 100644 --- a/src/utils/bit_writer.c +++ b/src/utils/bit_writer.c @@ -25,18 +25,23 @@ extern "C" { static int BitWriterResize(VP8BitWriter* const bw, size_t extra_size) { uint8_t* new_buf; size_t new_size; - const size_t needed_size = bw->pos_ + extra_size; + const uint64_t needed_size_64b = (uint64_t)bw->pos_ + extra_size; + const size_t needed_size = (size_t)needed_size_64b; + if (needed_size_64b != needed_size) { + bw->error_ = 1; + return 0; + } if (needed_size <= bw->max_pos_) return 1; + // If the following line wraps over 32bit, the test just after will catch it. new_size = 2 * bw->max_pos_; - if (new_size < needed_size) - new_size = needed_size; + if (new_size < needed_size) new_size = needed_size; if (new_size < 1024) new_size = 1024; new_buf = (uint8_t*)malloc(new_size); if (new_buf == NULL) { bw->error_ = 1; return 0; } - if (bw->pos_ > 0) memcpy(new_buf, bw->buf_, bw->pos_); + memcpy(new_buf, bw->buf_, bw->pos_); free(bw->buf_); bw->buf_ = new_buf; bw->max_pos_ = new_size; @@ -51,10 +56,8 @@ static void kFlush(VP8BitWriter* const bw) { bw->nb_bits_ -= 8; if ((bits & 0xff) != 0xff) { size_t pos = bw->pos_; - if (pos + bw->run_ >= bw->max_pos_) { // reallocate - if (!BitWriterResize(bw, bw->run_ + 1)) { - return; - } + if (!BitWriterResize(bw, bw->run_ + 1)) { + return; } if (bits & 0x100) { // overflow -> propagate carry over pending 0xff's if (pos > 0) bw->buf_[pos - 1]++; @@ -194,23 +197,28 @@ void VP8BitWriterWipeOut(VP8BitWriter* const bw) { static int VP8LBitWriterResize(VP8LBitWriter* const bw, size_t extra_size) { uint8_t* allocated_buf; size_t allocated_size; - const size_t size_required = VP8LBitWriterNumBytes(bw) + extra_size; - if ((bw->max_bytes_ > 0) && (size_required <= bw->max_bytes_)) return 1; - allocated_size = (3 * bw->max_bytes_) >> 1; - if (allocated_size < size_required) { - allocated_size = size_required; + const size_t current_size = VP8LBitWriterNumBytes(bw); + const uint64_t size_required_64b = (uint64_t)current_size + extra_size; + const size_t size_required = (size_t)size_required_64b; + if (size_required != size_required_64b) { + bw->error_ = 1; + return 0; } - // Make Allocated size multiple of KBs + if (bw->max_bytes_ > 0 && size_required <= bw->max_bytes_) return 1; + allocated_size = (3 * bw->max_bytes_) >> 1; + if (allocated_size < size_required) allocated_size = size_required; + // make allocated size multiple of 1k allocated_size = (((allocated_size >> 10) + 1) << 10); allocated_buf = (uint8_t*)malloc(allocated_size); - if (allocated_buf == NULL) return 0; - memset(allocated_buf, 0, allocated_size); - if (bw->bit_pos_ > 0) { - memcpy(allocated_buf, bw->buf_, VP8LBitWriterNumBytes(bw)); + if (allocated_buf == NULL) { + bw->error_ = 1; + return 0; } + memcpy(allocated_buf, bw->buf_, current_size); free(bw->buf_); bw->buf_ = allocated_buf; bw->max_bytes_ = allocated_size; + memset(allocated_buf + current_size, 0, allocated_size - current_size); return 1; } @@ -232,33 +240,37 @@ void VP8LWriteBits(VP8LBitWriter* const bw, int n_bits, uint32_t bits) { // Technically, this branch of the code can write up to 25 bits at a time, // but in prefix encoding, the maximum number of bits written is 18 at a time. { - uint8_t* p = &bw->buf_[bw->bit_pos_ >> 3]; - uint32_t v = *(const uint32_t*)(p); + uint8_t* const p = &bw->buf_[bw->bit_pos_ >> 3]; + uint32_t v = *(const uint32_t*)p; v |= bits << (bw->bit_pos_ & 7); - *(uint32_t*)(p) = v; + *(uint32_t*)p = v; bw->bit_pos_ += n_bits; } -#else // LITTLE_ENDIAN - // implicit & 0xff is assumed for uint8_t arithmetics +#else // BIG_ENDIAN { uint8_t* p = &bw->buf_[bw->bit_pos_ >> 3]; - const int bits_reserved_in_first_byte = (bw->bit_pos_ & 7); - *p++ |= (bits << bits_reserved_in_first_byte); + const int bits_reserved_in_first_byte = bw->bit_pos_ & 7; const int bits_left_to_write = n_bits - 8 + bits_reserved_in_first_byte; + // implicit & 0xff is assumed for uint8_t arithmetics + *p++ |= bits << bits_reserved_in_first_byte; + bits >>= 8 - bits_reserved_in_first_byte; if (bits_left_to_write >= 1) { - *p++ = bits >> (8 - bits_reserved_in_first_byte); + *p++ = bits; + bits >>= 8; if (bits_left_to_write >= 9) { - *p++ = bits >> (16 - bits_reserved_in_first_byte); + *p++ = bits; + bits >>= 8; } } assert(n_bits <= 25); - *p = bits >> (24 - bits_reserved_in_first_byte); + *p = bits; bw->bit_pos_ += n_bits; } -#endif // BIG_ENDIAN +#endif if ((bw->bit_pos_ >> 3) > (bw->max_bytes_ - 8)) { - const size_t kAdditionalBuffer = 32768 + bw->max_bytes_; - if (!VP8LBitWriterResize(bw, kAdditionalBuffer)) { + const uint64_t extra_size = 32768ULL + bw->max_bytes_; + if (extra_size != (size_t)extra_size || + !VP8LBitWriterResize(bw, (size_t)extra_size)) { bw->bit_pos_ = 0; bw->error_ = 1; }