-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Add LIFETIMEBOUND to CScript where needed #22278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Interesting, I wouldn't have considered using utACK fabcaf3. My understanding is that this cannot introduce bugs if it compiles with issues, as the attribute only adds warning/errors. |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
@theuni want to take a look here? |
utACK |
Concept ACK/utACK. I guess ideally we'd annotate anywhere we return references to I tried switching the return type from The one it turned up is |
Correct. I think where
Thanks, added a commit. Can be tested with this snippet:
|
9e429f0
to
fa7e6c5
Compare
utACK fa7e6c5. |
There was a problem hiding this 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
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
…)::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
…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
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:
Before: (no warning)
After: