Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jun 18, 2021

Without this, stack-use-after-scope can only be detected at runtime with ASan or code review, both of which are expensive.

Use LIFETIMEBOUND to turn this error into a compile warning.

See https://releases.llvm.org/12.0.0/tools/clang/docs/AttributeReference.html#lifetimebound

Example:

const CScript a{WITH_LOCK(::cs_main, return CScript{} << OP_0 << OP_1)};

Before: (no warning)
After:

warning: returning reference to local temporary object [-Wreturn-stack-address]
    const CScript a{WITH_LOCK(::cs_main, return CScript{} << OP_0 << OP_1)};
                                                ^~~~~~~~~
./sync.h:276:65: note: expanded from macro 'WITH_LOCK'
#define WITH_LOCK(cs, code) [&]() -> decltype(auto) { LOCK(cs); code; }()
                                                                ^~~~

@sipa
Copy link
Member

sipa commented Jun 18, 2021

Interesting, I wouldn't have considered using lifetimebound for those, but it makes sense.

utACK fabcaf3.

My understanding is that this cannot introduce bugs if it compiles with issues, as the attribute only adds warning/errors.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 18, 2021

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

Conflicts

No conflicts as of last run.

@fanquake
Copy link
Member

fanquake commented Sep 2, 2021

@theuni want to take a look here?

@sipa
Copy link
Member

sipa commented Sep 2, 2021

utACK

@theuni
Copy link
Member

theuni commented Sep 2, 2021

Concept ACK/utACK. I guess ideally we'd annotate anywhere we return references to *this or *this.foo?

I tried switching the return type from decltype(auto) to auto, which works around this problem by allowing copies to be made as a typical return would, but that disables the ability to bind to a legit returned reference. However, it does have the nice side-effect of showing where these annotations would be useful :)

The one it turned up is ChainstateManager::InitializeChainstate, maybe add an annotation there as well?

@maflcko
Copy link
Member Author

maflcko commented Sep 3, 2021

Concept ACK/utACK. I guess ideally we'd annotate anywhere we return references to *this or *this.foo?

Correct. I think where foo is a (trivial?) built-in type, the annotation isn't needed because at least clang will figure it out by itself. Though, it shouldn't hurt, so maybe someone writes a clang-tidy script to do the refactor in a follow-up?

The one it turned up is ChainstateManager::InitializeChainstate, maybe add an annotation there as well?

Thanks, added a commit. Can be tested with this snippet:

warning: returning reference to local temporary object [-Wreturn-stack-address]
    CChainState& c1 = WITH_LOCK(::cs_main, return ChainstateManager{}.InitializeChainstate(&mempool));
                                                  ^~~~~~~~~~~~~~~~~~~
./sync.h:276:65: note: expanded from macro 'WITH_LOCK'
#define WITH_LOCK(cs, code) [&]() -> decltype(auto) { LOCK(cs); code; }()
                                                                ^~~~
1 warning generated.

@theuni
Copy link
Member

theuni commented Sep 3, 2021

utACK fa7e6c5.

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Light ACK fa7e6c5 debug build with clang 13, reproduced the example compiler warning in the pull description, and briefly looked at clang::lifetimebound support in earlier versions of clang; it is in clang 7 (https://releases.llvm.org/7.0.0/tools/clang/docs/AttributeReference.html#lifetimebound-clang-lifetimebound), did not see references to it in earlier docs

@fanquake fanquake merged commit f4e12fd into bitcoin:master Sep 4, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 4, 2021
fa7e6c5 Add LIFETIMEBOUND to InitializeChainstate (MarcoFalke)
fa5c896 Add LIFETIMEBOUND to CScript where needed (MarcoFalke)

Pull request description:

  Without this, stack-use-after-scope can only be detected at runtime with ASan or code review, both of which are expensive.

  Use `LIFETIMEBOUND` to turn this error into a compile warning.

  See https://releases.llvm.org/12.0.0/tools/clang/docs/AttributeReference.html#lifetimebound

  Example:

  ```cpp
  const CScript a{WITH_LOCK(::cs_main, return CScript{} << OP_0 << OP_1)};
  ```

  Before: (no warning)
  After:

  ```
  warning: returning reference to local temporary object [-Wreturn-stack-address]
      const CScript a{WITH_LOCK(::cs_main, return CScript{} << OP_0 << OP_1)};
                                                  ^~~~~~~~~
  ./sync.h:276:65: note: expanded from macro 'WITH_LOCK'
  #define WITH_LOCK(cs, code) [&]() -> decltype(auto) { LOCK(cs); code; }()
                                                                  ^~~~

ACKs for top commit:
  theuni:
    utACK fa7e6c5.
  jonatack:
    Light ACK fa7e6c5 debug build with clang 13, reproduced the example compiler warning in the pull description, and briefly looked at `clang::lifetimebound` support in earlier versions of clang; it is in clang 7 (https://releases.llvm.org/7.0.0/tools/clang/docs/AttributeReference.html#lifetimebound-clang-lifetimebound), did not see references to it in earlier docs

Tree-SHA512: e915acdc4532445205b7703fab61a5d682231ace78ecfb274cb8523ca2bddefd85828f50ac047cfb1afaff92a331f5f7b5a1472539f999e30f7cf8ac8c3222f3
@maflcko maflcko deleted the 2106-scriptBound branch September 5, 2021 15:35
maflcko pushed a commit that referenced this pull request May 4, 2022
…)::start_time

4cb9d21 blockstorage: add LIFETIMEBOUND to GetFirstStoredBlock()::start_time (Jon Atack)

Pull request description:

  Suggested in #25016 (comment), the lifetimebound attribute here indicates that a resource owned by the `start_block` param of `CBlockIndex* BlockManager::GetFirstStoredBlock()` can be retained by the method's return value, which enables detecting the use of out-of-scope stack memory (ASan `stack-use-after-scope`) at compile time.

  See https://releases.llvm.org/12.0.0/tools/clang/docs/AttributeReference.html#lifetimebound and #22278 for related discussion, and #25040 for a similar example.

ACKs for top commit:
  MarcoFalke:
    review ACK 4cb9d21

Tree-SHA512: a3f5ef83ebb6f08555d7c89f2437a682071b4ad77a7aa3326b6d2282c909bf9fcf4dac6bf05ee1d9931f2102cad4a02df5468bde1cf377d7126e84e8541604dc
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 4, 2022
…dBlock()::start_time

4cb9d21 blockstorage: add LIFETIMEBOUND to GetFirstStoredBlock()::start_time (Jon Atack)

Pull request description:

  Suggested in bitcoin#25016 (comment), the lifetimebound attribute here indicates that a resource owned by the `start_block` param of `CBlockIndex* BlockManager::GetFirstStoredBlock()` can be retained by the method's return value, which enables detecting the use of out-of-scope stack memory (ASan `stack-use-after-scope`) at compile time.

  See https://releases.llvm.org/12.0.0/tools/clang/docs/AttributeReference.html#lifetimebound and bitcoin#22278 for related discussion, and bitcoin#25040 for a similar example.

ACKs for top commit:
  MarcoFalke:
    review ACK 4cb9d21

Tree-SHA512: a3f5ef83ebb6f08555d7c89f2437a682071b4ad77a7aa3326b6d2282c909bf9fcf4dac6bf05ee1d9931f2102cad4a02df5468bde1cf377d7126e84e8541604dc
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants