Deallocate at least - #3819
Merged
Stephan T. Lavavej (StephanTLavavej) merged 3 commits intoJun 23, 2023
Merged
Conversation
This reverts commit e37227e. This broke BitCoin in Microsoft's internal Real World Code (RWC) test suite. They publicy derive an allocator from `std::allocate`, implementing `allocate` and `deallocate` but not `allocate_at_least` (https://github.com/bitcoin/bitcoin/blob/f1b4975461364d5d40d2bfafc6b165dd5d7eec30/src/support/allocators/secure.h#L19-L56). When `vector` allocates memory with `std::allocator::allocate_at_least` and tries to free it with `secure_allocator::deallocate`, terrible things happen. We suspect this pattern is widespread, so we're reverting the change for now.
Stephan T. Lavavej (StephanTLavavej)
approved these changes
Jun 22, 2023
Member
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Member
|
I've pushed a merge with |
Member
|
Thanks for finding the root cause of this major regression and fixing it so fast! 🌲 🎯 🛠️ |
Merged
Ava Chow (achow101)
added a commit
to bitcoin-core/gui
that referenced
this pull request
Jul 25, 2023
… std::allocator 07c59ed Don't derive secure_allocator from std::allocator (Casey Carter) Pull request description: Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`. (Discovered by microsoft/STL#3712, which will be reverted by microsoft/STL#3819 before it ships.) ACKs for top commit: jonatack: re-ACK 07c59ed no change since my previous ACK apart from squashing the commits achow101: ACK 07c59ed john-moffett: ACK 07c59ed Reviewed and tested. Performance appears unaffected in my environment. Tree-SHA512: 23606c40414d325f5605a9244d4dd50907fdf5f2fbf70f336accb3a2cb98baa8acd2972f46eab1b7fdec1d28a843a96b06083cd2d09791cda7c90ee218e5bbd5
jagdeep sidhu (sidhujag)
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 9, 2023
…locator 07c59ed Don't derive secure_allocator from std::allocator (Casey Carter) Pull request description: Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`. (Discovered by microsoft/STL#3712, which will be reverted by microsoft/STL#3819 before it ships.) ACKs for top commit: jonatack: re-ACK 07c59ed no change since my previous ACK apart from squashing the commits achow101: ACK 07c59ed john-moffett: ACK 07c59ed Reviewed and tested. Performance appears unaffected in my environment. Tree-SHA512: 23606c40414d325f5605a9244d4dd50907fdf5f2fbf70f336accb3a2cb98baa8acd2972f46eab1b7fdec1d28a843a96b06083cd2d09791cda7c90ee218e5bbd5
Vijay (vijaydasmp)
pushed a commit
to vijaydasmp/dash
that referenced
this pull request
May 29, 2025
…locator 07c59ed Don't derive secure_allocator from std::allocator (Casey Carter) Pull request description: Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`. (Discovered by microsoft/STL#3712, which will be reverted by microsoft/STL#3819 before it ships.) ACKs for top commit: jonatack: re-ACK 07c59ed no change since my previous ACK apart from squashing the commits achow101: ACK 07c59ed john-moffett: ACK 07c59ed Reviewed and tested. Performance appears unaffected in my environment. Tree-SHA512: 23606c40414d325f5605a9244d4dd50907fdf5f2fbf70f336accb3a2cb98baa8acd2972f46eab1b7fdec1d28a843a96b06083cd2d09791cda7c90ee218e5bbd5
Joaquín Antonio De Vicente López (Kino1994)
pushed a commit
to Kino1994/bitcoin-full-history
that referenced
this pull request
Jun 28, 2026
… std::allocator 19632ea Don't derive secure_allocator from std::allocator (Casey Carter) Pull request description: Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`. (Discovered by microsoft/STL#3712, which will be reverted by microsoft/STL#3819 before it ships.) ACKs for top commit: jonatack: re-ACK 19632ea no change since my previous ACK apart from squashing the commits achow101: ACK 19632ea john-moffett: ACK 19632ea Reviewed and tested. Performance appears unaffected in my environment. Tree-SHA512: 23606c40414d325f5605a9244d4dd50907fdf5f2fbf70f336accb3a2cb98baa8acd2972f46eab1b7fdec1d28a843a96b06083cd2d09791cda7c90ee218e5bbd5
BigcoinBGC
pushed a commit
to BigcoinBGC/bigcoin
that referenced
this pull request
Jun 30, 2026
… std::allocator 25830e2 Don't derive secure_allocator from std::allocator (Casey Carter) Pull request description: Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`. (Discovered by microsoft/STL#3712, which will be reverted by microsoft/STL#3819 before it ships.) ACKs for top commit: jonatack: re-ACK 25830e2 no change since my previous ACK apart from squashing the commits achow101: ACK 25830e2 john-moffett: ACK 25830e2 Reviewed and tested. Performance appears unaffected in my environment. Tree-SHA512: 23606c40414d325f5605a9244d4dd50907fdf5f2fbf70f336accb3a2cb98baa8acd2972f46eab1b7fdec1d28a843a96b06083cd2d09791cda7c90ee218e5bbd5
Joaquín Antonio De Vicente López (Kino1994)
pushed a commit
to Kino1994/bitcoin-full-history
that referenced
this pull request
Aug 19, 2026
… std::allocator 07c59eda00841aafaafd8fd648217b56b1e907c9 Don't derive secure_allocator from std::allocator (Casey Carter) Pull request description: Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`. (Discovered by microsoft/STL#3712, which will be reverted by microsoft/STL#3819 before it ships.) ACKs for top commit: jonatack: re-ACK 07c59eda00841aafaafd8fd648217b56b1e907c9 no change since my previous ACK apart from squashing the commits achow101: ACK 07c59eda00841aafaafd8fd648217b56b1e907c9 john-moffett: ACK 07c59eda00841aafaafd8fd648217b56b1e907c9 Reviewed and tested. Performance appears unaffected in my environment. Tree-SHA512: 23606c40414d325f5605a9244d4dd50907fdf5f2fbf70f336accb3a2cb98baa8acd2972f46eab1b7fdec1d28a843a96b06083cd2d09791cda7c90ee218e5bbd5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#3712 broke BitCoin in Microsoft's internal Real World Code (RWC) test suite. They publicly derive an allocator from
std::allocate, implementingallocateanddeallocatebut notallocate_at_least(https://github.com/bitcoin/bitcoin/blob/f1b4975461364d5d40d2bfafc6b165dd5d7eec30/src/support/allocators/secure.h#L19-L56).
When
vectorallocates memory withstd::allocator::allocate_at_leastand tries to free it withsecure_allocator::deallocate, terrible things happen.We suspect this pattern is widespread, so we're reverting the change for now.
There's a second commit here to reapply the
basic_stringdrive-by fix which we don't want to revert. (We want to merge these commits separately to make it easier to revert the revert in the future.)Fixes VSO-1839141 / AB#1839141.