ProxyClientBase: avoid static_cast to partially destructed object#126
Closed
ryanofsky wants to merge 2 commits intobitcoin-core:masterfrom
Closed
ProxyClientBase: avoid static_cast to partially destructed object#126ryanofsky wants to merge 2 commits intobitcoin-core:masterfrom
ryanofsky wants to merge 2 commits intobitcoin-core:masterfrom
Conversation
Also rename "cleanup" variables to distinguish between cleanup iterators and cleanup callback functions.
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 moves ProxyClientBase cleanup out of the ProxyClientBase destructor, into ProxyClient subclass destructors to prevent this. An alternate fix could maybe avoid the need to do this by making ProxyClient::construct() and ProxyClient::destroy() methods into static methods taking a ProxyClientBase& self argument instead of instance methods using *this. This would avoid the need to use static_cast at all. But it would also require changes to the ProxyClient class code generator so unclear if it would be a simpler fix. Fixes bitcoin-core#125
This was referenced Jan 15, 2025
Collaborator
Author
|
Closing this in favor of alternative fix in #127 which should be simpler and less fragile. |
ryanofsky
added a commit
that referenced
this pull request
Jan 16, 2025
…d object 63a39d4 ProxyClientBase: avoid static_cast to partially destructed object (Ryan Ofsky) Pull request description: 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. Top commit has no ACKs. Tree-SHA512: da55eba1c7f8aeb42e9d34a63793aa2f702c9a2b6e7a17869c35ce24eb188b5ff253a8a975ee8950fe8516ea1fadd1aec22e0755ad3c835bdeb826a18cf8541d
ryanofsky
added a commit
that referenced
this pull request
Jan 27, 2025
700085f refactor: Add CleanupRun function to dedup clean list code (Ryan Ofsky) Pull request description: Add CleanupRun function to dedup clean list code. Also rename "cleanup" variables to distinguish between cleanup iterators and cleanup callback functions. These changes were originally part of #126 which was closed for other reasons, but I think they are still helpful and make code easier to navigate. Since this renames a public struct member, it will require a change to bitcoin core when the depends version is bumped: ```c++ diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp index 4b67a5bd1e36..691bdf5f9242 100644 --- a/src/ipc/capnp/protocol.cpp +++ b/src/ipc/capnp/protocol.cpp @@ -73,7 +73,7 @@ public: } void addCleanup(std::type_index type, void* iface, std::function<void()> cleanup) override { - mp::ProxyTypeRegister::types().at(type)(iface).cleanup.emplace_back(std::move(cleanup)); + mp::ProxyTypeRegister::types().at(type)(iface).cleanup_fns.emplace_back(std::move(cleanup)); } Context& context() override { return m_context; } void startLoop(const char* exe_name) ``` Top commit has no ACKs. Tree-SHA512: 04ca403f80ddc2d0bf6ba5b0aa5cee651f0c509dcf67137789c744a1ae63bed32a4e34510ba21f2ed9797c46ace640b49e2db67659e03ea4ad2cca18491cabef
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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 moves ProxyClientBase cleanup out of the ProxyClientBase destructor, into ProxyClient subclass destructors to prevent this.
An alternate fix could maybe avoid the need to do this by making ProxyClient::construct() and ProxyClient::destroy() methods into static methods taking a ProxyClientBase& self argument instead of instance methods using *this. This would avoid the need to use static_cast at all. But it would also require changes to the ProxyClient class code generator so unclear if it would be a simpler fix.
Fixes #125