span: Add lifetimebound attribute to guard against temporary lifetime issues #19382

1. 
   theuni commented at 10:42 pm on June 25, 2020: member

   See here for more detail on lifetimebound.

   This is implemented using preprocesor macros rather than configure checks in order to keep span.h self-contained.

   The [[clang::lifetimebound]] syntax was chosen over __attribute__((lifetimebound)) because the former is more flexible and works to guard this as well as function parameters, and also because at least for now, it’s available only in clang.

   There are currently no violations in our codebase, but this can easily be tested by inserting one like this somewhere and compiling with a modern clang:

   0Span<const int> bad(std::vector<int>{1,2,3});
   

   The result:

   warning: temporary whose address is used as value of local variable ‘bad’ will be destroyed at the end of the full-expression [-Wdangling] Span bad(std::vector{1,2,3});

2. span: remove default copy ctor and assignment oper

   These have the effect of disabling the default move constructor and move
   operator. This is generally meaningless for spans due to their simplicity,
   but consider the (simplified here) internal implementation of .first():
   
       Span<C> first(std::size_t count) const noexcept {
           return Span<C>(m_data, count);
       }
   
   Technically this creates a new Span object and moves/copies it when it returns,
   but RVO takes care of optimizing that out.
   
   Because trivial moves have been implicitly deleted, it falls into the more
   complicated conversion rvalue constructor to handle the return.
   
   The next commit will add an attribute to the conversion constructor to help
   detect dangling stack references, so we want to make sure that we're not
   sending trivial copies through it.
   
   NOTE: This copy/move is could also be avoided by constructing the return value
   in-place:
       Span<C> first(std::size_t count) const noexcept {
           return {m_data, count};
       }

   397e5eda38
3. span: add lifetimebound attribute

   See http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0936r0.pdf for
   reference.
   
   This helps to guard against dangling references caused by construction from
   temporaries such as:
   
       Span<const int> sp(std::vector<int>{1,2,3});

   e53b5ab37c
4. in src/span.h:21 in e53b5ab37c

   17@@ -18,6 +18,12 @@
   18 #define ASSERT_IF_DEBUG(x)
   19 #endif
   20 
   21+#if defined(__clang__) && __has_attribute(lifetimebound)
   

   ---

   ---

   

   MarcoFalke commented at 11:49 pm on June 25, 2020:

   This seems to fail on gcc 4.8:

    0libtool: compile:  /usr/bin/ccache g++ -m32 -std=c++11 -DHAVE_CONFIG_H -I. -I../src/config -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I./obj -I./secp256k1/include -DBUILD_BITCOIN_INTERNAL -I/home/travis/build/bitcoin/bitcoin/depends/i686-pc-linux-gnu/share/../include/ -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -D_FILE_OFFSET_BITS=64 -fstack-reuse=none -Wstack-protector -fstack-protector-all -pipe -O2 -fno-extended-identifiers -fvisibility=hidden -c consensus/merkle.cpp  -fPIC -DPIC -o consensus/.libs/libbitcoinconsensus_la-merkle.o
    1
    2In file included from ./serialize.h:25:0,
    3
    4                 from ./script/script.h:11,
    5
    6                 from ./primitives/transaction.h:11,
    7
    8                 from ./primitives/block.h:9,
    9
   10                 from ./consensus/merkle.h:10,
   11
   12                 from consensus/merkle.cpp:5:
   13
   14./span.h:21:42: error: missing binary operator before token "("
   15
   16 #if defined(__clang__) && __has_attribute(lifetimebound)
   17
   18                                          ^
   

   ---

   

   theuni commented at 1:46 am on June 26, 2020:

   Thanks, will break this up into nested if’s.
5. in src/span.h:68 in e53b5ab37c

   66@@ -61,12 +67,6 @@ class Span
   67     template <typename O, typename std::enable_if<std::is_convertible<O (*)[], C (*)[]>::value, int>::type = 0>
   68     constexpr Span(const Span<O>& other) noexcept : m_data(other.m_data), m_size(other.m_size) {}
   69 
   70-    /** Default copy constructor. */
   71-    constexpr Span(const Span&) noexcept = default;
   72-
   73-    /** Default assignment operator. */
   74-    Span& operator=(const Span& other) noexcept = default;
   

   ---

   ---

   

   MarcoFalke commented at 0:06 am on June 26, 2020:

   in commit 397e5eda38144147330163e7a51413ac01d1d6f5

   It looks like std::span has no move constructor (https://en.cppreference.com/w/cpp/container/span/span), so shouldn’t we do the same?

   ---

   

   theuni commented at 1:57 am on June 26, 2020:

   Hmm. I’m seeing conflicting info on this.

   According to the (official?) revision 7 draft, the move constructor was removed in r6. See here: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0122r7.pdf

   If we were to follow that, I believe we would drop our fun rvalue ctor.

   I’m not sure what draft version cppreference is referencing, but I believe it must be an old one.

   libc++ for example has implemented r7: https://reviews.llvm.org/D49338.

   Will explore more.

   ---

   

   MarcoFalke commented at 11:38 am on June 26, 2020:

   libc++ for example has implemented r7: https://reviews.llvm.org/D49338.

   looking at the source code, it shows they are using = default, no?

   https://github.com/llvm/llvm-project/blame/6fafde0387229d6656faee41100b73615343819a/libcxx/include/span#L70

   ---

   

   theuni commented at 6:03 pm on June 26, 2020:

   @MarcoFalke Yes, but they also don’t have a universal reference constructor like we do.

   The problem is that return values are passed through a move constructor if they exist, before copy constructors. See here under “Automatic move from local variables and parameters”.

   Because the default move constructor is removed by adding the default constructor/assignment operator, our return values are passed through our universal constructor as rvalues. That’s really not a problem except that we want to minimize what’s going through that constructor so that we can apply the lifetimebound attribute to it.

   ( Bonus TIL: apparently c++17 does away with this return value path altogether when possible: “If expression is a prvalue, the result object is initialized directly by that expression. This does not involve a copy or move constructor when the types match” )

   So there are 2 possible solutions for us (if you’re willing to accept that there’s something to be solved)

   1. remove the default ctor/assignment operator as I’ve done here, thus enabling a trivial move constructor for return values to pass through
   2. remove our universal reference constructor and replace it with something less greedy

   Your question made me look up the current c++20 draft spec, where I noticed that the universal reference constructor has been removed in a newish revision. So what’s on cppreference is actually stale.

   But that gives us an easy answer to the question above, we should just update our implementation to match the latest draft spec. I’m going to close this and open a new PR with that approach, since it’s sufficiently different from what’s here.

6. 
   MarcoFalke commented at 0:07 am on June 26, 2020: member

   Concept ACK. The compile time warning will go away (just like the ASSERT_IF_DEBUG) if we upgrade to std::span, but it seems fine to have additional safeguards in place in the meantime.
7. 
   theuni commented at 2:02 am on June 26, 2020: member

   The compile time warning will go away (just like the ASSERT_IF_DEBUG) if we upgrade to std::span, but it seems fine to have additional safeguards in place in the meantime.

   Since clang has already has it implemented, presumably libc++ will ship with this attribute (or keyword) wired up once the standard is finished.

8. 
   jonatack commented at 7:18 am on June 26, 2020: member

   Concept ACK.
9. theuni closed this on Jun 26, 2020

   ---

10. MarcoFalke referenced this in commit 5c0aebfcd4 on Nov 25, 2020
11. DrahtBot locked this on Feb 15, 2022

Contributors
theuni MarcoFalke jonatack