Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 5, 2023
+30 −2
Merged

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
23e6671
[libc++] Use correct size for deallocation of arrays in shared_ptr
ilya-biryukov Oct 4, 2023
000d933
fixup! [libc++] Use correct size for deallocation of arrays in shared…
ilya-biryukov Oct 4, 2023
7e79214
fixup! [libc++] Use correct size for deallocation of arrays in shared…
ilya-biryukov Oct 5, 2023
049fa20
fixup! [libc++] Use correct size for deallocation of arrays in shared…
ilya-biryukov Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions libcxx/include/__memory/shared_ptr.h
View file
Original file line number Diff line number Diff line change
@@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

_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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

_LIBCPP_NO_UNIQUE_ADDRESS _Alloc __alloc_;
27 changes: 27 additions & 0 deletions libcxx/test/libcxx/memory/shared_ptr_array.pass.cpp
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//===----------------------------------------------------------------------===//
//
// 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(int, char**) {
std::allocate_shared<int[]>(std::allocator<int>{}, 10);
std::make_shared<int[]>(10);

std::allocate_shared<int[10]>(std::allocator<int>{});
std::make_shared<int[10]>();

return 0;
}