-
Notifications
You must be signed in to change notification settings - Fork 37
mpgen: Work around c++20 / capnproto 0.8 incompatibility #194
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
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
Updated 151ef65 -> c659685 ( |
| rev = "v${capnprotoVersion}"; | ||
| hash = lib.attrByPath [capnprotoVersion] "" capnprotoHashes; | ||
| }; | ||
| patches = lib.optionals (lib.versionAtLeast capnprotoVersion "0.9.0" && lib.versionOlder capnprotoVersion "0.10.4") [ ./ci/patches/spaceship.patch ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
88d9504: do I read this correctly as: apply patch to versions 0.9.0 <= ... < 0.10.4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #194 (comment)
88d9504: do I read this correctly as: apply patch to versions 0.9.0 <= ... < 0.10.4?
Yes that's right. These versions of capnproto added some c++20 support which didn't actually work with later compiler versions.
|
utACK 88d9504 |
The kj/string.h header installed by capnproto 0.8 doesn't work well when
compiling with -std=c++20 or later because the reverse StringPtr/char*
comparison function it provides is broken in c++20:
inline bool operator==(const char* a, const StringPtr& b) { return b == a; }
Before C++20 this would implicitly convert `a` to a StringPtr and call the
StringPtr::operator== method. But starting with C++20 it actually calls itself
recursively and either loops forever or crashes.
This problem was fixed upstream by
capnproto/capnproto#1170 in Cap'n Proto 0.9.0. Avoid
the problem here for older versions by just not using the operator. A CI job
testing older versions is added in the next commit to avoid similar breakage in
the future.
The CI job currently just tests old Cap'n Proto versions, but it might be nice to extend in the future to test old compilers & build tools too. Support for versions of Cap'n Proto before 0.7.0 was dropped in bitcoin-core#88 in order to avoid compiler warnings and simplify code. Before that, versions back to 0.5 were supported and are basically still compatible since the Cap'n Proto API hasn't changed and libmultiprocess does not rely on newer features.
30930df build: require CapnProto 0.7.0 or better (Sjors Provoost) Pull request description: Although 1.0.1. is the oldest version currently covered by Bitcoin Core's extensive CI, Debian Bookwork ships 0.9.2 and #194 introduces test coverage for even older versions. 0.7 has been required since #88. The CI run of Sjors/bitcoin#100 @ [3d55222](https://github.com/Sjors/bitcoin/pull/100/checks?sha=3d552223712eed88d17e5ead1ef7d1ba6fd7e89e) previously checked Bitcoin Core CI against 1.0.1 as the minimum. Lowering the minimum further should not be a problem for that CI. ACKs for top commit: ryanofsky: Code review ACK 30930df. Planning to follow up in #194 to actually test minimum version and error if capnproto version detected is affected by CVE-2022-46149 Tree-SHA512: bed5843973c8ff1f0b2bd93efe7169824c2306097efefaace1752efeb06606df765b68b7ef50c07f5d703010c4d1b324099d6780fa0363e126d34ac1307fba1a
Also document minimum Cap'n Proto version in doc/install.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased 88d9504 -> dc3ba22 (pr/string.3 -> pr/string.4, compare) on top of #193 and added explicit check for CVE-2022-46149
| rev = "v${capnprotoVersion}"; | ||
| hash = lib.attrByPath [capnprotoVersion] "" capnprotoHashes; | ||
| }; | ||
| patches = lib.optionals (lib.versionAtLeast capnprotoVersion "0.9.0" && lib.versionOlder capnprotoVersion "0.10.4") [ ./ci/patches/spaceship.patch ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #194 (comment)
88d9504: do I read this correctly as: apply patch to versions 0.9.0 <= ... < 0.10.4?
Yes that's right. These versions of capnproto added some c++20 support which didn't actually work with later compiler versions.
|
utACK dc3ba22 Thanks for adding the CVE version check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK dc3ba22
| OR CapnProto_VERSION STREQUAL "0.10.0" | ||
| OR CapnProto_VERSION STREQUAL "0.10.1" | ||
| OR CapnProto_VERSION STREQUAL "0.10.2") | ||
| message(FATAL_ERROR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't ipc a trusted interface, where anyone having access can already fully control the process (like calling createTransaction), so leaking memory should not make anything worse? A warning seems fine here? Also, a cve check seems a bit unrelated from a C++20 code workaround (from the title).
Just a nit and not a blocker, but I wanted to mention it, since this will lead to a compile error on Ubuntu 22.04 (bitcoin/bitcoin#33176 (comment)), for an option that will be enabled by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For ubuntu 22.04 if this will create confusion, I feel like a good solution might be to improve error message and documentation to specifically suggest disabling ENABLE_IPC, or installing a newer version of capnproto. Ideally it seems like ubuntu could update from 0.8.0 to 0.8.1 given 0.8.1 was released to fix the CVE and doesn't have any other changes.
On IPC being a trusted interface, it's definitely true with the current interface, a malicious client could do a lot of things to crash the node and maybe even take control of it. But this doesn't have to be the case. We could provide capnproto interfaces that do rigorously check all their inputs and enforce resource constraints. It would even be possible to adapt the Mining interface to do this, but it hasn't been a reason to make the extra effort. I also feel like showing an error after discovering a package with a CVE is kind of a public service, since we don't know if there may be other software using the package.
Currently the version of capnproto on Debian 12, 0.9.2, does not play well with `mpgen` used by recently-merged multiprocess PR #31802. This is currently being worked on: bitcoin-core/libmultiprocess#194 ... but in the meantime bump Debian versions to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "mpgen: Work around c++20 / capnproto 0.8 incompatibility" (c4cb758)
I think commit message is wrong in suggesting that with c++20 the operator== function is buggy and will call itself recursively. It seems the reason this happens is not really because of c++20 but because of a gcc bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53499. See also bitcoin/bitcoin#33176 (comment)
…6f1e54 1b8d4a6f1e54 Merge bitcoin-core/libmultiprocess#194: mpgen: Work around c++20 / capnproto 0.8 incompatibility f1fad396bf5f Merge bitcoin-core/libmultiprocess#195: ci: Add openbsd eed42f210d17 ci: Bump all tasks to actions/checkout@v5 486a510bbeff ci: Remove ancient and problematic -lstdc++fs in mpexample dd40897efe79 Add missing thread include 98414e7d2867 ci: Add openbsd dc3ba2204606 cmake, doc: Add check for CVE-2022-46149 cb170d4913a2 Merge bitcoin-core/libmultiprocess#193: build: require CapnProto 0.7.0 or better 8ceeaa6ae401 ci: Add olddeps job to test old dependencies versions c4cb758eccb5 mpgen: Work around c++20 / capnproto 0.8 incompatibility 30930dff7b06 build: require CapnProto 0.7.0 or better git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1b8d4a6f1e54b92708bd2ad627ec6d440a1daf3d
1b8d4a6f1e Merge bitcoin-core/libmultiprocess#194: mpgen: Work around c++20 / capnproto 0.8 incompatibility f1fad396bf Merge bitcoin-core/libmultiprocess#195: ci: Add openbsd eed42f210d ci: Bump all tasks to actions/checkout@v5 486a510bbe ci: Remove ancient and problematic -lstdc++fs in mpexample dd40897efe Add missing thread include 98414e7d28 ci: Add openbsd dc3ba22046 cmake, doc: Add check for CVE-2022-46149 cb170d4913 Merge bitcoin-core/libmultiprocess#193: build: require CapnProto 0.7.0 or better 8ceeaa6ae4 ci: Add olddeps job to test old dependencies versions c4cb758ecc mpgen: Work around c++20 / capnproto 0.8 incompatibility 30930dff7b build: require CapnProto 0.7.0 or better git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1b8d4a6f1e54b92708bd2ad627ec6d440a1daf3d
1b8d4a6f1e Merge bitcoin-core/libmultiprocess#194: mpgen: Work around c++20 / capnproto 0.8 incompatibility f1fad396bf Merge bitcoin-core/libmultiprocess#195: ci: Add openbsd eed42f210d ci: Bump all tasks to actions/checkout@v5 486a510bbe ci: Remove ancient and problematic -lstdc++fs in mpexample dd40897efe Add missing thread include 98414e7d28 ci: Add openbsd dc3ba22046 cmake, doc: Add check for CVE-2022-46149 cb170d4913 Merge bitcoin-core/libmultiprocess#193: build: require CapnProto 0.7.0 or better 8ceeaa6ae4 ci: Add olddeps job to test old dependencies versions c4cb758ecc mpgen: Work around c++20 / capnproto 0.8 incompatibility 30930dff7b build: require CapnProto 0.7.0 or better git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1b8d4a6f1e54b92708bd2ad627ec6d440a1daf3d
1b8d4a6f1e Merge bitcoin-core/libmultiprocess#194: mpgen: Work around c++20 / capnproto 0.8 incompatibility f1fad396bf Merge bitcoin-core/libmultiprocess#195: ci: Add openbsd eed42f210d ci: Bump all tasks to actions/checkout@v5 486a510bbe ci: Remove ancient and problematic -lstdc++fs in mpexample dd40897efe Add missing thread include 98414e7d28 ci: Add openbsd dc3ba22046 cmake, doc: Add check for CVE-2022-46149 cb170d4913 Merge bitcoin-core/libmultiprocess#193: build: require CapnProto 0.7.0 or better 8ceeaa6ae4 ci: Add olddeps job to test old dependencies versions c4cb758ecc mpgen: Work around c++20 / capnproto 0.8 incompatibility 30930dff7b build: require CapnProto 0.7.0 or better git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1b8d4a6f1e54b92708bd2ad627ec6d440a1daf3d
dd68d0f Squashed 'src/ipc/libmultiprocess/' changes from b4120d34bad2..1b8d4a6f1e54 (Ryan Ofsky) Pull request description: Includes: - bitcoin-core/libmultiprocess#193 - bitcoin-core/libmultiprocess#195 - bitcoin-core/libmultiprocess#194 These changes are needed to build fix libmultiprocess build issue that happens on OpenBSD and work around an incompatibility between GCC versions <14 and cap'nproto versions <0.9 when compiling with c++20 that was fixed upstream in capnproto/capnproto#1170. The issues were reported: - #33219 - #33176 - willcl-ark/bitcoin-core-docker#43 The fixes added CI jobs upstream to catch these issues earlier. The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh) ACKs for top commit: Sjors: ACK 323b3fd achow101: ACK 323b3fd hebasto: ACK 323b3fd, I've reproduced the subtree update locally. The two issues noted in this PR are unrelated to its changes and can be addressed separately. Tree-SHA512: 3d03693d269c04d9ed10e8dd03e8059062929f37616d974c6fdf346ee62737c990ec550e013575e7474bfa4efcead3938bf9b259d62c073d76e720ebafe4ff66
|
I'm not sure, as you say this was fixed upstream in v0.9 , but I think I might still be hitting this on debian bookworm with 0.9.2?: May also be a different issue (in which case I can open a new issue here or in bitcoin/bitcoin if that would be more appropriate). |
Yeah, to reproduce: diff --git a/ci/configs/openbsd.bash b/ci/configs/openbsd.bash
index a404e2b..70ae010 100644
--- a/ci/configs/openbsd.bash
+++ b/ci/configs/openbsd.bash
@@ -1,5 +1,6 @@
CI_DESC="CI config for OpenBSD"
CI_DIR=build-openbsd
+export CXX=clang++-16
export CXXFLAGS="-Werror -Wall -Wextra -Wpedantic -Wno-unused-parameter"
CMAKE_ARGS=(-G Ninja)
BUILD_ARGS=(-k 0)And (on debian:bookworm !): |
|
re: @willcl-ark #194 (comment)
If I'm interpreting correctly, debian bookworm uses clang 14 according to https://packages.debian.org/bookworm/clang so maybe doesn't have this issue by default, but I think you will run into this problem with clang-16 and newer. I made a new issue for this #199 so maybe we can figure out a solution there |
…6f1e54 1b8d4a6f1e54 Merge bitcoin-core/libmultiprocess#194: mpgen: Work around c++20 / capnproto 0.8 incompatibility f1fad396bf5f Merge bitcoin-core/libmultiprocess#195: ci: Add openbsd eed42f210d17 ci: Bump all tasks to actions/checkout@v5 486a510bbeff ci: Remove ancient and problematic -lstdc++fs in mpexample dd40897efe79 Add missing thread include 98414e7d2867 ci: Add openbsd dc3ba2204606 cmake, doc: Add check for CVE-2022-46149 cb170d4913a2 Merge bitcoin-core/libmultiprocess#193: build: require CapnProto 0.7.0 or better 8ceeaa6ae401 ci: Add olddeps job to test old dependencies versions c4cb758eccb5 mpgen: Work around c++20 / capnproto 0.8 incompatibility 30930dff7b06 build: require CapnProto 0.7.0 or better git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1b8d4a6f1e54b92708bd2ad627ec6d440a1daf3d
Fix ubuntu 22.04 compatibility issue reported by fanquake in bitcoin/bitcoin#33176 and add CI job to make sure libmultiprocess stays compatible with old versions of Cap'n Proto.
Changes are described in more detail in commit messages.