rpc: Disallow captures in RPCMethodImpl #34049
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34049. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
ajtowns
force-pushed
the
202512-rpcimpl-noref
branch
from
7064e93 to
48c3c41
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
lgtm. Seems fine to remove this silent footgun. I left a nit in the second commit, because I think it is fine to relax it a bit.
Also, while doing a global scripted-diff here, might as well do the rename RPCHelpMan to RPCMethod from #19386 (comment)?
src/wallet/rpc/spend.cpp
Outdated
| [want_psbt](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue | ||
| [](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue | ||
| { | ||
| const bool want_psbt = request.strMethod == "psbtbumpfee"; |
There was a problem hiding this comment.
nit in 48c3c41fa604b8634c509bb38d67de9bea2ab59d: I think the first commit makes a lot of sense and should be done. However, the second commit seems overly restrictive. Here, it is easy to re-create the calculation, but will it always be the case? I think it could make sense to either:
- drop the second commit, and let asan/valgrind disallow the dangling references, like they have done in the past
- Clarify the comment around the static_assert which is hit at compile time to say that any captured variables may have to be re-constructed a second time inside the lambda body
There was a problem hiding this comment.
Here, it is easy to re-create the calculation, but will it always be the case?
I think so -- the RpcMethodFnType functions don't take any params, so if you're doing a calculation (like a deprecatedrpc test) you can just do the same thing inside the RPCMethodImpl, and if you're providing something hardcoded, you can pass a boolean or enum via a template (eg template <bool wants_enum> static RPCHelpMan bumpfee_filter) and make your logic conditional on that.
There was a problem hiding this comment.
I dont think the second commit is necessary either should be fine with just removal of the captures
There was a problem hiding this comment.
Here, it is easy to re-create the calculation, but will it always be the case?
I think so -- the
RpcMethodFnTypefunctions don't take any params, so if you're doing a calculation (like a deprecatedrpc test) you can just do the same thing inside theRPCMethodImpl, and if you're providing something hardcoded, you can pass a boolean or enum via a template (egtemplate <bool wants_enum> static RPCHelpMan bumpfee_filter) and make your logic conditional on that.
Oh, I meant "succinct" instead of "easy". Imagine there was another (maybe even more verbose) const std::string incremental_fee{CFeeRate(DEFAULT_INCREMENTAL_RELAY_FEE).ToString(FeeEstimateMode::SAT_VB)};, which was used in both places (doc and impl). At some point it becomes tedious to keep the two in sync. Sure, it is possible to add a global function for it, but that also seems tedious.
We are somewhat stuck with C++ and can't use Rust, so it will be normal to use sanitizers. In this case, I think it is fine to defer to them and prefer the cleaner code. But no strong opinion, this is just my preference.
There was a problem hiding this comment.
Hmm; personally, I think if we can get compile time errors without too much hassle, that's much better than leaving it for CI to uncover.
Bumped the PR with a way of capturing succintly, but being very explicit about it when you're doing it.
(In that model, if you wanted to capture incremental_fee as well, you'd put both items of data into a struct Data { bool want_psbt; std::string incremental_fee_desc; } and write data.want_psbt instead of just want_psbt, etc)
Since this class defines the functionality of the RPC method, not just its help text, this better reflects reality. -BEGIN VERIFY SCRIPT- sed -i -e 's/\bRPCHelpMan\b/RPCMethod/g' $(git grep -l RPCHelpMan src/) -END VERIFY SCRIPT-
ajtowns
force-pushed
the
202512-rpcimpl-noref
branch
from
48c3c41 to
3cc9c87
Compare
-BEGIN VERIFY SCRIPT- sed -i 's/\[[&]\][(]const RPCMethod[&]/[](const RPCMethod\&/' $(git grep -l '\[\&\](const RPCMethod') -END VERIFY SCRIPT-
ajtowns
force-pushed
the
202512-rpcimpl-noref
branch
from
3cc9c87 to
aae82df
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
Capturing by reference in these functions is buggy (whatever it refers to is gone by the time the lambda returns), so disallow it.
ajtowns
force-pushed
the
202512-rpcimpl-noref
branch
from
aae82df to
cc1249c
Compare
4 participants
When defining
RPCHelpManobjects, we usually return a lambda, and mostly we define those via[&](...) { ... }which explicitly captures any parameters or local variables by reference. If we were to actually use any of those captures (we don't), we would invoke undefined behaviour. So instead, convert all the[&]to[]to avoid capturing, and as part ofRPCHelpMancheck that the function we provide is convertible to a bare function pointer, so that any attempts to capture anything (even if it's by-copy, which is safe) results in an error.While we're at it, rename
RPCHelpMantoRPCMethod, reflecting it's greater responsibility since #19386.