diff --git a/.github/workflows/bazel.yml b/.github/workflows/bazel.yml index 26d28c6..13667fa 100644 --- a/.github/workflows/bazel.yml +++ b/.github/workflows/bazel.yml @@ -10,20 +10,34 @@ env: jobs: check_lints: + name: Check Lints runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v2 - - name: Check format - run: ./format.sh + - name: Check Format + run: "./format.sh" build_and_test: + name: Build and Test (fastbuild) runs-on: ubuntu-latest steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v2 - - name: Build everything - run: ./bazel build //... + - name: Build Everything + run: "./bazel build //... -c fastbuild" - - name: Run tests - run: ./bazel test //... \ No newline at end of file + - name: Test Everything + run: "./bazel test //... -c fastbuild" + + build_and_test_opt: + name: Build and Test (opt) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + + - name: Build Everything + run: "./bazel build //... -c opt" + + - name: Test Everything + run: "./bazel test //... -c opt" diff --git a/best/container/span_test.cc b/best/container/span_test.cc index 47e81e7..39af335 100644 --- a/best/container/span_test.cc +++ b/best/container/span_test.cc @@ -207,6 +207,9 @@ best::test FrontAndBack = [](auto& t) { best::vec ints = {1, 2, 3, 4, 5}; best::span sp = ints; + t.expect_eq(ints.size(), 5); + t.expect_eq(ints[4], 5); + t.expect_eq(ints, {1, 2, 3, 4, 5}); t.expect_eq(sp.first(), 1); t.expect_eq(sp.last(), 5); @@ -366,11 +369,16 @@ struct NonPod { ~NonPod() {} bool operator==(int y) const { return x == y; } - friend auto& operator<<(auto& os, NonPod np) { return os << np.x; } + friend void BestFmt(auto& fmt, const NonPod& np) { fmt.format(np.x); } }; static_assert(!best::relocatable); best::test Shift = [](auto& t) { + // This test doesn't work correctly in opt mode, because it relies on doing + // UAF reads of destroyed values. + // TODO: Make this test sound? + if (!best::is_debug()) return; + unsafe u("test"); int a[] = {1, 2, 3, 4, 5, 6, 7, 8}; diff --git a/best/container/vec.h b/best/container/vec.h index 549d6d7..38edc4e 100644 --- a/best/container/vec.h +++ b/best/container/vec.h @@ -108,8 +108,7 @@ class vec final { /// /// Constructs an empty vector using the given allocator. vec() : vec(alloc{}) {} - explicit vec(alloc alloc) - : size_(0), alloc_(best::in_place, std::move(alloc)) {} + explicit vec(alloc alloc) : alloc_(best::in_place, std::move(alloc)) {} /// # `vec::vec(range)` /// @@ -598,7 +597,7 @@ class vec final { void splice_within(size_t idx, size_t start, size_t count); best::option> on_heap() const { - if (best::to_signed(size_) < 0) { + if (best::to_signed(load_size()) < 0) { return raw_; } return best::none; @@ -641,9 +640,18 @@ class vec final { // How big the area where inlined values live is, including the size. size_t inlined_region_size() const { - return reinterpret_cast(&size_) - + return reinterpret_cast( + &size_do_not_use_directly_or_else_ub_) - reinterpret_cast(this) + best::size_of; } + size_t load_size() const { + size_t size; + std::memcpy(&size, &size_do_not_use_directly_or_else_ub_, sizeof(size)); + return size; + } + void store_size(size_t size) { + std::memcpy(&size_do_not_use_directly_or_else_ub_, &size, sizeof(size)); + } static constexpr size_t InternalAlignment = max_inline > 0 && best::align_of > best::align_of> @@ -652,7 +660,9 @@ class vec final { alignas(InternalAlignment) best::span raw_; [[no_unique_address]] padding padding_; - ssize_t size_; // Signed so we get "reasonable" debugging prints in gdb. + ssize_t // Signed so we get "reasonable" + // debugging prints in gdb. + size_do_not_use_directly_or_else_ub_ = 0; [[no_unique_address]] best::object alloc_; }; @@ -703,7 +713,7 @@ void vec::move_construct(vec&& that, bool assign) { // construct_at because raw_ may contain garbage. std::construct_at(&raw_, *heap); - size_ = ~that.size(); // Need to explicitly flip to on-heap mode here. + store_size(~that.size()); // Need to explicitly flip to on-heap mode here. } else if (best::relocatable) { destroy(); @@ -729,7 +739,7 @@ void vec::move_construct(vec&& that, bool assign) { alloc_ = std::move(that.alloc_); } - that.size_ = 0; // This resets `that` to being empty and inlined. + that.store_size(0); // This resets `that` to being empty and inlined. } template @@ -739,7 +749,7 @@ void vec::destroy() { alloc_->dealloc(heap->data(), layout::array(capacity())); } - size_ = 0; // This returns `this` to being empty and inlined. + store_size(0); // This returns `this` to being empty and inlined. } template @@ -760,11 +770,12 @@ best::object_ptr vec::data() { template size_t vec::size() const { + auto size = load_size(); if constexpr (max_inline == 0) { // size_ is implicitly always zero if off-heap. - return on_heap() ? ~size_ : 0; + return on_heap() ? ~size : 0; } else { - return on_heap() ? ~size_ : size_ >> (bits_of - SizeBytes * 8); + return on_heap() ? ~size : size >> (bits_of - SizeBytes * 8); } } template @@ -775,7 +786,7 @@ void vec::set_size(unsafe, size_t new_size) { } if (on_heap()) { - size_ = ~new_size; + store_size(~new_size); } else { if constexpr (max_inline == 0) { // size_ is implicitly always zero if off-heap, so we don't have to do @@ -784,9 +795,16 @@ void vec::set_size(unsafe, size_t new_size) { auto offset = new_size << (bits_of - SizeBytes * 8); auto mask = ~size_t{0} << (bits_of - SizeBytes * 8); - // TODO(mcyoung): This is a strict aliasing violation... - size_ &= ~mask; - size_ |= offset; + // XXX: we cannot touch size_ directly; it may have been overwritten + // partly via the data() pointer, and thus reading through size_ directly + // is a strict aliasing violation. + // + // There are tests that will fail with optimizations turned on if this we + // are not careful here. + size_t size = load_size(); + size &= ~mask; + size |= offset; + store_size(size); } } } @@ -990,7 +1008,7 @@ void vec::spill_to_heap(best::option capacity_hint) { // calling its operator= is UB. std::construct_at(&raw_, new_data, new_size); - size_ = ~old_size; // Update the size to the "on heap" form. + store_size(~old_size); // Update the size to the "on heap" form. } } // namespace best diff --git a/best/func/fnref_test.cc b/best/func/fnref_test.cc index c8d3c48..3643b5e 100644 --- a/best/func/fnref_test.cc +++ b/best/func/fnref_test.cc @@ -30,11 +30,17 @@ best::test FromFnptr = [](auto& t) { f = nullptr; t.expect_eq(f, nullptr); + + f = [](int x) { return x - 42; }; + t.expect_eq(f(8), -34); }; best::test FromLambda = [](auto& t) { int total = 0; - best::fnref f = [&](int x) { return total += x; }; + auto f0 = [&](int x) { return total += x; }; + best::fnref f = f0; // Need to hoist this, since fnref + // doesn't trigger RLE. Only causes a + // failure in opt mode. t.expect_eq(f(5), 5); t.expect_eq(total, 5); diff --git a/best/func/internal/fnref.h b/best/func/internal/fnref.h index e7b4ca0..ac0ef95 100644 --- a/best/func/internal/fnref.h +++ b/best/func/internal/fnref.h @@ -26,6 +26,11 @@ #include "best/meta/taxonomy.h" namespace best::fnref_internal { +template +concept no_captures = requires(F f) { + { +f } -> best::is_func_ptr; +}; + template class impl { public: @@ -37,7 +42,8 @@ class impl { constexpr impl(R (*fn)(Args...)) : ptr_(nullptr), fnptr_(fn) {} constexpr impl(const auto& fn) - requires best::callable + requires best::callable && + (!no_captures) : ptr_(best::addr(fn)), lambda_(+[](const void* captures, Args... args) -> R { if constexpr (best::is_void) { @@ -52,8 +58,7 @@ class impl { }) {} constexpr impl(best::callable auto&& fn) - requires best::callable && - (!best::is_const_func) + requires(!best::is_const_func) && (!no_captures) : ptr_(best::addr(fn)), lambda_(+[](const void* captures, Args... args) -> R { if constexpr (best::is_void) { @@ -67,6 +72,10 @@ class impl { } }) {} + constexpr impl(best::callable auto&& fn) + requires no_captures + : impl(+fn) {} + constexpr R operator()(Args... args) const { if (ptr_) { return lambda_(ptr_, BEST_FWD(args)...); diff --git a/best/meta/taxonomy.h b/best/meta/taxonomy.h index 7020d1f..285d7c3 100644 --- a/best/meta/taxonomy.h +++ b/best/meta/taxonomy.h @@ -232,6 +232,13 @@ concept is_ptr = std::is_pointer_v; template concept is_member_ptr = std::is_member_pointer_v; +/// # `best::is_func_ptr` +/// +/// Whether this is a function pointer type. +template +concept is_func_ptr = + best::is_ptr && best::is_func>; + /// # `best::as_ptr` /// /// If `T` is an object, void, or tame function type, returns a pointer to it.