Add compile-time checking for literal format strings passed to strprintf and tfm::format to make sure the right number of format arguments are passed.
There is still no compile-time checking if non-literal std::string or bilingual_str format strings are passed, but this is improved in other PRs:
This is needed in the next commit to add compile-time checking to strprintf
calls, because bitcoin-cli.cpp uses dynamic width in many format strings.
This change is easiest to review ignoring whitespace.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31174.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, l0rinc, hodlinator |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
68+ PassFmt<3>("%2$*3$.9d");
69+ PassFmt<3>("%2$.*3$d");
70+ PassFmt<3>("%2$9.*3$d");
71+ PassFmt<3>("%2$+9.*3$d");
72+ PassFmt<4>("%3$*2$.*4$f");
73
nit in 97dd5fe5128592332c83998825bbeda063815120: in the commit message: Missing needed *in the* next commit.
Also, it would be good to add some failing test cases for this stuff.
For example, "%1" should fail, because terminate called after throwing an instance of 'tinyformat::format_error'
what(): tinyformat: Conversion spec incorrectly terminated by end of string. See https://godbolt.org/z/1eehh1oTv . Also see 51c56e8b38033b96fb3930c2bcba3add2047d324
"%*", "%+*", "%.*", …
Also, some more “fancy” passing test cases:
PassFmt<3>("'%1$- 0+*3$.*2$f'"); (space and 0 are ignored, confusing, but valid)PassFmt<3>("'%- 0+*.*f'"); (same, without pos specs)
re: #31174 (review)
Thanks, I added the suggested test cases. The suggested tests that “should fail” didn’t actually fail with 97dd5fe5128592332c83998825bbeda063815120 because it accepted \0 as a valid specifier character, so I added an extra check to prevent that. I also added extra code to consume digits after . otherwise format strings like "%1.2" would be accepted treating 2 as the specifier.
178@@ -178,6 +179,23 @@ namespace tfm = tinyformat;
179
180 namespace tinyformat {
181
182+// Added for Bitcoin Core. Wrapper type for format strings to allow compile-time checks.
183+struct FormatString {
184+ const char* fmt;
185+ operator const char*() { return fmt; }
186+};
Nit in 1d16d6e6bac994fed7c695f530b9984edcd290bd: I don’t really see why this is useful. Maybe I am missing something, but everything compiles by just moving the two members to the derived struct:
0diff --git a/src/tinyformat.h b/src/tinyformat.h
1index 56c25d8f7f..6227cdeca0 100644
2--- a/src/tinyformat.h
3+++ b/src/tinyformat.h
4@@ -179,21 +179,16 @@ namespace tfm = tinyformat;
5
6 namespace tinyformat {
7
8-// Added for Bitcoin Core. Wrapper type for format strings to allow compile-time checks.
9-struct FormatString {
10- const char* fmt;
11- operator const char*() { return fmt; }
12-};
13-
14 // Added for Bitcoin Core. Wrapper for checking format strings at compile time.
15 // Unlike ConstevalFormatString this supports std::string for runtime string
16 // formatting without compile time checks.
17 template <unsigned num_params>
18-struct FormatStringCheck : FormatString {
19- consteval FormatStringCheck(const char* str) : FormatString{util::ConstevalFormatString<num_params>{str}.fmt} {}
20- FormatStringCheck(const std::string& str) : FormatString{str.c_str()} {}
21- FormatStringCheck(FormatString str) : FormatString{str} {}
22- FormatStringCheck(util::ConstevalFormatString<num_params> str) : FormatString{str.fmt} {}
23+struct FormatStringCheck {
24+ consteval FormatStringCheck(const char* str) : fmt{util::ConstevalFormatString<num_params>{str}.fmt} {}
25+ FormatStringCheck(const std::string& str) : fmt{str.c_str()} {}
26+ FormatStringCheck(util::ConstevalFormatString<num_params> str) : fmt{str.fmt} {}
27+ const char* fmt;
28+ operator const char*() { return fmt; }
29 };
30
31 // Added for Bitcoin Core
re: #31174 (review)
Thanks, applied patch. The reason for having a FormatString class was to provide a cleaner escape from compile-time checks strprintf(FormatString{"%*s"}, width, str) before the first commit was implemented. But it’s no longer necessary after that commit.
54+ auto add_arg = [&] {
55+ unsigned maybe_num{0};
56+ while ('0' <= *it && *it <= '9') {
57+ maybe_num *= 10;
58+ maybe_num += *it - '0';
59+ ++it;
re: #31174 (review)
we still have unbounded increment without checking the end (I though we’ve fixed this already, maybe it got stuck in the comments…)
Thanks. I fixed this by just switching the string type to const char* instead of string_view since tinyformat already assumes the string is null terminated.
I think it would be possible to write a clean version of this code that used string_view, but it would have to be structured differently to avoid the need for it < str.end() checks everywhere.
Found the PR thanks to @hodlinator, this is how I handled that case: https://github.com/bitcoin/bitcoin/pull/30999/files#diff-71badc1cc71ba46244f7841a088251bb294265f4fe9e662c0ad6b15be540eee4R60
We could a test case for the incomplete trailing number (or any other test that seems relevant here): https://github.com/bitcoin/bitcoin/pull/30999/files#diff-718d0d85269ec81595bd9f9181eea3a74b20244b07f14c546e3e07520b2b5f82R81
pr/tcheck.1 -> pr/tcheck.2, compare) addressing comments and making ConstEvalFormatString parsing stricter to reject incomplete specifiers.
Updated e6086e00e32e486aaeeeb346ccca1377bbf647b2 -> e53829d3952c6ed275507a66e77636aad67d106b (pr/tcheck.2 -> pr/tcheck.3, compare) cleaning up whitespace and comments.
65+ unsigned maybe_num{0};
66+ while ('0' <= *it && *it <= '9') {
67+ maybe_num *= 10;
68+ maybe_num += *it - '0';
69+ ++it;
70+ };
0 }
re: #31174 (review)
Thanks, updated
Concept ACK e53829d3952c6ed275507a66e77636aad67d106b
Cleanest attempt at increased compile time validation of format so far. When reviewing #31149 I had the gnawing feeling that more complete format string support would have reduced the diff, but pushed it away for expediency (an earlier attempt at more complete support was attempted in #30999).
consteval checking and tinyformat are gone as far as our current tests go - rebased commit: 32d4cf37d5056dc42bbf317b059e10910b984b0e
re: #31174 (comment)
mismatches between our custom
constevalchecking and tinyformat are gone as far as our current tests go
In case you do want a test with different behavior, I think you can use %n specifier which is not supported by tinyformat
113 }
114+
115+ if (*it == '\0') throw "Format specifier incorrectly terminated by end of string";
116+
117 // The remainder "[flags][width][.precision][length]type" of the
118 // specifier is not checked. Parsing continues with the next '%'.
re: #31174 (review)
Should this be removed?
I think it’s useful to document the format of the specifier, and it’s still true that the remainder of the specifier (length and type) is not checked. Happy to update this if there’s a specific suggestion, but I think the comment is still helpful and accurate so would not want to remove it.
As you say, only the length and type are no longer checked. This PR now implements rudimentary checking of flags, width and precision.
0 // Length and type in "[flags][width][.precision][length]type"
1 // is not checked. Parsing continues with the next '%'.
re: #31174 (review)
As you say, only the length and type are no longer checked. This PR now implements rudimentary checking of flags, width and precision.
Thanks, applied suggestion
106 ++it;
107+ if (*it == '*') {
108+ ++it;
109+ add_arg();
110+ } else {
111+ while ('0' <= *it && *it <= '9') ++it;
re: #31174 (review)
(One could expect that at least one precision-digit was required after ‘.’ but it is not in tinyformat so this behavior is consistent).
That’s good to know, I was just trying to make the parsing as simple as possible, but this syntax does seem to be commonly accepted (it works in python too) and I added test cases for it following your other suggestion
1077
1078 /// Format list of arguments according to the given format string and return
1079 /// the result as a string.
1080 template<typename... Args>
1081-std::string format(const char* fmt, const Args&... args)
1082+std::string format(FormatStringCheck<sizeof...(Args)> fmt, const Args&... args)
FormatStringCheck<sizeof...(Args)> in printf and printfln directly below as well?
re: #31174 (review)
Why not use
FormatStringCheck<sizeof...(Args)>inprintfandprintflndirectly below as well?
No reason, added now.
60- PassFmt<1>("%*c");
61- PassFmt<2>("%2$*3$d");
62- PassFmt<1>("%.*f");
63+ PassFmt<2>("%*c");
64+ PassFmt<2>("%+*c");
65+ PassFmt<2>("%.*f");
Could also add
0PassFmt<1>("%.f");
maybe grouped with the <1> calls above. Confirmed tinyformat allows for it, see how the success status returned by parseWidthOrPrecision is ignored:
https://github.com/bitcoin/bitcoin/blob/e53829d3952c6ed275507a66e77636aad67d106b/src/tinyformat.h#L778-L788
In case you do want a test with different behavior, I think you can use %n specifier which is not supported by tinyformat
Could document non-parity like so (unless you prefer I do it as part of #30933):
0 // Non-parity
1 int n{};
2 BOOST_CHECK_EXCEPTION(tfm::format(std::string{"%n"}, n), tfm::format_error,
3 HasReason{"tinyformat: %n conversion spec not supported"});
4 ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%n");
Updated e53829d3952c6ed275507a66e77636aad67d106b -> ecc5cb9a89c6b001df839675b23d8fc1f7ac69ba (pr/tcheck.3 -> pr/tcheck.4, compare) with review suggestions.
re: #31174#pullrequestreview-2405166236
Could document non-parity like so (unless you prefer I do it as part of #30933):
I think that change doesn’t really fit into this PR, since this PR isn’t checking type characters. But it does make sense as part of #30933, so would be good to add there and I’d be happy to review it.
ACK ecc5cb9a89c6b001df839675b23d8fc1f7ac69ba
Implemented my suggestions (except comment removal suggestion) + broke out parse_size() since my last review.
util_string_tests tests passed locally.
Left one comment, but nothing blocking.
88@@ -79,7 +89,18 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
89
Could add new failing cases here as well?
0FailFmtWithError<2>("%2$*$d", err_0_pos);
1FailFmtWithError<2>("%2$*0$d", err_0_pos);
2FailFmtWithError<3>("%3$*2$.*$f", err_0_pos);
3FailFmtWithError<3>("%3$*2$.*0$f", err_0_pos);
re: #31174 (review)
Thanks, added
76+ // Make sure multiple flag characters "- 0+" are accepted
77+ PassFmt<3>("'%- 0+*.*f'");
78+ PassFmt<3>("'%1$- 0+*3$.*2$f'");
79
80 auto err_mix{"Format specifiers must be all positional or all non-positional!"};
81 FailFmtWithError<1>("%s%1$s", err_mix);
Same:
0FailFmtWithError<2>("%2$*d", err_mix);
1FailFmtWithError<2>("%*2$d", err_mix);
2FailFmtWithError<2>("%.*3$d", err_mix);
3FailFmtWithError<2>("%2$.*d", err_mix);
Same (below):
0 FailFmtWithError<1>("%*c", err_num);
re: #31174 (review)
Thanks, added
left some nit ideas for more tests, but this is good either way.
review ACK ecc5cb9a89c6b001df839675b23d8fc1f7ac69ba 🕯
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK ecc5cb9a89c6b001df839675b23d8fc1f7ac69ba 🕯
3+oFB4Q8dHdvzp6J/1Ir4akTLS5GbDLpGOeKvcRP31CsusrUqTTnwOMie2fGfDcGYiEyKkNN9JiriK4ne+GSICw==
97+ auto parse_size = [&] {
98+ if (*it == '*') {
99+ ++it;
100+ add_arg();
101+ } else {
102+ while ('0' <= *it && *it <= '9') ++it;
Given that we have two separate number “parsers” (one that keeps the result and one that throws it away), we might as well extract number parsing to a local lambda like you did with the other ones.
0diff --git a/src/util/string.h b/src/util/string.h
1--- a/src/util/string.h (revision ecc5cb9a89c6b001df839675b23d8fc1f7ac69ba)
2+++ b/src/util/string.h (date 1731267170701)
3@@ -45,14 +45,16 @@
4 continue;
5 }
6
7+ auto parse_number = [&] {
8+ unsigned num{0};
9+ for (; '0' <= *it && *it <= '9'; ++it) {
10+ num = num * 10 + (*it - '0');
11+ }
12+ return num;
13+ };
14+
15 auto add_arg = [&] {
16- unsigned maybe_num{0};
17- while ('0' <= *it && *it <= '9') {
18- maybe_num *= 10;
19- maybe_num += *it - '0';
20- ++it;
21- }
22-
23+ unsigned maybe_num = parse_number();
24 if (*it == '$') {
25 ++it;
26 // Positional specifier, like %8$s
27@@ -75,7 +77,7 @@
28 ++it;
29 add_arg();
30 } else {
31- while ('0' <= *it && *it <= '9') ++it;
32+ parse_number();
33 }
34 };
re: #31174 (review)
we might as well extract number parsing to a local lambda like you did with the other ones.
This seems reasonable but I"m not sure it’s clearer, and it does make the diff bigger replacing the maybe_num code that doesn’t have to change currently. Happy to apply this suggestion if other reviews think it is a good idea.
while ('0' <= *it && *it <= '9') ++it; is fine. It is pretty standard self-explanatory code. I don’t think a one-line while loop needs to be de-duplicated. Also, I like that the diff is minimal as-is now.
52- if (++it >= str.end()) throw "Format specifier incorrectly terminated by end of string";
53 if (*it == '%') {
54 // Percent escape: %%
55- ++it;
56 continue;
57 }
These are all here to check if we’re inside a format string, but we don’t have a %%, right?
Could we maybe simplify that to something like:
0 if (*it != '%' || *(++it) == '%') continue; // Skip escaped %%
?
re: #31174 (review)
Nice suggestion! Applied
90- ++count_normal;
91+ // Increase argument count and consume positional specifier, if present.
92+ add_arg();
93+
94+ // Consume flags.
95+ while (*it == '#' || *it == '0' || *it == '-' || *it == ' ' || *it == '+') ++it;
In C++23 this could be a simple .contains, but even in C++20 we should be able to group the flags to something like:
0 while ("#0- +"sv.find(*it) != std::string_view::npos) ++it;
(we could even extract the flag in which case we could get rid of the comment)
Time to return to C89? ;)
0 while (strchr("#0- +", *it)) ++it;
I thought of that, but not sure how to make it work, I’m getting:
note: non-constexpr function ‘strchr’ cannot be used in a constant expression
re: #31174 (review)
I can apply the "#0- +"sv.find change if others like it, but to me it seems less readable and only a little shorter.
178@@ -178,6 +179,18 @@ namespace tfm = tinyformat;
179
180 namespace tinyformat {
181
182+// Added for Bitcoin Core. Wrapper for checking format strings at compile time.
183+// Unlike ConstevalFormatString this supports std::string for runtime string
184+// formatting without compile time checks.
185+template <unsigned num_params>
186+struct FormatStringCheck {
Nice, simple approach, like it a lot! I think we can simplify the validator a bit more - let me know what you think.
Also, not exactly sure why %n parity wasn’t added like in https://github.com/bitcoin/bitcoin/pull/30999/files#diff-71badc1cc71ba46244f7841a088251bb294265f4fe9e662c0ad6b15be540eee4R69 (is it controversial or unnecessary or not useful)?
pr/tcheck.4 -> pr/tcheck.5, compare) with suggested changes. Thanks for the reviews and suggestions!
re: #31174#pullrequestreview-2425834191
Also, not exactly sure why
%nparity wasn’t added
Thanks I hadn’t seen #30999, and it seems like that would be a reasonable thing to check for, though I think there is a case for keeping the code as simple as possible and not trying to reproduce tinyformat quirks here. But the reason for not making that change here is I don’t think it’s related to this PR, and I think it’s generally better to make separate changes n separate PRs so they can be evaluated correctly and discussed more clearly.
re-ACK fe39acf88ff552bfc4a502c99774375b91824bb1 🕐
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK fe39acf88ff552bfc4a502c99774375b91824bb1 🕐
3Gv99KVENxeKH2/4i5mCpvzXcxQ66+sVorM28bzE1QmucATBYm7QTTpcvRNaaGKsCRRtGOmk0QEigsfGUPldxCA==
73+ PassFmt<3>("%2$+9.*3$d");
74+ PassFmt<4>("%3$*2$.*4$f");
75+
76+ // Make sure multiple flag characters "- 0+" are accepted
77+ PassFmt<3>("'%- 0+*.*f'");
78+ PassFmt<3>("'%1$- 0+*3$.*2$f'");
' deliberate? If so, what do they mean?
re: #31174 (review)
I’m pretty sure they must be accidental. These cases came from #31174 (review), and I just pasted them without noticing the single quotes. Can remove if the PR is updated again.
' are used to denote the begin and the end of the string, which would otherwise not be possible, because trailing spaces can normally not be seen when printing. They are not needed in this test and they are a leftover when I tested this against tinyformat at runtime.
Thanks for improving developer productivity with these small changes <3
ACK fe39acf88ff552bfc4a502c99774375b91824bb1