From 7c853619ee9ea17a79f1234b6c7871a45ceadcb9 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 26 Jun 2023 12:12:20 -0400 Subject: [PATCH] refactor: Drop unsafe AsBytePtr function Replace calls to AsBytePtr with direct calls to AsBytes or reinterpret_cast. AsBytePtr is just a wrapper around reinterpret_cast. It accepts any type of pointer as an argument and uses reinterpret_cast to cast the argument to a std::byte pointer. Despite taking any type of pointer as an argument, it is not useful to call AsBytePtr on most types of pointers, because byte representations of most types will be implmentation-specific. Also, because it is named similarly to the AsBytes function, AsBytePtr looks safer than it actually is. Both AsBytes and AsBytePtr call reinterpret_cast internally and may be unsafe to use with certain types, but AsBytes at least has some type checking and can only be called on Span objects, while AsBytePtr can be called on any pointer argument. Co-authored-by: Pieter Wuille --- src/bench/ellswift.cpp | 4 ++-- src/serialize.h | 8 ++++---- src/span.h | 9 ++------- src/wallet/bdb.cpp | 10 +++++++--- src/wallet/sqlite.cpp | 18 +++++++++--------- 5 files changed, 24 insertions(+), 25 deletions(-) diff --git a/src/bench/ellswift.cpp b/src/bench/ellswift.cpp index 75729e170c7..f0348421b38 100644 --- a/src/bench/ellswift.cpp +++ b/src/bench/ellswift.cpp @@ -17,12 +17,12 @@ static void EllSwiftCreate(benchmark::Bench& bench) uint256 entropy = GetRandHash(); bench.batch(1).unit("pubkey").run([&] { - auto ret = key.EllSwiftCreate(AsBytes(Span{entropy})); + auto ret = key.EllSwiftCreate(MakeByteSpan(entropy)); /* Use the first 32 bytes of the ellswift encoded public key as next private key. */ key.Set(UCharCast(ret.data()), UCharCast(ret.data()) + 32, true); assert(key.IsValid()); /* Use the last 32 bytes of the ellswift encoded public key as next entropy. */ - std::copy(ret.begin() + 32, ret.begin() + 64, AsBytePtr(entropy.data())); + std::copy(ret.begin() + 32, ret.begin() + 64, MakeWritableByteSpan(entropy).begin()); }); ECC_Stop(); diff --git a/src/serialize.h b/src/serialize.h index 7bc7b10779c..c46e5a56408 100644 --- a/src/serialize.h +++ b/src/serialize.h @@ -472,10 +472,10 @@ struct CustomUintFormatter if (v < 0 || v > MAX) throw std::ios_base::failure("CustomUintFormatter value out of range"); if (BigEndian) { uint64_t raw = htobe64(v); - s.write({AsBytePtr(&raw) + 8 - Bytes, Bytes}); + s.write(AsBytes(Span{&raw, 1}).last(Bytes)); } else { uint64_t raw = htole64(v); - s.write({AsBytePtr(&raw), Bytes}); + s.write(AsBytes(Span{&raw, 1}).first(Bytes)); } } @@ -485,10 +485,10 @@ struct CustomUintFormatter static_assert(std::numeric_limits::max() >= MAX && std::numeric_limits::min() <= 0, "Assigned type too small"); uint64_t raw = 0; if (BigEndian) { - s.read({AsBytePtr(&raw) + 8 - Bytes, Bytes}); + s.read(AsWritableBytes(Span{&raw, 1}).last(Bytes)); v = static_cast(be64toh(raw)); } else { - s.read({AsBytePtr(&raw), Bytes}); + s.read(AsWritableBytes(Span{&raw, 1}).first(Bytes)); v = static_cast(le64toh(raw)); } } diff --git a/src/span.h b/src/span.h index c98784aee40..0563556dc27 100644 --- a/src/span.h +++ b/src/span.h @@ -243,21 +243,16 @@ T& SpanPopBack(Span& span) return back; } -//! Convert a data pointer to a std::byte data pointer. -//! Where possible, please use the safer AsBytes helpers. -inline const std::byte* AsBytePtr(const void* data) { return reinterpret_cast(data); } -inline std::byte* AsBytePtr(void* data) { return reinterpret_cast(data); } - // From C++20 as_bytes and as_writeable_bytes template Span AsBytes(Span s) noexcept { - return {AsBytePtr(s.data()), s.size_bytes()}; + return {reinterpret_cast(s.data()), s.size_bytes()}; } template Span AsWritableBytes(Span s) noexcept { - return {AsBytePtr(s.data()), s.size_bytes()}; + return {reinterpret_cast(s.data()), s.size_bytes()}; } template diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 68abdcd81e9..658500e4563 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -31,6 +31,10 @@ namespace wallet { namespace { +Span SpanFromDbt(const SafeDbt& dbt) +{ + return {reinterpret_cast(dbt.get_data()), dbt.get_size()}; +} //! Make sure database has a unique fileid within the environment. If it //! doesn't, throw an error. BDB caches do not work properly when more than one @@ -702,7 +706,7 @@ DatabaseCursor::Status BerkeleyCursor::Next(DataStream& ssKey, DataStream& ssVal return Status::FAIL; } - Span raw_key = {AsBytePtr(datKey.get_data()), datKey.get_size()}; + Span raw_key = SpanFromDbt(datKey); if (!m_key_prefix.empty() && std::mismatch(raw_key.begin(), raw_key.end(), m_key_prefix.begin(), m_key_prefix.end()).second != m_key_prefix.end()) { return Status::DONE; } @@ -711,7 +715,7 @@ DatabaseCursor::Status BerkeleyCursor::Next(DataStream& ssKey, DataStream& ssVal ssKey.clear(); ssKey.write(raw_key); ssValue.clear(); - ssValue.write({AsBytePtr(datValue.get_data()), datValue.get_size()}); + ssValue.write(SpanFromDbt(datValue)); return Status::MORE; } @@ -796,7 +800,7 @@ bool BerkeleyBatch::ReadKey(DataStream&& key, DataStream& value) int ret = pdb->get(activeTxn, datKey, datValue, 0); if (ret == 0 && datValue.get_data() != nullptr) { value.clear(); - value.write({AsBytePtr(datValue.get_data()), datValue.get_size()}); + value.write(SpanFromDbt(datValue)); return true; } return false; diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index 8d7fe97bb10..ecd34bb96a6 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -24,6 +24,12 @@ namespace wallet { static constexpr int32_t WALLET_SCHEMA_VERSION = 0; +static Span SpanFromBlob(sqlite3_stmt* stmt, int col) +{ + return {reinterpret_cast(sqlite3_column_blob(stmt, col)), + static_cast(sqlite3_column_bytes(stmt, col))}; +} + static void ErrorLogCallback(void* arg, int code, const char* msg) { // From sqlite3_config() documentation for the SQLITE_CONFIG_LOG option: @@ -434,10 +440,8 @@ bool SQLiteBatch::ReadKey(DataStream&& key, DataStream& value) return false; } // Leftmost column in result is index 0 - const std::byte* data{AsBytePtr(sqlite3_column_blob(m_read_stmt, 0))}; - size_t data_size(sqlite3_column_bytes(m_read_stmt, 0)); value.clear(); - value.write({data, data_size}); + value.write(SpanFromBlob(m_read_stmt, 0)); sqlite3_clear_bindings(m_read_stmt); sqlite3_reset(m_read_stmt); @@ -527,12 +531,8 @@ DatabaseCursor::Status SQLiteCursor::Next(DataStream& key, DataStream& value) value.clear(); // Leftmost column in result is index 0 - const std::byte* key_data{AsBytePtr(sqlite3_column_blob(m_cursor_stmt, 0))}; - size_t key_data_size(sqlite3_column_bytes(m_cursor_stmt, 0)); - key.write({key_data, key_data_size}); - const std::byte* value_data{AsBytePtr(sqlite3_column_blob(m_cursor_stmt, 1))}; - size_t value_data_size(sqlite3_column_bytes(m_cursor_stmt, 1)); - value.write({value_data, value_data_size}); + key.write(SpanFromBlob(m_cursor_stmt, 0)); + value.write(SpanFromBlob(m_cursor_stmt, 1)); return Status::MORE; }