ProxyClientBase: avoid static_cast to partially constructed object #121

ryanofsky · 2024-12-02T17:08:40Z

ProxyClientBase constructor was trying to call ProxyClient::construct() method before ProxyClient object had been fully constructed. This is causing a UBSAN error reported:

that looks like:

include/mp/proxy.h:95:45: runtime error: downcast of address 0x50600002bdc0 which does not point to an object of type 'ProxyClient<Interface>' (aka 'ProxyClient<ipc::capnp::messages::Init>')
0x50600002bdc0: note: object is of type 'mp::ProxyClientBase<ipc::capnp::messages::Init, interfaces::Init>'

ProxyClientBase constructor was trying to call ProxyClient::construct() method before ProxyClient object had been fully constructed yet. This was causing a UBSAN error reported: https://github.com/bitcoin/bitcoin/actions/runs/11970857809/job/33374462331?pr=30975 bitcoin/bitcoin#30975 (comment) that looked like: include/mp/proxy.h:95:45: runtime error: downcast of address 0x50600002bdc0 which does not point to an object of type 'ProxyClient<Interface>' (aka 'ProxyClient<ipc::capnp::messages::Init>') 0x50600002bdc0: note: object is of type 'mp::ProxyClientBase<ipc::capnp::messages::Init, interfaces::Init>'

ryanofsky · 2024-12-02T17:09:58Z

A more complete stack trace from https://github.com/bitcoin/bitcoin/actions/runs/11970857809/job/33374462331?pr=30975 is:

2024-11-22T10:38:22.317815Z/usr/local/include/mp/proxy.h:95:45: runtime error: downcast of address 0x50600002bdc0 which does not point to an object of type 'ProxyClient<Interface>' (aka 'ProxyClient<ipc::capnp::messages::Init>')
0x50600002bdc0: note: object is of type 'mp::ProxyClientBase<ipc::capnp::messages::Init, interfaces::Init>'
 00 00 00 00  18 04 23 9a 4a 56 00 00  08 03 23 9a 4a 56 00 00  68 17 01 00 b0 50 00 00  60 17 01 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'mp::ProxyClientBase<ipc::capnp::messages::Init, interfaces::Init>'
    #0 0x564a98a7b7fa in mp::ProxyClientBase<ipc::capnp::messages::Init, interfaces::Init>::self() /usr/local/include/mp/proxy.h:95:45
    #1 0x564a98a7b7fa in mp::ProxyClientBase<ipc::capnp::messages::Init, interfaces::Init>::ProxyClientBase(ipc::capnp::messages::Init::Client, mp::Connection*, bool) /usr/local/include/mp/proxy-io.h:435:5
    #2 0x564a98a75cac in mp::ProxyClientCustom<ipc::capnp::messages::Init, interfaces::Init>::ProxyClientCustom(ipc::capnp::messages::Init::Client, mp::Connection*, bool) /usr/local/include/mp/proxy.h:106:45
    #3 0x564a98a75cac in mp::ProxyClient<ipc::capnp::messages::Init>::ProxyClient(ipc::capnp::messages::Init::Client, mp::Connection*, bool) /home/runner/work/_temp/ci/scratch/build-x86_64-pc-linux-gnu/src/ipc/capnp/init.capnp.proxy.h:73:30
    #4 0x564a98a75cac in std::__detail::_MakeUniq<mp::ProxyClient<ipc::capnp::messages::Init>>::__single_object std::make_unique<mp::ProxyClient<ipc::capnp::messages::Init>, ipc::capnp::messages::Init::Client, mp::Connection*, bool>(ipc::capnp::messages::Init::Client&&, mp::Connection*&&, bool&&) /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/unique_ptr.h:1070:34
    #5 0x564a98a74cf6 in std::unique_ptr<mp::ProxyClient<ipc::capnp::messages::Init>, std::default_delete<mp::ProxyClient<ipc::capnp::messages::Init>>> mp::ConnectStream<ipc::capnp::messages::Init>(mp::EventLoop&, int) /usr/local/include/mp/proxy-io.h:577:12
    #6 0x564a98a72c26 in ipc::capnp::(anonymous namespace)::CapnpProtocol::connect(int, char const*) /home/runner/work/_temp/ci/scratch/build-x86_64-pc-linux-gnu/src/ipc/./ipc/capnp/protocol.cpp:54:16
    #7 0x564a989e0d13 in IpcSocketPairTest() /home/runner/work/_temp/ci/scratch/build-x86_64-pc-linux-gnu/src/./test/ipc_test.cpp:141:61
    #8 0x564a97a43380 in ipc_tests::ipc_tests::test_method() /home/runner/work/_temp/ci/scratch/build-x86_64-pc-linux-gnu/src/test/./test/ipc_tests.cpp:15:5
    #9 0x564a97a425ef in ipc_tests::ipc_tests_invoker() /home/runner/work/_temp/ci/scratch/build-x86_64-pc-linux-gnu/src/test/./test/ipc_tests.cpp:12:1
    #10 0x564a965e731d in boost::function0<void>::operator()() const /usr/include/boost/function/function_template.hpp:771:14
    #11 0x564a96667db8 in boost::detail::forward::operator()() /usr/include/boost/test/impl/execution_monitor.ipp:1395:32
    #12 0x564a96667db8 in boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) /usr/include/boost/function/function_template.hpp:137:18
    #13 0x564a966618ad in boost::function0<int>::operator()() const /usr/include/boost/function/function_template.hpp:771:14
    #14 0x564a9654cfec in int boost::detail::do_invoke<boost::shared_ptr<boost::detail::translator_holder_base>, boost::function<int ()>>(boost::shared_ptr<boost::detail::translator_holder_base> const&, boost::function<int ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:308:30
    #15 0x564a9654cfec in boost::execution_monitor::catch_signals(boost::function<int ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:910:16
    #16 0x564a9654d4fd in boost::execution_monitor::execute(boost::function<int ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:1308:16
    #17 0x564a96545b88 in boost::execution_monitor::vexecute(boost::function<void ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:1404:5
    #18 0x564a96545b88 in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned long) /usr/include/boost/test/impl/unit_test_monitor.ipp:49:9
    #19 0x564a965a9b75 in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /usr/include/boost/test/impl/framework.ipp:815:44
    #20 0x564a965a8a84 in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /usr/include/boost/test/impl/framework.ipp:784:58
    #21 0x564a965a8a84 in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /usr/include/boost/test/impl/framework.ipp:784:58
    #22 0x564a96543fdb in boost::unit_test::framework::run(unsigned long, bool) /usr/include/boost/test/impl/framework.ipp:1722:29
    #23 0x564a96572ca0 in boost::unit_test::unit_test_main(boost::unit_test::test_suite* (*)(int, char**), int, char**) /usr/include/boost/test/impl/unit_test_main.ipp:250:9
    #24 0x7f99693bf1c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
    #25 0x7f99693bf28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
    #26 0x564a9643eea4 in _start (/home/runner/work/_temp/ci/scratch/build-x86_64-pc-linux-gnu/src/test/test_bitcoin+0x1355ea4) (BuildId: 5aa0be89931561f87bfc4b03f135b8264e316e4e)

TheCharlatan

lgtm ACK 5b81192

Makes sense to instantiate the custom implementation first before calling construct.

Sjors · 2025-01-10T17:58:07Z

Testing on Bitcoin Core CI: bitcoin/bitcoin@93f7f30 bitcoin/bitcoin#30975 (comment)

Which fails, but it seems to happen at a different place (at Echo instead of Init), so perhaps it's a completely different issue.

Sjors · 2025-01-13T10:45:33Z

I reproduced the original failure locally on macOS 15.2, by installing libmultiprocess from master @ abe254b (no depends).

export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1"
cmake -B build -DWITH_MULTIPROCESS=ON -DSANITIZERS=undefined
cmake --build build

build/src/test/test_bitcoin --run_test=ipc_tests --log_level=all | grep -v disabled
...
test/ipc_test.cpp:133: info: check socketpair(1, 1, 0, fds) == 0 has passed
/usr/local/include/mp/proxy.h:95:45: runtime error: downcast of address 0x600001d74480 which does not point to an object of type 'ProxyClient<Interface>' (aka 'ProxyClient<ipc::capnp::messages::Init>')
0x600001d74480: note: object is of type 'mp::ProxyClientBase<ipc::capnp::messages::Init, interfaces::Init>'
...
SUMMARY: UndefinedBehaviorSanitizer: dynamic-type-mismatch /usr/local/include/mp/proxy.h:95:45 in

I then installed libmultiprocess from this PR, deleted the Bitcoin Core build dir and did the above again.

The test now makes it line 143, so I'd say the original issue is fixed.

test/ipc_test.cpp:133: info: check socketpair(1, 1, 0, fds) == 0 has passed
test/ipc_test.cpp:143: info: check remote_echo->echo("echo test") == "echo test" has passed
/usr/local/include/mp/proxy.h:95:45: runtime error: downcast of address 0x600001f28700 which does not point to an object of type 'ProxyClient<Interface>' (aka 'ProxyClient<ipc::capnp::messages::Echo>')
0x600001f28700: note: object is of type 'mp::ProxyClientBase<ipc::capnp::messages::Echo, interfaces::Echo>'

I'll open a new issue for the new failure.

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

… object ProxyClientBase: avoid static_cast to partially destructed object 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

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

…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

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

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#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

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

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

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

…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

ryanofsky mentioned this pull request Dec 2, 2024

ci: build multiprocess on most jobs bitcoin/bitcoin#30975

Closed

Sjors mentioned this pull request Dec 3, 2024

Stratum v2 via IPC Mining Interface tracking issue bitcoin/bitcoin#31098

Open

33 tasks

ryanofsky mentioned this pull request Dec 3, 2024

Multiprocess for non depends Sjors/bitcoin#65

Closed

TheCharlatan approved these changes Jan 5, 2025

View reviewed changes

Sjors mentioned this pull request Jan 13, 2025

Bitcoin Core UBSan failure for interfaces::Echo #125

Closed

ryanofsky merged commit 350067f into bitcoin-core:master Jan 13, 2025

ryanofsky mentioned this pull request Jan 15, 2025

ProxyClientBase: avoid static_cast to partially destructed object #126

Closed

ryanofsky mentioned this pull request Jan 15, 2025

ProxyClientBase: avoid static_cast to partially destructed object #127

Merged

ryanofsky mentioned this pull request Jan 27, 2025

depends: Update libmultiprocess library before converting to subtree bitcoin/bitcoin#31740

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ProxyClientBase: avoid static_cast to partially constructed object #121

ProxyClientBase: avoid static_cast to partially constructed object #121

Uh oh!

ryanofsky commented Dec 2, 2024

Uh oh!

ryanofsky commented Dec 2, 2024

Uh oh!

TheCharlatan left a comment

Uh oh!

Sjors commented Jan 10, 2025 •

edited

Loading

Uh oh!

Sjors commented Jan 13, 2025

Uh oh!

Uh oh!

ProxyClientBase: avoid static_cast to partially constructed object #121

ProxyClientBase: avoid static_cast to partially constructed object #121

Uh oh!

Conversation

ryanofsky commented Dec 2, 2024

Uh oh!

ryanofsky commented Dec 2, 2024

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Jan 13, 2025

Uh oh!

Uh oh!

Sjors commented Jan 10, 2025 •

edited

Loading