From 6b5fd05889cbe677f534c5c3aa9727d24c2082fd Mon Sep 17 00:00:00 2001 From: "Matthew \"strager\" Glazar" Date: Fri, 3 Nov 2023 14:44:42 -0400 Subject: [PATCH] fix(container): ensure Flexible_Array aligns capacity Flexible_Array assumes that (alignof(T) <= maximum(alignof(Header), std::size_t))). This is a bad assumption. Fix Flexible_Array to align its capacity properly, removing restrictions on T's alignment. --- src/quick-lint-js/container/flexible-array.h | 23 +++- test/CMakeLists.txt | 1 + test/test-flexible-array.cpp | 125 +++++++++++++++++++ 3 files changed, 145 insertions(+), 4 deletions(-) create mode 100644 test/test-flexible-array.cpp diff --git a/src/quick-lint-js/container/flexible-array.h b/src/quick-lint-js/container/flexible-array.h index daf6ce6626..94bb0f1838 100644 --- a/src/quick-lint-js/container/flexible-array.h +++ b/src/quick-lint-js/container/flexible-array.h @@ -5,13 +5,30 @@ #include #include +#include #include #include namespace quick_lint_js { +// Helper for Flexible_Array. Do not use directly. +// +// Flexible_Array_Base exists to have the compiler compute the required +// alignment for Header and the capacity field. +template +class Flexible_Array_Base : public Header { + protected: + template + explicit Flexible_Array_Base(std::size_t capacity, Args&&... args) + : Header(std::forward(args)...), capacity_(capacity) {} + + std::size_t capacity_; +}; + // Like a C99 flexible array member but with its size too. template -class Flexible_Array : public Header { +class alignas(maximum(alignof(T), + alignof(Flexible_Array_Base
))) Flexible_Array + : public Flexible_Array_Base
{ public: T* flexible_capacity_begin() { return reinterpret_cast(&this[1]); } @@ -53,9 +70,7 @@ class Flexible_Array : public Header { private: template explicit Flexible_Array(std::size_t capacity, Args&&... args) - : Header(std::forward(args)...), capacity_(capacity) {} - - std::size_t capacity_; + : Flexible_Array_Base
(capacity, std::forward(args)...) {} }; } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index a07387674c..b38e5cc038 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -106,6 +106,7 @@ quick_lint_js_add_executable( test-file-path.cpp test-file.cpp test-fixed-vector.cpp + test-flexible-array.cpp test-function-ref.cpp test-instance-tracker.cpp test-integer-decimal.cpp diff --git a/test/test-flexible-array.cpp b/test/test-flexible-array.cpp new file mode 100644 index 0000000000..2a90a1dd91 --- /dev/null +++ b/test/test-flexible-array.cpp @@ -0,0 +1,125 @@ +// Copyright (C) 2020 Matthew "strager" Glazar +// See end of file for extended copyright information. + +#include +#include +#include +#include +#include +#include + +namespace quick_lint_js { +namespace { +// Wraps any allocator with support for any arbitrary alignment. +// +// Designed for testing only. +// +// Each allocation has the following form: +// +// [padding][header][data] +// ^~~~~~~~~' ^ +// +// * The returned pointer points to the beginning of data. Padding ensures it is +// suitably aligned. +// * The header contains a pointer pointing to the beginning of the underlying +// allocation. This is used to free the memory. +struct Aligning_Memory_Resource : public Memory_Resource { + public: + explicit Aligning_Memory_Resource(Memory_Resource *underlying_memory) + : underlying_memory_(underlying_memory) {} + + void *do_allocate(std::size_t bytes, std::size_t alignment) override { + std::size_t underlying_size = this->underlying_size(bytes, alignment); + void *underlying_allocation = + this->underlying_memory_->allocate(underlying_size, 1); + + void *result = &reinterpret_cast
(underlying_allocation)[1]; + std::size_t space_after_result = underlying_size; + bool ok = + std::align(alignment, bytes, result, space_after_result) != nullptr; + QLJS_ALWAYS_ASSERT(ok); + + // Write the header. memcpy must be used because the header might not be + // aligned. + Header header = {.underlying_allocation = underlying_allocation}; + std::memcpy(&reinterpret_cast
(result)[-1], &header, + sizeof(Header)); + + return result; + } + + void do_deallocate(void *p, std::size_t bytes, + std::size_t alignment) override { + // Read the header. memcpy must be used because the header might not be + // aligned. + Header header; + std::memcpy(&header, &reinterpret_cast
(p)[-1], sizeof(Header)); + this->underlying_memory_->deallocate( + header.underlying_allocation, this->underlying_size(bytes, alignment), + alignment); + } + + private: + struct Header { + void *underlying_allocation; + }; + + std::size_t underlying_size(std::size_t bytes, std::size_t alignment) { + return bytes + alignment + sizeof(void *); + } + + Memory_Resource *underlying_memory_; +}; + +Aligning_Memory_Resource memory = + Aligning_Memory_Resource(new_delete_resource()); + +TEST(Test_Flexible_Array, allocating_header_creates_array) { + struct My_Array_Header {}; + using My_Array = Flexible_Array; + My_Array *a = My_Array::allocate_and_construct_header(&memory, 10); + + int *begin = a->flexible_capacity_begin(); + int *end = a->flexible_capacity_end(); + ASSERT_EQ(end - begin, 10); + ASSERT_EQ(a->flexible_capacity(), 10); + + // Ensure the array elements are writable. This is intended to cause an ASAN + // or UBSAN report if Flexible_Array is buggy. + for (int i = 0; i < 10; ++i) { + begin[i] = i * 11111111; + } + for (int i = 0; i < 10; ++i) { + ASSERT_EQ(begin[i], i * 11111111) << i; + } +} + +TEST(Test_Flexible_Array, allocating_header_over_aligned_by_type) { + struct alignas(128) My_Type {}; + struct My_Array_Header {}; + using My_Array = Flexible_Array; + My_Array *a = My_Array::allocate_and_construct_header(&memory, 10); + + My_Type *begin = a->flexible_capacity_begin(); + ASSERT_TRUE(is_aligned(begin, 128)) << begin; +} +} +} + +// quick-lint-js finds bugs in JavaScript programs. +// Copyright (C) 2020 Matthew "strager" Glazar +// +// This file is part of quick-lint-js. +// +// quick-lint-js is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// quick-lint-js is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with quick-lint-js. If not, see .