From ef53e0a0d28d02743eae27f6dd99c6da4a677cbd Mon Sep 17 00:00:00 2001 From: Omer Katz Date: Thu, 3 Feb 2022 10:33:39 +0100 Subject: [PATCH] cppgc: Prevent misuse of explicit_management.h The methods in explicit_management.h should be called via the public variants in the subtle namespace. Calling the variants in the internal namespace directly skips asserts and required size coversions. Doing so may cause misuse of the api that may break GC inernals Change-Id: I58a0f324ca1ee0839bb85eb9b53ce57785dc7b91 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3435187 Reviewed-by: Michael Lippautz Commit-Queue: Omer Katz Cr-Commit-Position: refs/heads/main@{#78920} --- include/cppgc/explicit-management.h | 26 ++++++++++++++++--- src/heap/cppgc/explicit-management.cc | 5 ++-- .../heap/cppgc-js/unified-heap-unittest.cc | 2 +- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/include/cppgc/explicit-management.h b/include/cppgc/explicit-management.h index cdb6af4858..0290328dcc 100644 --- a/include/cppgc/explicit-management.h +++ b/include/cppgc/explicit-management.h @@ -15,11 +15,27 @@ namespace cppgc { class HeapHandle; +namespace subtle { + +template +void FreeUnreferencedObject(HeapHandle& heap_handle, T& object); +template +bool Resize(T& object, AdditionalBytes additional_bytes); + +} // namespace subtle + namespace internal { -V8_EXPORT void FreeUnreferencedObject(HeapHandle&, void*); -V8_EXPORT bool Resize(void*, size_t); +class ExplicitManagementImpl final { + private: + V8_EXPORT static void FreeUnreferencedObject(HeapHandle&, void*); + V8_EXPORT static bool Resize(void*, size_t); + template + friend void subtle::FreeUnreferencedObject(HeapHandle&, T&); + template + friend bool subtle::Resize(T&, AdditionalBytes); +}; } // namespace internal namespace subtle { @@ -45,7 +61,8 @@ template void FreeUnreferencedObject(HeapHandle& heap_handle, T& object) { static_assert(IsGarbageCollectedTypeV, "Object must be of type GarbageCollected."); - internal::FreeUnreferencedObject(heap_handle, &object); + internal::ExplicitManagementImpl::FreeUnreferencedObject(heap_handle, + &object); } /** @@ -73,7 +90,8 @@ template bool Resize(T& object, AdditionalBytes additional_bytes) { static_assert(IsGarbageCollectedTypeV, "Object must be of type GarbageCollected."); - return internal::Resize(&object, sizeof(T) + additional_bytes.value); + return internal::ExplicitManagementImpl::Resize( + &object, sizeof(T) + additional_bytes.value); } } // namespace subtle diff --git a/src/heap/cppgc/explicit-management.cc b/src/heap/cppgc/explicit-management.cc index dd77fe7ea9..01226ac192 100644 --- a/src/heap/cppgc/explicit-management.cc +++ b/src/heap/cppgc/explicit-management.cc @@ -47,7 +47,8 @@ void InvalidateRememberedSlots(HeapBase& heap, void* begin, void* end) { } // namespace -void FreeUnreferencedObject(HeapHandle& heap_handle, void* object) { +void ExplicitManagementImpl::FreeUnreferencedObject(HeapHandle& heap_handle, + void* object) { if (InGC(heap_handle)) { return; } @@ -144,7 +145,7 @@ bool Shrink(HeapObjectHeader& header, BasePage& base_page, size_t new_size, } // namespace -bool Resize(void* object, size_t new_object_size) { +bool ExplicitManagementImpl::Resize(void* object, size_t new_object_size) { // `object` is guaranteed to be of type GarbageCollected, so getting the // BasePage is okay for regular and large objects. BasePage* base_page = BasePage::FromPayload(object); diff --git a/test/unittests/heap/cppgc-js/unified-heap-unittest.cc b/test/unittests/heap/cppgc-js/unified-heap-unittest.cc index 736ddb9568..c34d36d17a 100644 --- a/test/unittests/heap/cppgc-js/unified-heap-unittest.cc +++ b/test/unittests/heap/cppgc-js/unified-heap-unittest.cc @@ -113,7 +113,7 @@ TEST_F(UnifiedHeapTest, FreeUnreferencedDuringNoGcScope) { cpp_heap().stats_collector()->NotifySafePointForTesting(); { cppgc::subtle::NoGarbageCollectionScope no_gc_scope(cpp_heap()); - cppgc::internal::FreeUnreferencedObject(cpp_heap(), unreferenced); + cppgc::subtle::FreeUnreferencedObject(cpp_heap(), *unreferenced); // Force safepoint to make sure allocated size decrease due to freeing // unreferenced object is reported to CppHeap. Due to // NoGarbageCollectionScope, CppHeap will cache the reported decrease and