refactor: Drop unsafe AsBytePtr function

ryanofsky commented at 4:36 pm on June 26, 2023: contributor

Replace calls to AsBytePtr with 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 platform specific or undefined. 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.

The change was motivated by discussion on #27973 and #27927 and is compatible with those PRs

DrahtBot commented at 4:36 pm on June 26, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	jonatack, sipa, achow101
Stale ACK	MarcoFalke

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Refactoring on Jun 26, 2023

in src/serialize.h:475 in 3fca8c3c75 outdated

471@@ -472,10 +472,10 @@ struct CustomUintFormatter
472         if (v < 0 || v > MAX) throw std::ios_base::failure("CustomUintFormatter value out of range");
473         if (BigEndian) {
474             uint64_t raw = htobe64(v);
475-            s.write({AsBytePtr(&raw) + 8 - Bytes, Bytes});
476+            s.write({reinterpret_cast<const std::byte*>(&raw) + 8 - Bytes, Bytes});

sipa commented at 7:54 pm on June 26, 2023:

This can be written to make use of AsBytes, actually:

 0@@ -472,10 +472,10 @@ struct CustomUintFormatter
 1         if (v < 0 || v > MAX) throw std::ios_base::failure("CustomUintFormatter value out of range");
 2         if (BigEndian) {
 3             uint64_t raw = htobe64(v);
 4-            s.write({reinterpret_cast<const std::byte*>(&raw) + 8 - Bytes, Bytes});
 5+            s.write(AsBytes(Span{&raw, 1}).last(Bytes));
 6         } else {
 7             uint64_t raw = htole64(v);
 8-            s.write({reinterpret_cast<const std::byte*>(&raw), Bytes});
 9+            s.write(AsBytes(Span{&raw, 1}).first(Bytes));
10         }
11     }
12 
13@@ -485,10 +485,10 @@ struct CustomUintFormatter
14         static_assert(std::numeric_limits<U>::max() >= MAX && std::numeric_limits<U>::min() <= 0, "Assigned type too small");
15         uint64_t raw = 0;
16         if (BigEndian) {
17-            s.read({reinterpret_cast<std::byte*>(&raw) + 8 - Bytes, Bytes});
18+            s.read(AsWritableBytes(Span{&raw, 1}).last(Bytes));
19             v = static_cast<I>(be64toh(raw));
20         } else {
21-            s.read({reinterpret_cast<std::byte*>(&raw), Bytes});
22+            s.read(AsWritableBytes(Span{&raw, 1}).first(Bytes));
23             v = static_cast<I>(le64toh(raw));
24         }
25     }

ryanofsky commented at 10:27 pm on June 26, 2023:

Nice, I like the use of first() and last() to avoid the pointer math

ryanofsky force-pushed on Jun 26, 2023

ryanofsky commented at 10:50 pm on June 26, 2023: contributor

Updated 3fca8c3c7501b7a8e0a3adbbf6c78bb446e60924 -> 2109a147406247bb654ee5fb8421a8594919fbd5 (pr/noptr.1 -> pr/noptr.2, compare) using AsBytes and a SpanFromDbt function to avoid reinterpret_cast in a few places

DrahtBot added the label CI failed on Jun 26, 2023

maflcko commented at 6:08 am on June 27, 2023: member

Needs rebase

lgtm ACK 2109a147406247bb654ee5fb8421a8594919fbd5 🐭

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: lgtm ACK 2109a147406247bb654ee5fb8421a8594919fbd5 🐭
3s0s73KuMSK4XPfToyrjbfKq/G1uRROhpS1JoQ1ok0h6dsWjvu6s80qPtfU6rWJmBf8In1XqiGmp6oA7r3AdrCQ==

maflcko added the label Needs rebase on Jun 27, 2023

DrahtBot removed the label Needs rebase on Jun 27, 2023

ryanofsky force-pushed on Jun 27, 2023

ryanofsky commented at 5:04 pm on June 27, 2023: contributor

Rebased 2109a147406247bb654ee5fb8421a8594919fbd5 -> 650ca0d937c2c90adeeac60adae090902618d771 (pr/noptr.2 -> pr/noptr.3, compare) due to conflict with #27479

in src/wallet/bdb.h:193 in 650ca0d937 outdated

185@@ -186,6 +186,11 @@ class SafeDbt final
186     operator Dbt*();
187 };
188 
189+inline Span<const std::byte> SpanFromDbt(const SafeDbt& dbt)
190+{
191+   return {reinterpret_cast<const std::byte*>(dbt.get_data()), dbt.get_size()};
192+}
193+

jonatack commented at 7:32 pm on June 27, 2023:

Would it make sense for SpanFromDbt() to be a static method in src/wallet/bdb.cpp instead, which contains its only callers?

jonatack commented at 7:53 pm on June 27, 2023: contributor

ACK 650ca0d937c2c90adeeac60adae090902618d771

DrahtBot requested review from maflcko on Jun 27, 2023

DrahtBot removed the label CI failed on Jun 28, 2023

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 <pieter@wuille.net>

7c853619ee

ryanofsky force-pushed on Jun 28, 2023

ryanofsky commented at 7:21 pm on June 28, 2023: contributor

Updated 650ca0d937c2c90adeeac60adae090902618d771 -> 7c853619ee9ea17a79f1234b6c7871a45ceadcb9 (pr/noptr.3 -> pr/noptr.4, compare) switching from AsWritableBytes to MakeWritableByteSpan, making SpanFromDbt a static function, and adding a SpanFromBlob function to simplify sqlite code

jonatack commented at 8:04 pm on June 28, 2023: contributor

re-ACK 7c853619ee9ea17a79f1234b6c7871a45ceadcb9

The OP and commit message might need to be updated a bit.

sipa commented at 4:00 pm on June 29, 2023: member

utACK 7c853619ee9ea17a79f1234b6c7871a45ceadcb9

I’ve also changed AsBytes(Span{x}) -> MakeByteSpan(x) and similar where possible in #27993.

achow101 commented at 9:17 pm on June 29, 2023: member

ACK 7c853619ee9ea17a79f1234b6c7871a45ceadcb9

achow101 merged this on Jun 29, 2023

achow101 closed this on Jun 29, 2023

sidhujag referenced this in commit efd2416d43 on Jun 30, 2023

bitcoin locked this on Jun 28, 2024

refactor: Drop unsafe AsBytePtr function #27978

Reviews