ProxyClientBase: avoid static_cast to partially destructed object #126

pull ryanofsky wants to merge 2 commits into bitcoin-core:master from ryanofsky:pr/ucleanup changing 5 files +45 −38

ryanofsky commented at 6:06 PM on January 15, 2025: collaborator

This is a bugfix that should fix the UBSan failure reported https://github.com/chaincodelabs/libmultiprocess/issues/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 https://github.com/chaincodelabs/libmultiprocess/pull/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

refactor: Add CleanupRun function to dedup clean list code

Also rename "cleanup" variables to distinguish between cleanup iterators and
cleanup callback functions.

c32b38739f

ProxyClientBase: avoid static_cast to partially destructed object

This is a bugfix that should fix the UBSan failure reported
https://github.com/chaincodelabs/libmultiprocess/issues/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 https://github.com/chaincodelabs/libmultiprocess/pull/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

2f0122121f

ryanofsky commented at 7:32 PM on January 15, 2025: collaborator

Closing this in favor of alternative fix in #127 which should be simpler and less fragile.
ryanofsky closed this on Jan 15, 2025
ryanofsky referenced this in commit 3b2617b3e5 on Jan 16, 2025
ryanofsky referenced this in commit caf01fa469 on Jan 27, 2025
bitcoin-core locked this on Jan 16, 2026

Contributors

ryanofsky

Linked (view graph)

#125 Bitcoin Core UBSan failure for interfaces::Echo #127 ProxyClientBase: avoid static_cast to partially destructed object