Skip to content

proxy-types.h: add static_assert to detect int/enum size mismatch #120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

ryanofsky
Copy link
Collaborator

@ryanofsky ryanofsky commented Nov 12, 2024

Add static_assert to detect when an int field is too small to hold an enum value

This catches the bug TheCharlatan pointed out in bitcoin/bitcoin#29409 (comment)

Copy link
Collaborator

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm ACK bbc80ab

@Sjors
Copy link
Member

Sjors commented Jan 10, 2025

Looks correct to me based on the description, but I'm not very familiar with the libmultiprocess internals.

Maybe add a test to illustrate the fix?

@ryanofsky
Copy link
Collaborator Author

Maybe add a test to illustrate the fix?

Good idea. There was no coverage for enum/int conversions so I pushed a new commit 110349f to add some.

I don't think there's a good way to add test coverage for the new static_assert since it just triggers compile errors if enum and int types aren't compatible. But it is possible to trigger the static assert manually by changing:

--- a/test/mp/test/foo.capnp
+++ b/test/mp/test/foo.capnp
@@ -27,7 +27,7 @@ interface FooInterface $Proxy.wrap("mp::test::FooImplementation") {
     passEmpty @12 (arg :FooEmpty) -> (result :FooEmpty);
     passMessage @13 (arg :FooMessage) -> (result :FooMessage);
     passMutable @14 (arg :FooMutable) -> (arg :FooMutable);
-    passEnum @15 (arg :Int32) -> (result :Int32);
+    passEnum @15 (arg :Int16) -> (result :Int32);
 }
 
 interface FooCallback $Proxy.wrap("mp::test::FooCallback") {

Then there should be a compile error if you run make check:

include/mp/proxy-types.h:968:19: error: static assertion failed due to requirement 'std::numeric_limits<unsigned short>::max() >= std::numeric_limits<unsigned int>::max()': mismatched integral/enum types
  968 |     static_assert(std::numeric_limits<T>::max() >= std::numeric_limits<E>::max(), "mismatched integral/enum types");
[...]
include/mp/proxy-types.h:968:49: note: expression evaluates to '65535 >= 4294967295'
  968 |     static_assert(std::numeric_limits<T>::max() >= std::numeric_limits<E>::max(), "mismatched integral/enum types");

@ryanofsky ryanofsky merged commit 621a04a into bitcoin-core:master Jan 13, 2025
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants