From 57264dc9461022d53617baa238d6f38376a6cd62 Mon Sep 17 00:00:00 2001 From: huangs Date: Mon, 20 Jul 2015 14:55:04 -0700 Subject: [PATCH] [Courgette] Fix NoThrowBuffer::end() off-by-1; add unittests. NoThrowBuffer::end() should be an exclusive upperbound, but the old implementation was inclusive. The impact is that std::sort() statements in - EncodedProgram::GeneratePeRelocations() - EncodedProgram::GenerateElfRelocations() will not sort the last element, leading to slight change in results. Also found potential use-after-free in NoThrowBuffer::reserve(), but using DCHECK() to block offending case Added basic unit tests for NoThrowBuffer. TEST=courgette_unittests --gtest_filter=MemoryAllocatorTest.NoThrowBuffer Review URL: https://codereview.chromium.org/1242263003 Cr-Commit-Position: refs/heads/master@{#339529} --- courgette/BUILD.gn | 1 + courgette/courgette.gyp | 1 + courgette/memory_allocator.h | 12 ++-- courgette/memory_allocator_unittest.cc | 77 ++++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 courgette/memory_allocator_unittest.cc diff --git a/courgette/BUILD.gn b/courgette/BUILD.gn index ce89c42c11206e..a885678c5c1ab5 100644 --- a/courgette/BUILD.gn +++ b/courgette/BUILD.gn @@ -101,6 +101,7 @@ test("courgette_unittests") { "encode_decode_unittest.cc", "encoded_program_unittest.cc", "ensemble_unittest.cc", + "memory_allocator_unittest.cc", "streams_unittest.cc", "third_party/paged_array_unittest.cc", "typedrva_unittest.cc", diff --git a/courgette/courgette.gyp b/courgette/courgette.gyp index bc8b7bba4011d3..1eb669db1156bc 100644 --- a/courgette/courgette.gyp +++ b/courgette/courgette.gyp @@ -105,6 +105,7 @@ 'encoded_program_unittest.cc', 'encode_decode_unittest.cc', 'ensemble_unittest.cc', + 'memory_allocator_unittest.cc', 'streams_unittest.cc', 'typedrva_unittest.cc', 'versioning_unittest.cc', diff --git a/courgette/memory_allocator.h b/courgette/memory_allocator.h index ada7f40b8438b4..15b709e9c843fe 100644 --- a/courgette/memory_allocator.h +++ b/courgette/memory_allocator.h @@ -365,6 +365,10 @@ class NoThrowBuffer { if (!size) return true; + // Disallow source range from overlapping with buffer_, since in this case + // reallocation would cause use-after-free. + DCHECK(data + size <= buffer_ || data >= buffer_ + alloc_size_); + if ((alloc_size_ - size_) < size) { const size_t max_size = alloc_.max_size(); size_t new_size = alloc_size_ ? alloc_size_ : kStartSize; @@ -416,25 +420,25 @@ class NoThrowBuffer { const T* begin() const { if (!size_) return NULL; - return &buffer_[0]; + return buffer_; } T* begin() { if (!size_) return NULL; - return &buffer_[0]; + return buffer_; } const T* end() const { if (!size_) return NULL; - return &buffer_[size_ - 1]; + return buffer_ + size_; } T* end() { if (!size_) return NULL; - return &buffer_[size_ - 1]; + return buffer_ + size_; } const T& operator[](size_t index) const { diff --git a/courgette/memory_allocator_unittest.cc b/courgette/memory_allocator_unittest.cc new file mode 100644 index 00000000000000..335cdc9f8dcfef --- /dev/null +++ b/courgette/memory_allocator_unittest.cc @@ -0,0 +1,77 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "courgette/memory_allocator.h" + +#include + +#include "base/macros.h" +#include "testing/gtest/include/gtest/gtest.h" + +TEST(MemoryAllocatorTest, NoThrowBuffer) { + const size_t size_list[] = {0U, 1U, 2U, 11U, 15U, 16U}; + + // Repeat test for different sizes. + for (size_t idx = 0; idx < arraysize(size_list); ++idx) { + size_t size = size_list[idx]; + + courgette::NoThrowBuffer buf1; + EXPECT_EQ(0U, buf1.size()); + EXPECT_TRUE(buf1.empty()); + + // Ensure reserve() should not affect size. + EXPECT_TRUE(buf1.reserve(size / 2)); + EXPECT_EQ(0U, buf1.size()); + EXPECT_TRUE(buf1.empty()); + + // Populate with integers from |size| - 1 to 0. + for (size_t i = 0; i < size; ++i) { + size_t new_value = size - 1 - i; + EXPECT_TRUE(buf1.push_back(new_value)); + EXPECT_EQ(new_value, buf1.back()); + EXPECT_EQ(i + 1, buf1.size()); + EXPECT_FALSE(buf1.empty()); + } + + // Sort, and verify that list is indeed sorted. + std::sort(buf1.begin(), buf1.end()); + for (size_t i = 0; i < size; ++i) + EXPECT_EQ(i, buf1[i]); + + // Test operator[] for read and write. + for (size_t i = 0; i < size; ++i) + buf1[i] = buf1[i] * 2; + + // Test append(). + courgette::NoThrowBuffer buf2; + + if (size > 0) { + EXPECT_TRUE(buf2.append(&buf1[0], size)); + EXPECT_EQ(size, buf2.size()); + for (size_t i = 0; i < size; ++i) + EXPECT_EQ(buf1[i], buf2[i]); + } + + // Test shrinking by resize(). + const size_t kNewValue = 137; + size_t new_size = size / 2; + EXPECT_TRUE(buf2.resize(new_size, kNewValue)); + EXPECT_EQ(new_size, buf2.size()); + for (size_t i = 0; i < new_size; ++i) + EXPECT_EQ(buf1[i], buf2[i]); + + // Test expanding by resize(). + EXPECT_TRUE(buf2.resize(size, kNewValue)); + EXPECT_EQ(size, buf2.size()); + for (size_t i = 0; i < new_size; ++i) + EXPECT_EQ(buf1[i], buf2[i]); + for (size_t i = new_size; i < size; ++i) + EXPECT_EQ(kNewValue, buf2[i]); + + // Test clear(). + buf2.clear(); + EXPECT_EQ(0U, buf2.size()); + EXPECT_TRUE(buf2.empty()); + } +}