random: drop syscall wrapper usage for getrandom() #27699
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
maflcko
reviewed
May 19, 2023
49f44a6 to
f33211a
Compare
|
Added a commit to use Added a commit to slightly refactor |
|
Guix Build: 15e52e32e8ad2574fa5a1bd5e7699f33b7084a63d4ed3951a9b9f22632dd57fb guix-build-f33211a4fe56/output/aarch64-linux-gnu/SHA256SUMS.part
1c981a6d7cce696f4b562443ef53f401dfa6edc4704154f453ee4f66b8fc72d6 guix-build-f33211a4fe56/output/aarch64-linux-gnu/bitcoin-f33211a4fe56-aarch64-linux-gnu-debug.tar.gz
c8d0b81e5ad5f504650690c9b0192338b22a09aeead9cc1a48054e228704f1a8 guix-build-f33211a4fe56/output/aarch64-linux-gnu/bitcoin-f33211a4fe56-aarch64-linux-gnu.tar.gz
4e59bc9cbe2499fd415ab1b9eeab40d090cb19aae2df2eee1f978c5ec01b8e63 guix-build-f33211a4fe56/output/arm-linux-gnueabihf/SHA256SUMS.part
49b76219924fa46c7df008a5c36aa54f226f57223863e9da7be393f30f327475 guix-build-f33211a4fe56/output/arm-linux-gnueabihf/bitcoin-f33211a4fe56-arm-linux-gnueabihf-debug.tar.gz
71173c84fc02728c53d3cd02569f1767611544fa501929a91c17545647e8b436 guix-build-f33211a4fe56/output/arm-linux-gnueabihf/bitcoin-f33211a4fe56-arm-linux-gnueabihf.tar.gz
77a976c916ef30925e6b192d5ef49d1d8d1b3fe9aa3908c6b9b1373a0542657a guix-build-f33211a4fe56/output/arm64-apple-darwin/SHA256SUMS.part
4e655cf22f003455a14edd9408ea8f2620f39185a7e400664768bba3de0d0f8f guix-build-f33211a4fe56/output/arm64-apple-darwin/bitcoin-f33211a4fe56-arm64-apple-darwin-unsigned.dmg
db7a9a84d2b3d546bef11ed8f9bc63fe319133427a1c1033fbc962f44dd5cfcb guix-build-f33211a4fe56/output/arm64-apple-darwin/bitcoin-f33211a4fe56-arm64-apple-darwin-unsigned.tar.gz
df293de71a8c59767111a6353ade25a2d470fca1991cba06949a73667cc46ff9 guix-build-f33211a4fe56/output/arm64-apple-darwin/bitcoin-f33211a4fe56-arm64-apple-darwin.tar.gz
f59ce5b708a69ab9cfe80936a70e2f33490328b2eb16a31d6dd75f4655dad2da guix-build-f33211a4fe56/output/dist-archive/bitcoin-f33211a4fe56.tar.gz
aa2e1e6ebf6e781410660e8fc7ee71c568fdd849fbc6d2edf16dbf52936a8d9b guix-build-f33211a4fe56/output/powerpc64-linux-gnu/SHA256SUMS.part
cb5501376f3f23259b68c0a043f68d0adf39f9c5fb3daa734d2947c01135c5a1 guix-build-f33211a4fe56/output/powerpc64-linux-gnu/bitcoin-f33211a4fe56-powerpc64-linux-gnu-debug.tar.gz
986b1c06a16b9850de514eed315bb8ccd911148b2696a8e6c816de767696de24 guix-build-f33211a4fe56/output/powerpc64-linux-gnu/bitcoin-f33211a4fe56-powerpc64-linux-gnu.tar.gz
c2f4cd7bbfc4520f72862f7820a304a5610c5484c112a45247d21252af78d400 guix-build-f33211a4fe56/output/powerpc64le-linux-gnu/SHA256SUMS.part
e859c2f48937729752cbf066bc661e427e90a5fde28f21d4ef985d8f29cf6bc5 guix-build-f33211a4fe56/output/powerpc64le-linux-gnu/bitcoin-f33211a4fe56-powerpc64le-linux-gnu-debug.tar.gz
dcdfca6c1220828035d0d8be64baf327165143971abb9cec6aae2c498bbd9432 guix-build-f33211a4fe56/output/powerpc64le-linux-gnu/bitcoin-f33211a4fe56-powerpc64le-linux-gnu.tar.gz
a77bd1100669c1af1d3106b89d4a78cb526d072d6cfdb8465d93338783e85c18 guix-build-f33211a4fe56/output/riscv64-linux-gnu/SHA256SUMS.part
51cccb85c3a407be2da35820eeb40eef2608041ad62f831a643d01654462ea9f guix-build-f33211a4fe56/output/riscv64-linux-gnu/bitcoin-f33211a4fe56-riscv64-linux-gnu-debug.tar.gz
b5b5a801fed7544720734415d11f1595384d2ee5f7ebd142c20396ee7d16d860 guix-build-f33211a4fe56/output/riscv64-linux-gnu/bitcoin-f33211a4fe56-riscv64-linux-gnu.tar.gz
1818901a04e8bf65c74c3acd827785ee13bcf2473ee60dfae5e130fe3794eb15 guix-build-f33211a4fe56/output/x86_64-apple-darwin/SHA256SUMS.part
c8232b9c439f8e2d7cc461b0c5f26cd2fe95f49a9ae946d55b4fafe429a3768a guix-build-f33211a4fe56/output/x86_64-apple-darwin/bitcoin-f33211a4fe56-x86_64-apple-darwin-unsigned.dmg
a41eacbdcd5df69a91ebc79423b278289b639251c85ef287d59c41b0c516b8bb guix-build-f33211a4fe56/output/x86_64-apple-darwin/bitcoin-f33211a4fe56-x86_64-apple-darwin-unsigned.tar.gz
c615b6c7186387d9e5c0391ad3d09fc4f9b01a5a3ae1a60bfa7d7cb03efd0ba1 guix-build-f33211a4fe56/output/x86_64-apple-darwin/bitcoin-f33211a4fe56-x86_64-apple-darwin.tar.gz
6c7c8ab4bd739dbd6334bfebbe9a5f9105fdb3d1796a214fde7673889d4866e6 guix-build-f33211a4fe56/output/x86_64-linux-gnu/SHA256SUMS.part
465e8facee7866379a0b5e766d3aad288f0ae8b5cbd58c15c5692968c8eac178 guix-build-f33211a4fe56/output/x86_64-linux-gnu/bitcoin-f33211a4fe56-x86_64-linux-gnu-debug.tar.gz
4c71815664f197b4ca373c506958c124b3bf340c8e57eb8659d6330ab8f4055f guix-build-f33211a4fe56/output/x86_64-linux-gnu/bitcoin-f33211a4fe56-x86_64-linux-gnu.tar.gz
a1f608382d9f9e43f1305ff3456a37a1adeb0ef48f9a97e8eac1374bf56be459 guix-build-f33211a4fe56/output/x86_64-w64-mingw32/SHA256SUMS.part
ed976f91d0f55dec43d87312779c0e871b8a10638a490d7c95906760173cef1d guix-build-f33211a4fe56/output/x86_64-w64-mingw32/bitcoin-f33211a4fe56-win64-debug.zip
2006c8094f95a059fde52f2249da0b7917f3d00919eed523808468a431d221b7 guix-build-f33211a4fe56/output/x86_64-w64-mingw32/bitcoin-f33211a4fe56-win64-setup-unsigned.exe
03779f3b39bd5ffb89fc4371c0d2041f7042ff2ee1eddc761fda3ab467c76148 guix-build-f33211a4fe56/output/x86_64-w64-mingw32/bitcoin-f33211a4fe56-win64-unsigned.tar.gz
e4047bb5949b3b1815b9e6215ba5b1224b8e389e2caaeaf03a6d3d935c4a96e7 guix-build-f33211a4fe56/output/x86_64-w64-mingw32/bitcoin-f33211a4fe56-win64.zip |
|
ACK f33211a Nice to be able to drop the work-around 🚀 I got the same as you for the guix build: |
|
Guix builds: |
hebasto
reviewed
May 20, 2023
hebasto
reviewed
May 20, 2023
hebasto
reviewed
May 20, 2023
Remove it. Make this change, so in a future commit, we can combine #ifdefs, and avoid duplicate <sys/random.h> includes once we switch to using getrandom directly. Also remove the comment about macOS 10.12. We already require macOS > 10.15, so it is redundant.
Rather than multiple instances of (void)GetDevURandom to silence compiler warnings.
f33211a to
0ab6db5
Compare
This requires a linux kernel of 3.17.0+, which seems entirely reasonable. 3.17 went EOL in 2015, and the last supported 3.x kernel (3.16) went EOL > 4 years ago, in 2020. For reference, the current oldest maintained kernel is 4.14 (released 2017, EOL Jan 2024). Support for `getrandom()` (and `getentropy()`) was added to glibc 2.25, https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00079.html, and we already require 2.27+. All that being said, I don't think you would encounter a current day system, running with kernel headers older than 3.17 (released 2014) but also having a glibc of 2.27+ (released 2018).
The corresponding workaround will also be dropped in oss-fuzz: https://github.com/google/oss-fuzz/blob/25946a544856413d31d9cbb3a366a4aef5a8fd60/projects/bitcoin-core/build.sh#L49.
0ab6db5 to
5228223
Compare
|
Guix builds: |
|
Guix builds: |
|
ACK 5228223 guix builds match hebastos |
fanquake
added a commit
to fanquake/oss-fuzz
that referenced
this pull request
May 22, 2023
This is no-longer required after project changes. See: bitcoin/bitcoin#27699
|
oss-fuzz followup here: google/oss-fuzz#10361. |
DavidKorczynski
pushed a commit
to google/oss-fuzz
that referenced
this pull request
May 22, 2023
This is no-longer required after project changes. See: bitcoin/bitcoin#27699.
theuni
reviewed
May 22, 2023
| @@ -251,7 +251,7 @@ static void Strengthen(const unsigned char (&seed)[32], SteadyClock::duration du | |||
| /** Fallback: get 32 bytes of system entropy from /dev/urandom. The most | |||
| * compatible way to get cryptographic randomness on UNIX-ish platforms. | |||
| */ | |||
| static void GetDevURandom(unsigned char *ent32) | |||
| [[maybe_unused]] static void GetDevURandom(unsigned char *ent32) | |||
There was a problem hiding this comment.
TIL [[maybe_unused]] + static could be used for this effect. Neat :)
|
Ping @EthanHeilman. Not that I'm worried, but... curious if any part of what you're working on could verify that this is indeed a noop? |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 23, 2023
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
Apr 1, 2024
Align with bitcoin#27699.
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
Apr 1, 2024
be04ac9 fixup! cmake: Check system symbols (Hennadii Stepanov) 4ff1257 fixup! cmake: Check system symbols (Hennadii Stepanov) Pull request description: This PR: 1) Backports changes from bitcoin#27375. 2) Aligns symbol checks with bitcoin#27699. A possible way to review this PR and the whole symbol check logic is to observe the Autotools vs CMake diff in the `config/bitcoin-config.h`: ``` ./autogen.sh ./configure cmake -B build diff -u src/config/bitcoin-config.h build/src/config/bitcoin-config.h ``` ACKs for top commit: TheCharlatan: ACK be04ac9 Tree-SHA512: 71bde1e3133ae1550b2948991782173ee85913a4746d7195ed0ce79cd2106997f9127b35826b2ac848cdfaba73d604bd2567e129103e11e5f0d6163af59f5845
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
Apr 2, 2024
List of the squashed commits:
=============================
cmake: Add root `CMakeLists.txt` file
cmake: Introduce core interface libraries to encapsulate common flags
Also add a sanity check for non-encapsulated (directory-wide) build
properties.
cmake: Add `config/bitcoin-config.h` support
cmake: Check system headers
cmake: Check system symbols
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
cmake: Check compiler features
cmake: Add position independent code support
cmake: Add platform-specific definitions and properties
cmake: Build `crc32c` static library
cmake: Build `leveldb` static library
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
cmake: Build `minisketch` static library
cmake: Build `secp256k1` static library
cmake: Build `univalue` static library
cmake: Build `bitcoin_crypto` library
cmake: Build `bitcoin_util` static library
cmake: Build `bitcoin_consensus` library
cmake: Build `bitcoind` executable
depends: Amend handling flags environment variables
If any of {C,CXX,CPP,LD}FLAGS is specified it should be assigned to
a non-type-specific variable.
build: Generate `share/toolchain.cmake` in depends
cmake: Add cross-compiling support
To configure CMake for cross-compiling, use
`--toolchain depends/${HOST}/share/toolchain.cmake` command-line option.
cmake: Add `TristateOption` module
cmake: Add `ccache` support
cmake: Add `libnatpmp` optional package support
cmake: Add `libminiupnpc` optional package support
cmake: Add `libzmq` optional package support
cmake: Add `systemtap-sdt` optional package support
cmake: Build `bitcoin-cli` executable
cmake: Build `bitcoin-tx` executable
cmake: Build `bitcoin-util` executable
cmake: Add wallet functionality
cmake: Add test config and runners
cmake: Build `bench_bitcoin` executable
cmake: Build `test_bitcoin` executable
cmake: Include CTest
cmake: Add `TryAppendCXXFlags` module
cmake: Add `TryAppendLinkerFlag` module
cmake: Add platform-specific compiler flags
cmake: Add platform-specific linker flags
cmake: Redefine/adjust per-configuration flags
cmake: Add general compile options
cmake: Add `HARDENING` option
cmake: Add `REDUCE_EXPORTS` option
cmake: Add `WERROR` option
cmake: Implement `make install`
cmake: Generate `obj/build.h` header
cmake: Add `GenerateBuildInfo.cmake` script
Revert "build, qt: Do not install *.prl files"
This reverts commit 1155978.
qt, build: Drop `QT_STATICPLUGIN` macro
Our `QT_STATICPLUGIN` macro is effectively equivalent to the Qt's
`QT_STATIC` macro. No need to handle both of them.
cmake: Build `bitcoin-qt` executable
cmake: Build `test_bitcoin-qt` executable
qt: Drop `Q_IMPORT_PLUGIN` macros
When using CMake, each plugin comes with a C++ stub file that
automatically initializes the static plugin. Consequently, any target
that links against a plugin has this C++ file added to its SOURCES,
which makes the removed code redundant.
cmake: Add `libqrencode` optional package support
cmake: Add `SANITIZERS` option
cmake: Add external signer support
cmake: Add fuzzing options
cmake: Add `AddWindowsResources` module
cmake: Add `Maintenance` module
cmake: Migrate Guix build scripts to CMake
cmake: Add vcpkg manifest file
cmake: Add preset for MSVC build
Fix MSVC warning C4273 "inconsistent dll linkage"
cmake, doc: Update `release-process.md`
Only versioning has been updated for now.
cmake: Add compiler diagnostic flags
test: Fix MSVC warning C4101 "unreferenced local variable"
ci: Test CMake edge cases
Keep this commit at the top when rebasing.
Add macOS cross compile jobs.
Don't pass `-fno-extended-identifiers` to a linker
Process linker flags properly.
refactor! cmake: Redefine/adjust per-configuration flags
Process linker flags properly.
Add `deploy` target for macOS
Add the missing source file.
See: bitcoin#28578
Backport bitcoin#27375.
Align with bitcoin#27699.
hebasto
added a commit
to hebasto/bitcoin
that referenced
this pull request
Apr 2, 2024
List of the squashed commits:
=============================
cmake: Add root `CMakeLists.txt` file
cmake: Introduce core interface libraries to encapsulate common flags
Also add a sanity check for non-encapsulated (directory-wide) build
properties.
cmake: Add `config/bitcoin-config.h` support
cmake: Check system headers
cmake: Check system symbols
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
cmake: Check compiler features
cmake: Add position independent code support
cmake: Add platform-specific definitions and properties
cmake: Build `crc32c` static library
cmake: Build `leveldb` static library
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
cmake: Build `minisketch` static library
cmake: Build `secp256k1` static library
cmake: Build `univalue` static library
cmake: Build `bitcoin_crypto` library
cmake: Build `bitcoin_util` static library
cmake: Build `bitcoin_consensus` library
cmake: Build `bitcoind` executable
depends: Amend handling flags environment variables
If any of {C,CXX,CPP,LD}FLAGS is specified it should be assigned to
a non-type-specific variable.
build: Generate `share/toolchain.cmake` in depends
cmake: Add cross-compiling support
To configure CMake for cross-compiling, use
`--toolchain depends/${HOST}/share/toolchain.cmake` command-line option.
cmake: Add `TristateOption` module
cmake: Add `ccache` support
cmake: Add `libnatpmp` optional package support
cmake: Add `libminiupnpc` optional package support
cmake: Add `libzmq` optional package support
cmake: Add `systemtap-sdt` optional package support
cmake: Build `bitcoin-cli` executable
cmake: Build `bitcoin-tx` executable
cmake: Build `bitcoin-util` executable
cmake: Add wallet functionality
cmake: Add test config and runners
cmake: Build `bench_bitcoin` executable
cmake: Build `test_bitcoin` executable
cmake: Include CTest
cmake: Add `TryAppendCXXFlags` module
cmake: Add `TryAppendLinkerFlag` module
cmake: Add platform-specific compiler flags
cmake: Add platform-specific linker flags
cmake: Redefine/adjust per-configuration flags
cmake: Add general compile options
cmake: Add `HARDENING` option
cmake: Add `REDUCE_EXPORTS` option
cmake: Add `WERROR` option
cmake: Implement `make install`
cmake: Generate `obj/build.h` header
cmake: Add `GenerateBuildInfo.cmake` script
Revert "build, qt: Do not install *.prl files"
This reverts commit 1155978.
qt, build: Drop `QT_STATICPLUGIN` macro
Our `QT_STATICPLUGIN` macro is effectively equivalent to the Qt's
`QT_STATIC` macro. No need to handle both of them.
cmake: Build `bitcoin-qt` executable
cmake: Build `test_bitcoin-qt` executable
qt: Drop `Q_IMPORT_PLUGIN` macros
When using CMake, each plugin comes with a C++ stub file that
automatically initializes the static plugin. Consequently, any target
that links against a plugin has this C++ file added to its SOURCES,
which makes the removed code redundant.
cmake: Add `libqrencode` optional package support
cmake: Add `SANITIZERS` option
cmake: Add external signer support
cmake: Add fuzzing options
cmake: Add `AddWindowsResources` module
cmake: Add `Maintenance` module
cmake: Migrate Guix build scripts to CMake
cmake: Add vcpkg manifest file
cmake: Add preset for MSVC build
Fix MSVC warning C4273 "inconsistent dll linkage"
cmake, doc: Update `release-process.md`
Only versioning has been updated for now.
cmake: Add compiler diagnostic flags
test: Fix MSVC warning C4101 "unreferenced local variable"
ci: Test CMake edge cases
Keep this commit at the top when rebasing.
Add macOS cross compile jobs.
Don't pass `-fno-extended-identifiers` to a linker
Process linker flags properly.
refactor! cmake: Redefine/adjust per-configuration flags
Process linker flags properly.
Add `deploy` target for macOS
Add the missing source file.
See: bitcoin#28578
Backport bitcoin#27375.
Align with bitcoin#27699.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This requires a linux kernel of
3.17+, which seems entirelyreasonable.
3.17went EOL in 2015, and the last supported3.xkernel(
3.16) went EOL > 4 years ago, in 2020. For reference, the currentoldest maintained kernel is
4.14(released 2017, going EOL Jan 2024).Support for
getrandom()(andgetentropy()) was added toglibc
2.25https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00079.html:and we already require
2.27or later.All that being said, I don't think you would encounter a current day (+~6 months from now)
system, running with kernel headers older than 3.17 (released 2014) but also having a
glibc of 2.27+ (released 2018)?
Removing this (our only) use of
syscall()also means we can drop a workaround in our MSAN jobs.If this is merged, I'll drop the same workaround in oss-fuzz.