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.)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
54+ return true;
55+ }
56+ template <typename U>
57+ friend bool operator!=(const secure_allocator&, const secure_allocator<U>&) noexcept
58+ {
59+ return true;
This look likes a copy-pasta.
0 return false;
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 https://github.com/microsoft/STL/pull/3819:
https://github.com/microsoft/STL/pull/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?
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.
28@@ -29,7 +29,7 @@ struct secure_allocator {
29 template <typename Other>
30 struct rebind {
31 typedef secure_allocator<Other> other;
32- };
33+ };g
Please keep your commits squashed
Squashed and rebased onto current master.
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?
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:
0diff --git a/src/support/allocators/zeroafterfree.h b/src/support/allocators/zeroafterfree.h
1index 2dc644c242a..35312e9704d 100644
2--- a/src/support/allocators/zeroafterfree.h
3+++ b/src/support/allocators/zeroafterfree.h
4@@ -12,31 +12,46 @@
5 #include <vector>
6
7 template <typename T>
8-struct zero_after_free_allocator : public std::allocator<T> {
9- using base = std::allocator<T>;
10- using traits = std::allocator_traits<base>;
11- using size_type = typename traits::size_type;
12- using difference_type = typename traits::difference_type;
13- using pointer = typename traits::pointer;
14- using const_pointer = typename traits::const_pointer;
15- using value_type = typename traits::value_type;
16- zero_after_free_allocator() noexcept {}
17- zero_after_free_allocator(const zero_after_free_allocator& a) noexcept : base(a) {}
18+struct zero_after_free_allocator {
19+ using size_type = std::size_t;
20+ using difference_type = std::ptrdiff_t;
21+ using pointer = T*;
22+ using const_pointer = const T*;
23+ using value_type = T;
24+
25+ zero_after_free_allocator() noexcept {};
26+ zero_after_free_allocator(const zero_after_free_allocator& a) noexcept {};
27 template <typename U>
28- zero_after_free_allocator(const zero_after_free_allocator<U>& a) noexcept : base(a)
29- {
30- }
31- ~zero_after_free_allocator() noexcept {}
32+ zero_after_free_allocator(const zero_after_free_allocator<U>&) noexcept {};
33+ ~zero_after_free_allocator() noexcept {};
34 template <typename Other>
35 struct rebind {
36 typedef zero_after_free_allocator<Other> other;
37 };
38
39+ T* allocate(std::size_t n, const void* hint = nullptr)
40+ {
41+ return static_cast<pointer>(::operator new(n * sizeof(T)));
42+ }
43+
44 void deallocate(T* p, std::size_t n)
45 {
46- if (p != nullptr)
47+ if (p != nullptr) {
48 memory_cleanse(p, sizeof(T) * n);
49- std::allocator<T>::deallocate(p, n);
50+ }
51+ ::operator delete(p);
52+ }
53+
54+ template <typename U>
55+ friend bool operator==(const zero_after_free_allocator&, const zero_after_free_allocator<U>&) noexcept
56+ {
57+ return true;
58+ }
59+
60+ template <typename U>
61+ friend bool operator!=(const zero_after_free_allocator&, const zero_after_free_allocator<U>&) noexcept
62+ {
63+ return false;
64 }
65 };
zero_after_free_allocator ?
Otherwise, we can have someone cherry-pick you’re commit into a new PR, and make the additional changes.
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 ;)
ACK b79d9563169cc990ff2da9a44fef67205e907a8b 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.
Labels
Utils/log/libs
Milestone
26.0
