[libc++] Use correct size for deallocation of arrays in shared_ptr #68233

ilya-biryukov · 2023-10-04T16:45:27Z

Current implementation passes the number of _AlignedStorage objects when it calls to allocate and the number of bytes on deallocate. This only applies to allocations that allocate control block and the storage together, i.e. make_shared and allocate_shared.

Found by ASan.

Fixes llvm#68051. Current implementation passes the number of `_AlignedStorage` objects when it calls to `allocate` and the number of **bytes** on deallocate. This only applies to allocations that allocate control block and the storage together, i.e. `make_shared` and `allocate_shared`. Found by ASan.

llvmbot · 2023-10-04T16:46:15Z

@llvm/pr-subscribers-libcxx

Changes

Fixes #68051.

Current implementation passes the number of _AlignedStorage objects when it calls to allocate and the number of bytes on deallocate. This only applies to allocations that allocate control block and the storage together, i.e. make_shared and allocate_shared.

Found by ASan.

Full diff: https://github.com/llvm/llvm-project/pull/68233.diff

2 Files Affected:

(modified) libcxx/include/__memory/shared_ptr.h (+3-2)
(added) libcxx/test/libcxx/memory/shared_ptr_array.pass.cpp (+25)

diff --git a/libcxx/include/__memory/shared_ptr.h b/libcxx/include/__memory/shared_ptr.h
index 33a1b95a31ddbd5..d9ddb8a17be273f 100644
--- a/libcxx/include/__memory/shared_ptr.h
+++ b/libcxx/include/__memory/shared_ptr.h
@@ -1137,7 +1137,8 @@ struct __unbounded_array_control_block<_Tp[], _Alloc> : __shared_weak_count
         __alloc_.~_Alloc();
         size_t __size = __unbounded_array_control_block::__bytes_for(__count_);
         _AlignedStorage* __storage = reinterpret_cast<_AlignedStorage*>(this);
-        allocator_traits<_StorageAlloc>::deallocate(__tmp, _PointerTraits::pointer_to(*__storage), __size);
+        allocator_traits<_StorageAlloc>::deallocate(
+            __tmp, _PointerTraits::pointer_to(*__storage), __size / sizeof(_AlignedStorage));
     }
 
     _LIBCPP_NO_UNIQUE_ADDRESS _Alloc __alloc_;
@@ -1220,7 +1221,7 @@ struct __bounded_array_control_block<_Tp[_Count], _Alloc>
 
         _ControlBlockAlloc __tmp(__alloc_);
         __alloc_.~_Alloc();
-        allocator_traits<_ControlBlockAlloc>::deallocate(__tmp, _PointerTraits::pointer_to(*this), sizeof(*this));
+        allocator_traits<_ControlBlockAlloc>::deallocate(__tmp, _PointerTraits::pointer_to(*this), 1);
     }
 
     _LIBCPP_NO_UNIQUE_ADDRESS _Alloc __alloc_;
diff --git a/libcxx/test/libcxx/memory/shared_ptr_array.pass.cpp b/libcxx/test/libcxx/memory/shared_ptr_array.pass.cpp
new file mode 100644
index 000000000000000..f885c7344e41b9b
--- /dev/null
+++ b/libcxx/test/libcxx/memory/shared_ptr_array.pass.cpp
@@ -0,0 +1,25 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// REQUIRES: -fsized-deallocation
+// ADDITIONAL_COMPILE_FLAGS: -fsized-deallocation
+
+// This test will fail with ASan if the implementation passes different sizes
+// to corresponding allocation and deallocation functions.
+
+#include <memory>
+
+int main() {
+    std::allocate_shared<int64_t[]>(std::allocator<int64_t>{}, 10);
+    std::make_shared<int64_t[]>(10);
+
+    std::allocate_shared<int64_t[10]>(std::allocator<int64_t>{});
+    std::make_shared<int64_t[10]>();
+}

github-actions · 2023-10-04T16:47:16Z

✅ With the latest revision this PR passed the C/C++ code formatter.

…_ptr Reformat the test file.

EricWF · 2023-10-04T21:00:14Z

I wonder why ASAN wasn't flagging this externally. Any clues?

EricWF · 2023-10-04T21:03:13Z

Could you fix the missing include in the failing test, then I think this is good to go!

Though it would be nice if we could figure out why ASAN wasn't firing here.

EricWF

LGTM after getting the test passing (not your failure, but lets get the CI green).

EricWF · 2023-10-04T20:58:23Z

libcxx/include/__memory/shared_ptr.h

@@ -1220,7 +1221,7 @@ struct __bounded_array_control_block<_Tp[_Count], _Alloc>

        _ControlBlockAlloc __tmp(__alloc_);
        __alloc_.~_Alloc();
-        allocator_traits<_ControlBlockAlloc>::deallocate(__tmp, _PointerTraits::pointer_to(*this), sizeof(*this));
+        allocator_traits<_ControlBlockAlloc>::deallocate(__tmp, _PointerTraits::pointer_to(*this), 1);


Oof, that's a nasty bug. Thanks for the fix.

ldionne

Thanks for the fix! This is indeed a nasty bug, a bit surprising that it went unnoticed for so long.

libcxx/test/libcxx/memory/shared_ptr_array.pass.cpp

ldionne · 2023-10-04T22:26:38Z

libcxx/include/__memory/shared_ptr.h

@@ -1137,7 +1137,8 @@ struct __unbounded_array_control_block<_Tp[], _Alloc> : __shared_weak_count
        __alloc_.~_Alloc();
        size_t __size = __unbounded_array_control_block::__bytes_for(__count_);
        _AlignedStorage* __storage = reinterpret_cast<_AlignedStorage*>(this);
-        allocator_traits<_StorageAlloc>::deallocate(__tmp, _PointerTraits::pointer_to(*__storage), __size);
+        allocator_traits<_StorageAlloc>::deallocate(
+            __tmp, _PointerTraits::pointer_to(*__storage), __size / sizeof(_AlignedStorage));


Is this not always __count_ + 1? i.e. the number of elements in the array + the size of the control block at the beginning? I guess I'm kinda confused with my own code now.

It actually seems to be more complicated.

The aligned storage has the size equal to the alignment of the control block (which is 8 bytes on my machine), so, e.g., we allocate and deallocate 5 elements of AlignedStorage for int[] when __count_ is 0 (as the control block is 40 bytes on my machine).

In any case, I believe that reproducing exactly what gets passed to allocate is the right call here.

If alignof(size_t) < sizeof(size_t), it will not be __count_ + 1.

…_ptr - use full signature in main - return 0

…_ptr use int instead of int64_t to avoid the need for an include

ilya-biryukov · 2023-10-05T13:23:24Z

I wonder why ASAN wasn't flagging this externally. Any clues?

Thanks for the fix! This is indeed a nasty bug, a bit surprising that it went unnoticed for so long.

I think it's because it requires a rather rare combination of flags. First, we need to enable -fsized-deallocation[1] and build with ASan. In addition, one either needs to use std::boyer_moore_searcher in C++17 (which is a likely a rarely used API) or C++20's std::make_shared with array types (which should be more common, but C++20 is relatively rare itself).

[1]: it is surprising that -fsized-deallocation is not the default yet given that it was part of C++14. There was an attempt to make it the default in August 2023, but it got reverted.

ilya-biryukov · 2023-10-05T13:26:53Z

I'm going to merge the change in as all comments are addressed and this already has LGTM from @EricWF.
The only pending build is for FreeBSD and it came out green last time (there were no significant changes afterwards), the change itself seems relatively straightforward.

I hope I don't break any policies or expectations here, but please let me know if that's the case.

jyknight · 2023-10-13T17:12:10Z

Any objection to backporting this to 17.x branch?

ldionne · 2023-10-13T18:07:46Z

No, we should totally do that. Do you want to create the cherry-pick issue?

ilya-biryukov · 2023-11-14T13:51:28Z

Sorry, this slipped off my radar. I've filed #72255.

…68233) Fixes #68051. Current implementation passes the number of `_AlignedStorage` objects when it calls to `allocate` and the number of **bytes** on `deallocate`. This only applies to allocations that allocate control block and the storage together, i.e. `make_shared` and `allocate_shared`. Found by ASan under Clang combined with `-fsized-deallocation`. (cherry picked from commit f722db0)

ilya-biryukov added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 4, 2023

ilya-biryukov requested a review from a team as a code owner October 4, 2023 16:45

ilya-biryukov self-assigned this Oct 4, 2023

fixup! [libc++] Use correct size for deallocation of arrays in shared…

000d933

…_ptr Reformat the test file.

EricWF approved these changes Oct 4, 2023

View reviewed changes

ldionne reviewed Oct 4, 2023

View reviewed changes

ilya-biryukov added 2 commits October 5, 2023 10:54

fixup! [libc++] Use correct size for deallocation of arrays in shared…

7e79214

…_ptr - use full signature in main - return 0

fixup! [libc++] Use correct size for deallocation of arrays in shared…

049fa20

…_ptr use int instead of int64_t to avoid the need for an include

ilya-biryukov merged commit f722db0 into llvm:main Oct 5, 2023

ilya-biryukov deleted the sharedptr branch October 5, 2023 13:28

stepthomas mentioned this pull request Oct 10, 2023

AMDGPU stepthomas atomic csub no rtn forms ver2 stepthomas/llvm-project#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Use correct size for deallocation of arrays in shared_ptr #68233

[libc++] Use correct size for deallocation of arrays in shared_ptr #68233

ilya-biryukov commented Oct 4, 2023

llvmbot commented Oct 4, 2023

github-actions bot commented Oct 4, 2023 •

edited

Loading

EricWF commented Oct 4, 2023

EricWF commented Oct 4, 2023

EricWF left a comment

EricWF Oct 4, 2023

ldionne left a comment

ldionne Oct 4, 2023

ilya-biryukov Oct 5, 2023

sxeraverx Oct 5, 2023

ilya-biryukov commented Oct 5, 2023

ilya-biryukov commented Oct 5, 2023

jyknight commented Oct 13, 2023

ldionne commented Oct 13, 2023

ilya-biryukov commented Nov 14, 2023

[libc++] Use correct size for deallocation of arrays in shared_ptr #68233

[libc++] Use correct size for deallocation of arrays in shared_ptr #68233

Conversation

ilya-biryukov commented Oct 4, 2023

llvmbot commented Oct 4, 2023

github-actions bot commented Oct 4, 2023 • edited Loading

EricWF commented Oct 4, 2023

EricWF commented Oct 4, 2023

EricWF left a comment

Choose a reason for hiding this comment

EricWF Oct 4, 2023

Choose a reason for hiding this comment

ldionne left a comment

Choose a reason for hiding this comment

ldionne Oct 4, 2023

Choose a reason for hiding this comment

ilya-biryukov Oct 5, 2023

Choose a reason for hiding this comment

sxeraverx Oct 5, 2023

Choose a reason for hiding this comment

ilya-biryukov commented Oct 5, 2023

ilya-biryukov commented Oct 5, 2023

jyknight commented Oct 13, 2023

ldionne commented Oct 13, 2023

ilya-biryukov commented Nov 14, 2023

github-actions bot commented Oct 4, 2023 •

edited

Loading