util: Don't derive secure_allocator from std::allocator #27930

CaseyCarter · 2023-06-22T00:18:12Z

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.)

DrahtBot · 2023-06-22T00:18:15Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	john-moffett, jonatack, achow101
Concept ACK	jamesob

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

src/support/allocators/secure.h

dergoegge · 2023-06-22T10:04:00Z

Thanks for the PR and raising the issue!

I suspect that it'll be a long time before binaries using C++23 would be shipped but this brings up a wider issue: like you say "giving the C++ Standard Committee control of the public interface" might not be a great idea.

I'm guessing we should consider doing the same for zero_after_free_allocator? Even though it doesn't seem to be affected by this particular issue.

From microsoft/STL#3819:

microsoft/STL#3712 broke BitCoin in Microsoft's internal Real World Code (RWC) test suite.

I'd be curious to know how this was actually caught? Compiling with c++23 and then running our tests with/without sanitizers?

@theuni @ryanofsky any thoughts?

CaseyCarter · 2023-06-22T16:35:54Z

I'm guessing we should consider doing the same for zero_after_free_allocator? Even though it doesn't seem to be affected by this particular issue.

That's probably a good idea, yes.

From microsoft/STL#3819:

microsoft/STL#3712 broke BitCoin in Microsoft's internal Real World Code (RWC) test suite.

I'd be curious to know how this was actually caught? Compiling with c++23 and then running our tests with/without sanitizers?

Yes, we (MSVC) have a suite of a few dozen open-source projects that we use to validate by building and testing them in various configurations. One of your tests using this allocator with std::vector crashed (unsurprisingly) trying to deallocate memory.

src/support/allocators/secure.h

maflcko · 2023-06-23T07:25:38Z

Please keep your commits squashed according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits. This makes review easier, because reviewers will only need to look at one commit, and also is history cleaner because there will be less churn. (Note that we don't squash on merge, so a squash will need to be done before review, before merge)

CaseyCarter · 2023-06-23T14:40:35Z

Please keep your commits squashed

Squashed and rebased onto current master.

maflcko · 2023-06-28T11:17:15Z

I'm guessing we should consider doing the same for zero_after_free_allocator? Even though it doesn't seem to be affected by this particular issue.

That's probably a good idea, yes.

Looks like this wasn't addressed yet?

jamesob · 2023-06-28T16:52:01Z

Concept ACK. Thanks for filing the change @CaseyCarter, and welcome to the project!

john-moffett

Concept ACK and existing code ACK. Thanks for catching this!

I agree that zero_after_free_allocator also ought to be modified. FWIW, here's what I did:

diff --git a/src/support/allocators/zeroafterfree.h b/src/support/allocators/zeroafterfree.h
index 2dc644c242a..35312e9704d 100644
--- a/src/support/allocators/zeroafterfree.h
+++ b/src/support/allocators/zeroafterfree.h
@@ -12,31 +12,46 @@
 #include <vector>
 
 template <typename T>
-struct zero_after_free_allocator : public std::allocator<T> {
-    using base = std::allocator<T>;
-    using traits = std::allocator_traits<base>;
-    using size_type = typename traits::size_type;
-    using difference_type = typename traits::difference_type;
-    using pointer = typename traits::pointer;
-    using const_pointer = typename traits::const_pointer;
-    using value_type = typename traits::value_type;
-    zero_after_free_allocator() noexcept {}
-    zero_after_free_allocator(const zero_after_free_allocator& a) noexcept : base(a) {}
+struct zero_after_free_allocator {
+    using size_type = std::size_t;
+    using difference_type = std::ptrdiff_t;
+    using pointer = T*;
+    using const_pointer = const T*;
+    using value_type = T;
+
+    zero_after_free_allocator() noexcept {};
+    zero_after_free_allocator(const zero_after_free_allocator& a) noexcept {};
     template <typename U>
-    zero_after_free_allocator(const zero_after_free_allocator<U>& a) noexcept : base(a)
-    {
-    }
-    ~zero_after_free_allocator() noexcept {}
+    zero_after_free_allocator(const zero_after_free_allocator<U>&) noexcept {};
+    ~zero_after_free_allocator() noexcept {};
     template <typename Other>
     struct rebind {
         typedef zero_after_free_allocator<Other> other;
     };
 
+    T* allocate(std::size_t n, const void* hint = nullptr)
+    {
+        return static_cast<pointer>(::operator new(n * sizeof(T)));
+    }
+
     void deallocate(T* p, std::size_t n)
     {
-        if (p != nullptr)
+        if (p != nullptr) {
             memory_cleanse(p, sizeof(T) * n);
-        std::allocator<T>::deallocate(p, n);
+        }
+        ::operator delete(p);
+    }
+
+    template <typename U>
+    friend bool operator==(const zero_after_free_allocator&, const zero_after_free_allocator<U>&) noexcept
+    {
+        return true;
+    }
+
+    template <typename U>
+    friend bool operator!=(const zero_after_free_allocator&, const zero_after_free_allocator<U>&) noexcept
+    {
+        return false;
     }
 };

fanquake · 2023-07-21T15:30:14Z

@CaseyCarter did you also want to pickup the change to zero_after_free_allocator ?
Otherwise, we can have someone cherry-pick you're commit into a new PR, and make the additional changes.

CaseyCarter · 2023-07-21T17:38:32Z

I've made consistent changes to zero_after_free_allocator, and also performed some "aggressive cleanup" of cruft in both allocators that's unnecessary (as far as the Standard Library is concerned) after C++11. The CI will let me know if bitcoin itself was using any of that "cruft". If I have to fix any fallout, I'll learn how to build locally and stop lazily relying on CI ;)

jonatack

ACK b79d956 review and debug built/tested each commit locally

Note that per https://en.cppreference.com/w/cpp/memory/allocator and related pages, many of the member types and functions are changing with C++20. These can be revisited for updates when this codebase migrates to C++20 or later.

Affects both secure_allocator and zero_after_free_allocator. 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`. Drive-by: Aggressively remove facilities unnecessary since C++11 from both allocators to keep things simple.

CaseyCarter · 2023-07-25T05:36:17Z

Apologies for forgetting to keep the PR squashed. I've merged the two commits into one.

john-moffett

ACK 07c59ed Reviewed and tested. Performance appears unaffected in my environment.

jonatack · 2023-07-25T18:54:16Z

re-ACK 07c59ed no change since my previous ACK apart from squashing the commits

achow101 · 2023-07-25T22:44:07Z

ACK 07c59ed

…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

DrahtBot added CI failed and removed CI failed labels Jun 22, 2023

DrahtBot changed the title ~~Don't derive secure_allocator from std::allocator~~ util: Don't derive secure_allocator from std::allocator Jun 22, 2023

DrahtBot added the Utils/log/libs label Jun 22, 2023

frederick-vs-ja reviewed Jun 22, 2023

View reviewed changes

src/support/allocators/secure.h Outdated Show resolved Hide resolved

achow101 reviewed Jun 22, 2023

View reviewed changes

src/support/allocators/secure.h Outdated Show resolved Hide resolved

DrahtBot added CI failed and removed CI failed labels Jun 22, 2023

CaseyCarter force-pushed the master branch from 050df2a to 6dcf95b Compare June 23, 2023 14:33

github-actions bot mentioned this pull request Jun 24, 2023

2023-06-23 Hot Posts jiacai2050/mofish#323

Open

john-moffett reviewed Jun 28, 2023

View reviewed changes

fanquake added this to the 26.0 milestone Jul 21, 2023

jonatack reviewed Jul 21, 2023

View reviewed changes

CaseyCarter force-pushed the master branch from b79d956 to 07c59ed Compare July 25, 2023 05:35

DrahtBot added the CI failed label Jul 25, 2023

fanquake requested review from john-moffett, jamesob and darosior July 25, 2023 08:55

DrahtBot removed the CI failed label Jul 25, 2023

john-moffett approved these changes Jul 25, 2023

View reviewed changes

DrahtBot requested a review from jonatack July 25, 2023 18:09

DrahtBot removed the request for review from jonatack July 25, 2023 18:54

achow101 merged commit 32c1523 into bitcoin:master Jul 25, 2023

theuni mentioned this pull request Nov 16, 2023

refactor: Remove redundant checks in compat/assumptions.h #28579

Merged

bitcoin locked and limited conversation to collaborators Jul 24, 2024

util: Don't derive secure_allocator from std::allocator #27930

util: Don't derive secure_allocator from std::allocator #27930

Uh oh!

Conversation

CaseyCarter commented Jun 22, 2023

Uh oh!

DrahtBot commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

Uh oh!

dergoegge commented Jun 22, 2023

Uh oh!

CaseyCarter commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CaseyCarter commented Jun 23, 2023

Uh oh!

maflcko commented Jun 28, 2023

Uh oh!

jamesob commented Jun 28, 2023

Uh oh!

john-moffett left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Jul 21, 2023

Uh oh!

CaseyCarter commented Jul 21, 2023

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CaseyCarter commented Jul 25, 2023

Uh oh!

john-moffett left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Jul 25, 2023

Uh oh!

achow101 commented Jul 25, 2023

Uh oh!

Uh oh!

DrahtBot commented Jun 22, 2023 •

edited

Loading

CaseyCarter commented Jun 22, 2023 •

edited

Loading

maflcko commented Jun 23, 2023 •

edited

Loading

jonatack left a comment •

edited

Loading