Skip to content

Commit

Permalink
[Courgette] Fix NoThrowBuffer::end() off-by-1; add unittests.
Browse files Browse the repository at this point in the history
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}
  • Loading branch information
samuelhuang authored and Commit bot committed Jul 20, 2015
1 parent a7b2912 commit 57264dc
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 4 deletions.
1 change: 1 addition & 0 deletions courgette/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions courgette/courgette.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
12 changes: 8 additions & 4 deletions courgette/memory_allocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
77 changes: 77 additions & 0 deletions courgette/memory_allocator_unittest.cc
Original file line number Diff line number Diff line change
@@ -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 <algorithm>

#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<size_t> 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<size_t> 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());
}
}

0 comments on commit 57264dc

Please sign in to comment.