An empty span constructed from an empty vector may have a null data pointer depending on the implementation. Remove the BITCOINKERNEL_ARG_NONNULL requirement for these arguments and instead handle such null arguments in the implementation.
Also cherry-picked from #33845 to show that CI task passing now.
An empty span constructed from an empty vector may have a null data
pointer depending on the implementation. Remove the
BITCOINKERNEL_ARG_NONNULL requirement for these arguments and instead
handle such null arguments in the implementation.
Base the task on --preset=dev-mode to ensure maximal coverage and add
the following:
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33853.
See 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.
393@@ -395,6 +394,12 @@ BOOST_AUTO_TEST_CASE(btck_transaction_tests)
394 auto tx2{Transaction{tx_data_2}};
395 CheckHandle(tx, tx2);
396
397+ auto invalid_data = hex_string_to_byte_vec("012300");
398+ BOOST_CHECK_THROW(Transaction{invalid_data}, std::runtime_error);
399+ auto empty_data = hex_string_to_byte_vec("");
400+ BOOST_CHECK_THROW(Transaction{empty_data}, std::runtime_error);
401+ BOOST_CHECK_THROW(Transaction{std::span<std::byte>(static_cast<std::byte*>(nullptr), 2)}, std::runtime_error);
0 BOOST_CHECK_THROW(Transaction{std::span{static_cast<std::byte*>(nullptr), 2}}, std::runtime_error);
style-nit in 5b89956eeb76cf8c9717152fbb0928e026fc0087 (no need to re-touch), also I haven’t tried compiling, but I think the compiler can derive the type here and it can be omitted. Same below.
Ah sorry for missing this, but this doesn’t work at all. A nullptr can never point to an object, so it is always invalid to create a non-zero span from it.
libc++ also complains about this.
You can reproduce via:
rm -rf ./bld-cmake && cmake -B ./bld-cmake -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++;-stdlib=libc++;-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG' -DBUILD_GUI=OFF -DBUILD_FUZZ_BINARY=OFF -DBUILD_BENCH=OFF -DBUILD_KERNEL_LIB=ON -DENABLE_WALLET=OFF -DENABLE_IPC=OFF && cmake --build ./bld-cmake --parallel $( nproc ) && valgrind --tool=none ./bld-cmake/bin/test_kernel --catch_system_error=no
0/cxx_build/include/c++/v1/span:451: libc++ Hardening assertion __count == 0 || std::to_address(__first) != nullptr failed: passed nullptr with non-zero length in span's constructor (iterator, len)
I think the options are:
0diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
1index 0b9ea5137f..1695920f2b 100644
2--- a/src/kernel/bitcoinkernel.cpp
3+++ b/src/kernel/bitcoinkernel.cpp
4@@ -497,7 +497,7 @@ struct btck_Txid: Handle<btck_Txid, Txid> {};
5
6 btck_Transaction* btck_transaction_create(const void* raw_transaction, size_t raw_transaction_len)
7 {
8- if (raw_transaction == nullptr && raw_transaction_len != 0) {
9+ if (raw_transaction == nullptr) {
10 return nullptr;
11 }
12 try {
Side-step the C++ wrapper and directly call the C code with a raw C-span from this test.
Fully remove the check (and this test line) and just let the caller write safe code 😅
0diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
1index 0b9ea5137f..6545e02b84 100644
2--- a/src/kernel/bitcoinkernel.cpp
3+++ b/src/kernel/bitcoinkernel.cpp
4@@ -497,9 +497,6 @@ struct btck_Txid: Handle<btck_Txid, Txid> {};
5
6 btck_Transaction* btck_transaction_create(const void* raw_transaction, size_t raw_transaction_len)
7 {
8- if (raw_transaction == nullptr && raw_transaction_len != 0) {
9- return nullptr;
10- }
11 try {
12 DataStream stream{std::span{reinterpret_cast<const std::byte*>(raw_transaction), raw_transaction_len}};
13 return btck_Transaction::create(std::make_shared<const CTransaction>(deserialize, TX_WITH_WITNESS, stream));
Make sense to handle empty spans at runtime properly via our serialization and the existing serialize-error-handling, to avoid runtime violations of the non-null attribute.
review ACK a3ac59a4316305fb38a5338b48940682889d0dc2 🥈
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK a3ac59a4316305fb38a5338b48940682889d0dc2 🥈
31eAXvD6wGVVRpNLiXbsn7/zC6/P9NnLdS7YrwhpGGouhjeYRaXIrHQWgTTk9uYKe9BQAwuZmNJ0LekXwTOV4BA==
Code review ACK a3ac59a4316305fb38a5338b48940682889d0dc2
Logic correctly handles null pointers from empty spans.
code review ACK a3ac59a4316305fb38a5338b48940682889d0dc2
496@@ -497,6 +497,9 @@ struct btck_Txid: Handle<btck_Txid, Txid> {};
497
498 btck_Transaction* btck_transaction_create(const void* raw_transaction, size_t raw_transaction_len)
499 {
500+ if (raw_transaction == nullptr && raw_transaction_len != 0) {
501+ return nullptr;
LogError here, and in the other 2 places?
Post-merge ACK a3ac59a4316305fb38a5338b48940682889d0dc2
It seems like the same reasoning should apply to btck_chainstate_manager_options_create’s data_directory and blocks_directory? Perhaps useful for a follow-up (edit: see #33867 ), with a brief docstring on how to use and interpret BITCOINKERNEL_ARG_NONNULL so we’re clear on its usage? E.g.:
0/**
1 * BITCOINKERNEL_ARG_NONNULL is a compiler attribute used to indicate that
2 * certain pointer arguments to a function are not expected to be null.
3 *
4 * Callers must not pass a null pointer for arguments marked with this attribute,
5 * as doing so may result in undefined behavior. This attribute should only be
6 * used for arguments where a null pointer is unambiguously a programmer error,
7 * such as for opaque handles, and not for pointers to raw input data that might
8 * validly be null (e.g., from an empty std::span or std::string).
9 */