span: diagnose dangling views from MakeByteSpan/MakeUCharSpan #36183

pull kevkevinpal wants to merge 1 commits into bitcoin:master from kevkevinpal:span-lifetimebound-make-byte-uchar changing 4 files +14 −7

kevkevinpal commented at 2:35 PM on September 7, 2026: contributor

Follow-up to #36164, as suggested in #36164 (comment).

Tracked in https://github.com/kevkevinpal/bitcoin/issues/550.

Problem: MakeByteSpan and MakeUCharSpan return views into their input. A temporary container can leave those views dangling.

Fix: Add LIFETIMEBOUND so Clang diagnoses the misuse while preserving immediate use, matching the util/string.h helpers from #36164.

A few call sites that wrap a temporary view (leveldb::Slice, std::span, std::string_view) are updated so they do not trip the new diagnostic.

span: diagnose dangling views from MakeByteSpan/MakeUCharSpan

Follow-up to #36164. Add LIFETIMEBOUND so Clang diagnoses storing
spans of temporaries, matching the util/string.h helpers.

6591bf02ac

DrahtBot commented at 2:35 PM on September 7, 2026: 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/36183.



Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers

Concept ACK jeanpablojp

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
jeanpablojp commented at 10:31 PM on September 7, 2026: contributor

Concept ACK

LIFETIMEBOUND on const V& claims the result borrows from v. True when V owns the bytes, false when V is a non-owning view, and MakeByteSpan takes both, so the annotation also fires on correct code. The four warnings this diff works around are all that case.

It also reaches a shape outside the diff, where a function returning MakeByteSpan of a local std::span or std::string_view now gets -Wreturn-stack-address.

Would a requires (!std::ranges::borrowed_range<V>) on the annotated overload work? Compiled both ways here, the constrained form keeps the temporary-container warning and drops the two in net_tests. leveldb::Slice is not a borrowed range, so dbwrapper needs its own handling regardless.

Type	Reviewers
Concept ACK	jeanpablojp

in src/span.h:86 in 6591bf02ac

  82 | @@ -81,7 +83,7 @@ T& SpanPopBack(std::span<T>& span)
  83 |  }
  84 |  
  85 |  template <typename V>
  86 | -auto MakeByteSpan(const V& v) noexcept
  87 | +auto MakeByteSpan(const V& v LIFETIMEBOUND) noexcept

jeanpablojp commented at 10:31 PM on September 7, 2026:

Concretely this, plus a #include <ranges> at the top:

template <typename V> requires std::ranges::borrowed_range<V>
auto MakeByteSpan(const V& v) noexcept
{
    return std::as_bytes(std::span{v});
}
template <typename V> requires (!std::ranges::borrowed_range<V>)
auto MakeByteSpan(const V& v LIFETIMEBOUND) noexcept
{
    return std::as_bytes(std::span{v});
}

The first overload takes whatever std::ranges::borrowed_range accepts, std::span and std::string_view among them, and everything else takes the second. MakeUCharSpan takes the same pair, with the requires sitting alongside its decltype return.

in src/dbwrapper.cpp:419 in 6591bf02ac
```
 415 | @@ -416,12 +416,14 @@ std::span<const std::byte> CDBIterator::GetKeyImpl() const
 416 |  {
 417 |      // The returned span borrows from the current iterator entry and is only
 418 |      // valid until the iterator is advanced.
 419 | -    return MakeByteSpan(m_impl_iter->iter->key());
 420 | +    const leveldb::Slice key{m_impl_iter->iter->key()};
```
jeanpablojp commented at 10:31 PM on September 7, 2026:

Neither form keeps the helper here. MakeByteSpan(m_impl_iter->iter->key()) warns because the Slice is a temporary, and binding it to a local only changes the warning to "address of stack memory associated with local variable 'key' returned".

Would opting the Slice in be worth it? On top of those overloads it brings both getters back to the helper, and a stored MakeByteSpan of a temporary std::vector still warns. On its own it changes nothing, since the annotation as it stands is unconditional.

template<> inline constexpr bool std::ranges::enable_borrowed_range<leveldb::Slice> = true;

in src/test/serialize_tests.cpp:452 in 6591bf02ac

 447 | @@ -448,7 +448,8 @@ BOOST_AUTO_TEST_CASE(with_params_multi)
 448 |  //! Test creating a ParamsStream that moves from a stream argument.
 449 |  BOOST_AUTO_TEST_CASE(with_params_move)
 450 |  {
 451 | -    UncopyableStream stream{MakeByteSpan(std::string_view{"abc"})};
 452 | +    constexpr std::string_view abc{"abc"};
 453 | +    UncopyableStream stream{MakeByteSpan(abc)};

jeanpablojp commented at 10:31 PM on September 7, 2026:

UncopyableStream inherits the DataStream constructor that copies into vch, so no view is kept here. Reverted, the file still compiles clean.

    UncopyableStream stream{MakeByteSpan(std::string_view{"abc"})};

Contributors

kevkevinpal

DrahtBot

jeanpablojp

Linked (view graph)

#36164 util: diagnose dangling views of temporary strings

span: diagnose dangling views from MakeByteSpan/MakeUCharSpan #36183

Code Coverage & Benchmarks

Reviews