refactor: Remove UB in prevector reverse iterators #32490

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2505-less-UB changing 3 files +58 −84
  1. maflcko commented at 7:56 am on May 14, 2025: member

    rend() creates a pointer with offset -1. This is UB, according to the C++ standard: https://eel.is/c++draft/expr.add#4:

    When an expression J that has integral type is added to [...] an
    expression P of pointer type, the result has the type of P.
    
    ... if P points to a (possibly-hypothetical) array element i of an
    array object x with n elements [...] the expressions P + J and J + P
    (where J has the value j) point to the (possibly-hypothetical) array
    element i+j of x if 0≤i+j≤n [...]
    
    Otherwise, the behavior is undefined.
    

    Also, it is unclear why the functions exist at all, when stdlib utils such as std::reverse_iterator{it} or std::views::reverse can be used out of the box.

    So remove them, along with the ubsan suppressions, that are no longer used.

    I’ve tagged this a refactor, because the code was always dead (unused outside of tests). And since commit 2925bd537cbd8c70594e23f6c4298b7101f7f73d it was completely dead. Also, I could not find a sanitizer that detects this type of UB.

  2. refactor: Remove UB in prevector reverse iterators
    rend() creates a pointer with offset -1. This is UB, according to the
    C++ standard: https://eel.is/c++draft/expr.add#4:
    
        When an expression J that has integral type is added to [...] an
        expression P of pointer type, the result has the type of P.
    
        ... if P points to a (possibly-hypothetical) array element i of an
        array object x with n elements [...] the expressions P + J and J + P
        (where J has the value j) point to the (possibly-hypothetical) array
        element i+j of x if 0≤i+j≤n [...]
    
        Otherwise, the behavior is undefined.
    
    Also, it is unclear why the functions exist at all, when stdlib utils
    such as std::reverse_iterator{it} or std::views::reverse can be used out
    of the box.
    
    So remove them, along with the ubsan suppressions, that are no longer
    used.
    fa7f04c8a7
  3. test: Fix whitespace in prevector_tests.cpp
    Bitcoin Core uses 4 spaces indent, but the test was in some lines using
    5 or more.
    
    Just clang-format the whole file.
    faf9082a5f
  4. DrahtBot commented at 7:56 am on May 14, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32490.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK l0rinc, stickies-v, theuni, achow101

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

  5. DrahtBot added the label Refactoring on May 14, 2025
  6. fanquake requested review from stickies-v on May 14, 2025
  7. l0rinc commented at 1:28 pm on May 14, 2025: contributor

    tested ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685

    Thanks for fixing it properly - this explains why I couldn’t trigger an actual failure (local or CI) in #32296.

  8. stickies-v approved
  9. stickies-v commented at 2:30 pm on May 14, 2025: contributor

    ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685, nice find.

    Verified the UB, and removing the dead code makes sense. I’m getting the same clang-format diff for prevector_tests.cpp (+ copyright year change).

  10. theuni approved
  11. theuni commented at 5:59 pm on May 14, 2025: member

    utACK faf9082a5f689e2e51a474bf654e4e9b6ca29685

    I didn’t test for the UB, the argument for removal is good enough for me.

    0prevector<42, char> foo;
    1std::ranges::reverse_view rv{foo};
    

    ^^ works

  12. achow101 commented at 9:20 pm on May 14, 2025: member

    ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685

    Checked dead code removal, did not test for UB.

  13. achow101 merged this on May 14, 2025
  14. achow101 closed this on May 14, 2025

  15. maflcko deleted the branch on May 15, 2025
  16. l0rinc commented at 10:25 am on May 15, 2025: contributor
    This PR is an extension of #32296 (which originally just removed the unnecessary prevector suppression, but @maflcko bisected the cause here and removed the dead code as well). Your reviews would be appreciated there as well.

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: 2025-07-08 12:13 UTC

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