refactor: Enforce C-str fmt strings in WalletLogPrintf() #28237
Conversation
* Move module description from test to LogPrintfCheck * Add test doc * Remove unused comment, see https://github.com/bitcoin/bitcoin/pull/26296/files#r1279351539
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
DrahtBot
changed the title
maflcko
force-pushed
the
2308-wallet-c-str-
branch
from
faf82ca to
faa8ff0
Compare
maflcko
force-pushed
the
2308-wallet-c-str-
branch
from
faa8ff0 to
faf0575
Compare
maflcko
force-pushed
the
2308-wallet-c-str-
branch
from
faf0575 to
fa6dc57
Compare
| { | ||
| LOCK(cs_wallet); | ||
| WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); | ||
| WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); // NOLINT(bitcoin-unterminated-logprintf) |
There was a problem hiding this comment.
Hmm, this one wasn't being caught before?
There was a problem hiding this comment.
Hmm.
I haven't tried this using the c-i scripts, but running clang-tidy manually against current master, this works fine for me as-is:
$ /opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang-tidy-16 --load=/home/cory/dev/bitcoin2/contrib/devtools/bitcoin-tidy/build2/libbitcoin-tidy.so -checks="-*,bitcoin-*" wallet/wallet.cpp -- -DHAVE_BUILD_INFO -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -isystem /home/cory/dev/bitcoin2/depends/x86_64-pc-linux-gnu/include -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -DHAVE_CONFIG_H -std=c++17
1 warning generated.
/home/cory/dev/bitcoin2/src/wallet/wallet.cpp:2322:21: error: Unterminated format string used with LogPrintf [bitcoin-unterminated-logprintf,-warnings-as-errors]
WalletLogPrintf("CommitTransaction:\n%s", tx->ToString());
^
\n
1 warning treated as errorAny idea what the difference is?
There was a problem hiding this comment.
@theuni You'll have to share exact steps to reproduce, ideally starting from a fresh install of you operating system. Otherwise it will be close to impossible to reproduce. Locally and on CI this isn't caught (otherwise CI would be red). c.f
$ (cd src/ && clang-tidy --load=/home/fake/bitcoin/contrib/devtools/bitcoin-tidy/build/libbitcoin-tidy.so -checks="-*,bitcoin-*" wallet/wallet.cpp )
12 warnings generated.
Suppressed 12 warnings (12 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
There was a problem hiding this comment.
Huh. For whatever reason, my clang-tidy-16 acts as if IgnoreUnlessSpelledInSource is set. That's weird.
I can't reproduce with clang-tidy-15. clang-query-16 only matches if traversal is set to IgnoreUnlessSpelledInSource.
$ /opt/clang+llvm-15.0.2-x86_64-unknown-linux-gnu/bin/clang-query wallet/wallet.cpp -- -DHAVE_BUILD_INFO -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -isystem /home/cory/dev/bitcoin2/depends/x86_64-pc-linux-gnu/include -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -DHAVE_CONFIG_H -std=c++17 -O2
clang-query> m cxxMemberCallExpr(thisPointerType(qualType(hasDeclaration(cxxRecordDecl(hasName("CWallet"))))),callee(cxxMethodDecl(hasName("WalletLogPrintf"))),hasArgument(0, stringLiteral().bind("logstring")))
0 matches.
clang-query> set traversal IgnoreUnlessSpelledInSource
clang-query> m cxxMemberCallExpr(thisPointerType(qualType(hasDeclaration(cxxRecordDecl(hasName("CWallet"))))),callee(cxxMethodDecl(hasName("WalletLogPrintf"))),hasArgument(0, stringLiteral().bind("logstring")))...
Match #33:
/home/cory/dev/bitcoin2/src/wallet/wallet.cpp:4149:36: note: "logstring" binds here
wallet.WalletLogPrintf("Making a new watchonly wallet containing the unwatched solvable scripts\n");
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/cory/dev/bitcoin2/src/wallet/wallet.cpp:4149:13: note: "root" binds here
wallet.WalletLogPrintf("Making a new watchonly wallet containing the unwatched solvable scripts\n");
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
33 matches
I don't think this is worth spending any more time on, but I still don't understand why it's different.
theuni
left a comment
theuni
left a comment
There was a problem hiding this comment.
Makes sense to me. I almost did the same thing while poking at WalletLogPrintf for the sake of easier static analysis.
Am I missing some obvious reason though, other than "might as well"?
| void WalletLogPrintf(std::string fmt, Params... parameters) const { | ||
| LogPrintf(("%s " + fmt).c_str(), GetDisplayName(), parameters...); | ||
| template <typename... Params> | ||
| void WalletLogPrintf(const char* fmt, Params... parameters) const |
There was a problem hiding this comment.
I was going to suggest the nonnull attribute here, but I believe our tidy check actually enforces that :)
There was a problem hiding this comment.
I hope that the tests (and reviewers) will catch a LogPrint(nullptr, 1, 2, 3). But adding nonnull can be done in a follow-up, if needed, for all log-functions.
Edited OP to list both reasons. |
This makes it easier to add new ones without having to modify this file every time.
|
ACK fa60fa3 |
4 participants
All fmt functions only accept a raw C-string as argument.
There should never be a need to pass a format string that is not a compile-time string literal, so disallow it in
WalletLogPrintf()to avoid accidentally introducing it.Apart from consistency, this also fixes the clang-tidy plugin bug #26296 (comment).