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 <[email protected]>

Author: ryanofsky

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();

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<U>::max() >= MAX && std::numeric_limits<U>::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<I>(be64toh(raw));
         } else {
-            s.read({AsBytePtr(&raw), Bytes});
+            s.read(AsWritableBytes(Span{&raw, 1}).first(Bytes));
             v = static_cast<I>(le64toh(raw));
         }
     }

src/span.h

@@ -243,21 +243,16 @@ T& SpanPopBack(Span<T>& 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<const std::byte*>(data); }
-inline std::byte* AsBytePtr(void* data) { return reinterpret_cast<std::byte*>(data); }
-
 // From C++20 as_bytes and as_writeable_bytes
 template <typename T>
 Span<const std::byte> AsBytes(Span<T> s) noexcept
 {
-    return {AsBytePtr(s.data()), s.size_bytes()};
+    return {reinterpret_cast<const std::byte*>(s.data()), s.size_bytes()};
 }
 template <typename T>
 Span<std::byte> AsWritableBytes(Span<T> s) noexcept
 {
-    return {AsBytePtr(s.data()), s.size_bytes()};
+    return {reinterpret_cast<std::byte*>(s.data()), s.size_bytes()};
 }
 
 template <typename V>

src/wallet/bdb.cpp

@@ -31,6 +31,10 @@
 
 namespace wallet {
 namespace {
+Span<const std::byte> SpanFromDbt(const SafeDbt& dbt)
+{
+    return {reinterpret_cast<const std::byte*>(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<const std::byte> raw_key = {AsBytePtr(datKey.get_data()), datKey.get_size()};
+    Span<const std::byte> 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;

src/wallet/sqlite.cpp

@@ -24,6 +24,12 @@
 namespace wallet {
 static constexpr int32_t WALLET_SCHEMA_VERSION = 0;
 
+static Span<const std::byte> SpanFromBlob(sqlite3_stmt* stmt, int col)
+{
+    return {reinterpret_cast<const std::byte*>(sqlite3_column_blob(stmt, col)),
+            static_cast<size_t>(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;
 }