refactor: Drop unsafe AsBytePtr function #27978

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/noptr changing 5 files +24 −25
  1. 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

  2. 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.

  3. DrahtBot added the label Refactoring on Jun 26, 2023
  4. 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
  5. ryanofsky force-pushed on Jun 26, 2023
  6. 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
  7. DrahtBot added the label CI failed on Jun 26, 2023
  8. 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==
    
  9. maflcko added the label Needs rebase on Jun 27, 2023
  10. DrahtBot removed the label Needs rebase on Jun 27, 2023
  11. ryanofsky force-pushed on Jun 27, 2023
  12. 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
  13. 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?
  14. jonatack commented at 7:53 pm on June 27, 2023: contributor
    ACK 650ca0d937c2c90adeeac60adae090902618d771
  15. DrahtBot requested review from maflcko on Jun 27, 2023
  16. DrahtBot removed the label CI failed on Jun 28, 2023
  17. 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
  18. ryanofsky force-pushed on Jun 28, 2023
  19. 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
  20. 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.

  21. 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.

  22. achow101 commented at 9:17 pm on June 29, 2023: member
    ACK 7c853619ee9ea17a79f1234b6c7871a45ceadcb9
  23. achow101 merged this on Jun 29, 2023
  24. achow101 closed this on Jun 29, 2023

  25. sidhujag referenced this in commit efd2416d43 on Jun 30, 2023
  26. bitcoin locked this on Jun 28, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me