-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| CI_DESC="CI job using old Cap'n Proto version" | ||
| CI_DIR=build-olddeps | ||
| export CXXFLAGS="-Werror -Wall -Wextra -Wpedantic -Wno-unused-parameter -Wno-error=array-bounds" | ||
| NIX_ARGS=(--argstr capnprotoVersion "0.7.1") | ||
| BUILD_ARGS=(-k) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| commit e3da7da967b94f373c29a198ce45f30fb9f0e517 | ||
| Author: Ed Catmur <[email protected]> | ||
| Date: Tue Jan 31 16:27:04 2023 +0000 | ||
|
|
||
| Remove operator!= synthesized by spaceship | ||
|
|
||
| An operator!= suppresses the reversed equality comparison candidate generation. | ||
|
|
||
| This is visible in clang 16 (rc0 just released) and in gcc trunk (so probably gcc 13). | ||
|
|
||
| diff --git a/c++/src/kj/string.h b/c++/src/kj/string.h | ||
| index 193442aa..17835892 100644 | ||
| --- a/c++/src/kj/string.h | ||
| +++ b/c++/src/kj/string.h | ||
| @@ -122,10 +122,14 @@ public: | ||
| inline constexpr const char* end() const { return content.end() - 1; } | ||
|
|
||
| inline constexpr bool operator==(decltype(nullptr)) const { return content.size() <= 1; } | ||
| +#if !__cpp_impl_three_way_comparison | ||
| inline constexpr bool operator!=(decltype(nullptr)) const { return content.size() > 1; } | ||
| +#endif | ||
|
|
||
| inline bool operator==(const StringPtr& other) const; | ||
| +#if !__cpp_impl_three_way_comparison | ||
| inline bool operator!=(const StringPtr& other) const { return !(*this == other); } | ||
| +#endif | ||
| inline bool operator< (const StringPtr& other) const; | ||
| inline bool operator> (const StringPtr& other) const { return other < *this; } | ||
| inline bool operator<=(const StringPtr& other) const { return !(other < *this); } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,12 +2,39 @@ | |
| , crossPkgs ? import <nixpkgs> {} | ||
| , enableLibcxx ? false # Whether to use libc++ toolchain and libraries instead of libstdc++ | ||
| , minimal ? false # Whether to create minimal shell without extra tools (faster when cross compiling) | ||
| , capnprotoVersion ? null | ||
| }: | ||
|
|
||
| let | ||
| lib = pkgs.lib; | ||
| llvm = crossPkgs.llvmPackages_20; | ||
| capnproto = crossPkgs.capnproto.override (lib.optionalAttrs enableLibcxx { clangStdenv = llvm.libcxxStdenv; }); | ||
| capnprotoHashes = { | ||
| "0.7.0" = "sha256-Y/7dUOQPDHjniuKNRw3j8dG1NI9f/aRWpf8V0WzV9k8="; | ||
| "0.7.1" = "sha256-3cBpVmpvCXyqPUXDp12vCFCk32ZXWpkdOliNH37UwWE="; | ||
| "0.8.0" = "sha256-rfiqN83begjJ9eYjtr21/tk1GJBjmeVfa3C3dZBJ93w="; | ||
| "0.8.1" = "sha256-OZqNVYdyszro5rIe+w6YN00g6y8U/1b8dKYc214q/2o="; | ||
| "0.9.0" = "sha256-yhbDcWUe6jp5PbIXzn5EoKabXiWN8lnS08hyfxUgEQ0="; | ||
| "0.9.2" = "sha256-BspWOPZcP5nCTvmsDE62Zutox+aY5pw42d6hpH3v4cM="; | ||
| "0.10.0" = "sha256-++F4l54OMTDnJ+FO3kV/Y/VLobKVRk461dopanuU3IQ="; | ||
| "0.10.4" = "sha256-45sxnVyyYIw9i3sbFZ1naBMoUzkpP21WarzR5crg4X8="; | ||
| "1.0.0" = "sha256-NLTFJdeOzqhk4ATvkc17Sh6g/junzqYBBEoXYGH/czo="; | ||
| "1.0.2" = "sha256-LVdkqVBTeh8JZ1McdVNtRcnFVwEJRNjt0JV2l7RkuO8="; | ||
| "1.1.0" = "sha256-gxkko7LFyJNlxpTS+CWOd/p9x/778/kNIXfpDGiKM2A="; | ||
| "1.2.0" = "sha256-aDcn4bLZGq8915/NPPQsN5Jv8FRWd8cAspkG3078psc="; | ||
| }; | ||
| capnprotoBase = if capnprotoVersion == null then crossPkgs.capnproto else crossPkgs.capnproto.overrideAttrs (old: { | ||
| version = capnprotoVersion; | ||
| src = crossPkgs.fetchFromGitHub { | ||
| owner = "capnproto"; | ||
| repo = "capnproto"; | ||
| 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 ]; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #194 (comment)
Yes that's right. These versions of capnproto added some c++20 support which didn't actually work with later compiler versions. |
||
| } // (lib.optionalAttrs (lib.versionOlder capnprotoVersion "0.10") { | ||
| env = { }; # Drop -std=c++20 flag forced by nixpkgs | ||
| })); | ||
| capnproto = capnprotoBase.override (lib.optionalAttrs enableLibcxx { clangStdenv = llvm.libcxxStdenv; }); | ||
| clang = if enableLibcxx then llvm.libcxxClang else llvm.clang; | ||
| clang-tools = llvm.clang-tools.override { inherit enableLibcxx; }; | ||
| in crossPkgs.mkShell { | ||
|
|
||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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.