ProxyClientBase: avoid static_cast to partially destructed object #127
Conversation
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
This is a bugfix that should fix the UBSan failure reported bitcoin-core#125 In practice there is no real bug here, just like there was no real bug fixed in the recent "ProxyClientBase: avoid static_cast to partially constructed object" change in bitcoin-core#121. This is because in practice, ProxyClient subclasses only inherit ProxyClientBase state and do not define any state of their own, so there is nothing bad that could actually happen from static_cast-ing a ProxyClientBase pointer to a ProxyClient pointer inside the ProxyClientBase constructor or destructor. However, using static_cast this way technically triggers undefined behavior, and that causes UBSan to fail, so this commit drops the static_cast by making ProxyClient::construct() and ProxyClient::destroy() methods into static methods taking a ProxyClientBase& argument instead of instance methods using *this. Fixes bitcoin-core#125
TheCharlatan
approved these changes
Jan 16, 2025
Yeah I really did not like the previous fix in #126, but this fix is not too bad. The thing I still don't like about this fix is that it makes construct/destroy different than other methods, instead of treating them the same as other methods but just calling them automatically on the client side. But unfortunately there doesn't seem to be a good way to call construct/destroy automatically from with ProxyClientBase class without using static_cast or turning them into static methods. I think c++23 with http://wg21.link/P0847 might be able to help with this in the future, but i'm not sure. |
Sjors
added a commit
to Sjors/bitcoin
that referenced
this pull request
Jan 16, 2025
Add detection for int/enum size mismatch and recent fixes for undefined behavior. bitcoin-core/libmultiprocess#121: ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120: proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127: ProxyClientBase: avoid static_cast to partially destructed object
Sjors
added a commit
to Sjors/bitcoin
that referenced
this pull request
Jan 16, 2025
Add detection for int/enum size mismatch and recent fixes for undefined behavior. bitcoin-core/libmultiprocess#121: ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120: proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127: ProxyClientBase: avoid static_cast to partially destructed object
33 tasks
Sjors
added a commit
to Sjors/bitcoin
that referenced
this pull request
Jan 17, 2025
Add detection for int/enum size mismatch and recent fixes for undefined behavior. bitcoin-core/libmultiprocess#121: ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120: proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127: ProxyClientBase: avoid static_cast to partially destructed object
Sjors
added a commit
to Sjors/bitcoin
that referenced
this pull request
Jan 17, 2025
Add detection for int/enum size mismatch and recent fixes for undefined behavior. bitcoin-core/libmultiprocess#121: ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120: proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127: ProxyClientBase: avoid static_cast to partially destructed object
Sjors
added a commit
to Sjors/bitcoin
that referenced
this pull request
Jan 20, 2025
Add detection for int/enum size mismatch and recent fixes for undefined behavior. bitcoin-core/libmultiprocess#121: ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120: proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127: ProxyClientBase: avoid static_cast to partially destructed object
Sjors
added a commit
to Sjors/bitcoin
that referenced
this pull request
Jan 22, 2025
Add detection for int/enum size mismatch and recent fixes for undefined behavior. bitcoin-core/libmultiprocess#121: ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120: proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127: ProxyClientBase: avoid static_cast to partially destructed object
Sjors
added a commit
to Sjors/bitcoin
that referenced
this pull request
Jan 23, 2025
Add detection for int/enum size mismatch and recent fixes for undefined behavior. bitcoin-core/libmultiprocess#121: ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120: proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127: ProxyClientBase: avoid static_cast to partially destructed object
Sjors
added a commit
to Sjors/bitcoin
that referenced
this pull request
Jan 27, 2025
Add detection for int/enum size mismatch and recent fixes for undefined behavior. bitcoin-core/libmultiprocess#121: ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120: proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127: ProxyClientBase: avoid static_cast to partially destructed object
Sjors
added a commit
to Sjors/bitcoin
that referenced
this pull request
Jan 27, 2025
bitcoin-core/libmultiprocess#121: ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120: proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127: ProxyClientBase: avoid static_cast to partially destructed object bitcoin-core/libmultiprocess#129: Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races. bitcoin-core/libmultiprocess#130: refactor: Add CleanupRun function to dedup clean list code
Sjors
added a commit
to Sjors/bitcoin
that referenced
this pull request
Jan 27, 2025
bitcoin-core/libmultiprocess#121: ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120: proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127: ProxyClientBase: avoid static_cast to partially destructed object bitcoin-core/libmultiprocess#129: Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races. bitcoin-core/libmultiprocess#130: refactor: Add CleanupRun function to dedup clean list code bitcoin-core/libmultiprocess#131: doc: fix startAsyncThread comment bitcoin-core/libmultiprocess#133: Fix debian "libatomic not found" error in downstream builds
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jan 27, 2025
This should be the final update to the libmultiprocess package via the depends system. It brings in the libmultiprocess cmake changes from bitcoin-core/libmultiprocess#136 needed to support building as subtree. After this, a followup PR will add libmultiprocess as a git subtree and depends will just use the git subtree instead of hardcoding its own version hash. Since there have been libmultiprocess API changes since the last update, this commit also updates bitcoin code to be compatible with them. This update brings in the following changes: bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races. bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds bitcoin-core/libmultiprocess#94 c++ 20 cleanups bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jan 27, 2025
This should be the final update to the libmultiprocess package via the depends system. It brings in the libmultiprocess cmake changes from #### needed to support building as subtree. After this, a followup PR will add libmultiprocess as a git subtree and depends will just use the git subtree instead of specifying a separate version hash. Since there have been API changes in the meantime as well, it also updates bitcoin code to use the latest libmultiprocess API. This update brings in the following changes: bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jan 27, 2025
This should be the final update to the libmultiprocess package via the depends system. It brings in the libmultiprocess cmake changes from bitcoin-core/libmultiprocess#136 needed to support building as subtree. After this, a followup PR will add libmultiprocess as a git subtree and depends will just use the git subtree instead of hardcoding its own version hash. Since there have been libmultiprocess API changes since the last update, this commit also updates bitcoin code to be compatible with them. This update brings in the following changes: bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races. bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds bitcoin-core/libmultiprocess#94 c++ 20 cleanups bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory bitcoin-core/libmultiprocess#137 doc: Fix broken markdown links
Sjors
pushed a commit
to Sjors/bitcoin
that referenced
this pull request
Jan 28, 2025
This should be the final update to the libmultiprocess package via the depends system. It brings in the libmultiprocess cmake changes from bitcoin-core/libmultiprocess#136 needed to support building as subtree. After this, a followup PR will add libmultiprocess as a git subtree and depends will just use the git subtree instead of hardcoding its own version hash. Since there have been libmultiprocess API changes since the last update, this commit also updates bitcoin code to be compatible with them. This update brings in the following changes: bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races. bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds bitcoin-core/libmultiprocess#94 c++ 20 cleanups bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory bitcoin-core/libmultiprocess#137 doc: Fix broken markdown links
fanquake
added a commit
to bitcoin/bitcoin
that referenced
this pull request
Jan 29, 2025
…ng to subtree 4e0aa18 test: Add test for IPC serialization bug (Ryan Ofsky) 2221c88 depends: Update libmultiprocess library before converting to subtree (Ryan Ofsky) Pull request description: This should be the final update to the libmultiprocess package via the depends system. It brings in the libmultiprocess cmake changes from bitcoin-core/libmultiprocess#136 needed to support building as subtree. After this, followup PR #31741 will add libmultiprocess as a git subtree and depends will just use the git subtree instead of hardcoding its own version hash. Since there have been libmultiprocess API changes since the last update, this commit also updates bitcoin code to be compatible with them. This update has the following new changes since previous update #31105: bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races. bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds bitcoin-core/libmultiprocess#94 c++ 20 cleanups bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory bitcoin-core/libmultiprocess#137 doc: Fix broken markdown links ACKs for top commit: Sjors: ACK 4e0aa18 vasild: ACK 4e0aa18 TheCharlatan: ACK 4e0aa18 Tree-SHA512: 6d81cdf7f44762c7f476212295f6224054fd0a61315bb54786bc7758a2b33e5a2fce925c71e36f7bda320049aa14e7218a458ceb03dacbb869632c466c4789b0
janus
pushed a commit
to BitgesellOfficial/bitgesell
that referenced
this pull request
Sep 1, 2025
This should be the final update to the libmultiprocess package via the depends system. It brings in the libmultiprocess cmake changes from bitcoin-core/libmultiprocess#136 needed to support building as subtree. After this, a followup PR will add libmultiprocess as a git subtree and depends will just use the git subtree instead of hardcoding its own version hash. Since there have been libmultiprocess API changes since the last update, this commit also updates bitcoin code to be compatible with them. This update brings in the following changes: bitcoin-core/libmultiprocess#121 ProxyClientBase: avoid static_cast to partially constructed object bitcoin-core/libmultiprocess#120 proxy-types.h: add static_assert to detect int/enum size mismatch bitcoin-core/libmultiprocess#127 ProxyClientBase: avoid static_cast to partially destructed object bitcoin-core/libmultiprocess#129 Fix "disconnected: write(m_post_fd, &buffer, 1): Broken pipe" EventLoop shutdown races. bitcoin-core/libmultiprocess#130 refactor: Add CleanupRun function to dedup clean list code bitcoin-core/libmultiprocess#131 doc: fix startAsyncThread comment bitcoin-core/libmultiprocess#133 Fix debian "libatomic not found" error in downstream builds bitcoin-core/libmultiprocess#94 c++ 20 cleanups bitcoin-core/libmultiprocess#135 refactor: proxy-types.h API cleanup bitcoin-core/libmultiprocess#136 cmake: Support being included with add_subdirectory bitcoin-core/libmultiprocess#137 doc: Fix broken markdown links
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is a bugfix that should fix the UBSan failure reported #125
In practice there is no real bug here, just like there was no real bug fixed in the recent "ProxyClientBase: avoid static_cast to partially constructed object" change in #121. This is because in practice, ProxyClient subclasses only inherit ProxyClientBase state and do not define any state of their own, so there is nothing bad that could actually happen from static_cast-ing a ProxyClientBase pointer to a ProxyClient pointer inside the ProxyClientBase constructor or destructor.
However, using static_cast this way technically triggers undefined behavior, and that causes UBSan to fail, so this commit drops the static_cast by making ProxyClient::construct() and ProxyClient::destroy() methods into static methods taking a ProxyClientBase& argument instead of instance methods using *this.
Fixes #125
This PR should be a simpler, more robust alternative to a previous for the same bug implemented in #126.