Skip to content

Conversation

stickies-v
Copy link
Contributor

Noticed from #25637 (comment) that BlockFilter::GetFilter() returns a reference to a member variable. Added LIFETIMEBOUND to all blockfilter-related code to ensure that the return values do not have a lifetime that exceeds the lifetime of what it is bound to. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lifetimebound or #25060 for a similar example.

I used grep -E '[a-zA-Z>0-9][&*] ([a-zA-Z]*)\((.*)\)' src/**/blockfilter* to grep all possible occurrences (not all of them require LIFETIMEBOUND)

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

ACK. Seems unlikely that this would ever be used in a way to violate lifetimes, but adding the attribute can't hurt.

Ensure that the return values do not have a lifetime that exceeds
the lifetime of what it is bound to.
See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lifetimebound
@stickies-v stickies-v force-pushed the blockfilter-add-lifetimebound branch from 60063f1 to 89576cc Compare August 31, 2022 15:51
@stickies-v
Copy link
Contributor Author

Rebase to fix CI failure - no other changes.

Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

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

crACK 89576cc

Coincidentally I was studying about LIFETIMEBOUND this morning. Nice catch!

@maflcko maflcko merged commit f821fc9 into bitcoin:master Sep 1, 2022
@stickies-v stickies-v deleted the blockfilter-add-lifetimebound branch September 1, 2022 11:27
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 1, 2022
… needed

89576cc refactor: add LIFETIMEBOUND to blockfilter where needed (stickies-v)

Pull request description:

  Noticed from bitcoin#25637 (comment) that [`BlockFilter::GetFilter()`](https://github.com/bitcoin/bitcoin/blob/01e1627e25bc5477c40f51da03c3c31b609a85c9/src/blockfilter.h#L132) returns a reference to a member variable. Added LIFETIMEBOUND to all blockfilter-related code to ensure that the return values do not have a lifetime that exceeds the lifetime of what it is bound to. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lifetimebound or bitcoin#25060 for a similar example.

  I used `grep -E '[a-zA-Z>0-9][&*] ([a-zA-Z]*)\((.*)\)' src/**/blockfilter*` to grep all possible occurrences (not all of them require LIFETIMEBOUND)

ACKs for top commit:
  brunoerg:
    crACK 89576cc

Tree-SHA512: 6fe61fc0c1ed9e446edce083d1b093e1a5e2ef8c39ff74125bb12a24e514d45711845809817fbd4a04d7a9c23c8b362203771c17b6d831d2560b1af268453019
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Sep 16, 2022
… index names

26cf9ea scripted-diff: rename pszThread to thread_name (stickies-v)
200d84d refactor: use std::string for index names (stickies-v)
97f5b20 refactor: use std::string for thread names (stickies-v)

Pull request description:

  As a follow-up to bitcoin/bitcoin#25967 (comment), this PR changes the return type of [`BaseIndex::GetName()`](https://github.com/bitcoin/bitcoin/blob/fa5c224d444802dabec5841009e029b9754c92f1/src/index/base.h#L120) to `const std::string&` instead of `const char*`. The first commit is not essential for this change, but since the code is touched and index names are commonly used to specify thread names, I've made the same update there.

  No behaviour change, just refactoring to further phase out C-style strings.

  Note: `util::ThreadRename()` used to take an rvalue ref, but since it then passes this to `SetInternalName()` by value, I don't think there's any benefit to having both an rvalue and lvalue ref function so I just changed it into lvalue ref. Not 100% sure I'm missing something?

ACKs for top commit:
  MarcoFalke:
    review ACK 26cf9ea only change is new scripted-diff 😀
  hebasto:
    ACK 26cf9ea, I have reviewed the code and it looks OK.
  w0xlt:
    reACK bitcoin/bitcoin@26cf9ea

Tree-SHA512: 44a03ebf2bb86ca1411a36222a575217cdba8ee3a3c985e74d74c934516f002b27336147fa22f59eda7dac21204a93951563317005d475da95b23c427014d77b
@bitcoin bitcoin locked and limited conversation to collaborators Sep 1, 2023
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.

4 participants