tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error #30928
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. |
stickies-v
force-pushed
the
2024-09/try-format
branch
from
fb9d43f to
bb9b55f
Compare
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
stickies-v
force-pushed
the
2024-09/try-format
branch
from
bb9b55f to
6dc6e45
Compare
stickies-v
force-pushed
the
2024-09/try-format
branch
2 times, most recently
from
e48b847 to
ed57f6e
Compare
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
stickies-v
changed the title
|
Latest force-push increases the scope of this PR, see updated PR description for an overview. In a nutshell:
|
stickies-v
force-pushed
the
2024-09/try-format
branch
from
ed57f6e to
bb12305
Compare
There was a problem hiding this comment.
review ACK bb12305 💅
Show signature
Signature:
untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs SkW9a8N95d U4AP1RJMi krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK bb12305737e3c22325930b5bdb04028f1a6f2ef6 💅
Fge/JMP/cEBpCNfrVWYhr7mIRY4zXGiZRkBzWtbVouQ4q6WPnKlif9AdGqIb4Jrmkp/sIcvaECTG9QyCy1/mBA==
All (non-test) format strings are known at compile-time, so we don't need the std::string overload. Remove it so users use the safer util::ConstevalFormatString where possible, or the bilingual_str overload where applicable.
Callsites such as bitcoin-cli that (incorrectly) fail compile-time validation can still use tfm::format_raw.
stickies-v
force-pushed
the
2024-09/try-format
branch
from
4cb3749 to
31c625e
Compare
|
Force-pushed to rebase addressing a merge conflict, preserve Thank you for your thoughtful review @ryanofsky. tl;dr I share your concerns, but I think I have a different opinion on the trade-offs made. I think everyone here is in agreement that compile-time checks (ideally with
I agree that returning error strings is not a good way to handle errors in general, but I don't think throwing a Can you agree that the concept of not exposing I've force-pushed to preserve
The
Note:
I don't think this is correct: Line 139 in caf44e5
Oops, good catch, thanks. This string was lifted from
Adding |
In almost all cases, invalid format strings should not lead to crashing behaviour, so suppress run-time format errors by returning the error as a string instead of throwing a tinyformat::format_error. In DEBUG builds, tinyformat::format_error throwing behaviour is preserved. Note: most format string literals are partially validated at compile-time through util::ConstevalFormatString. Notably, bilingual_str format strings do not have this compile-time validation.
stickies-v
force-pushed
the
2024-09/try-format
branch
from
31c625e to
49f7307
Compare
There was a problem hiding this comment.
Left a nit.
Only change is adding throw when DEBUG, which seems fine. Edit: However, I think any attempt to catch bugs at runtime in this context is fragile and inefficient. If bugs should be found, it would be better to catch them at (or before) compile-time with the compiler, or with a tool.
re-ACK 49f7307 🎖
Show signature
Signature:
untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs SkW9a8N95d U4AP1RJMi krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 49f73071a8ec4131595090a619d6198a5f8b7c3c 🎖
XSyui/vuov5yPzjlUZ4UeGQZHo/JZ6LsYy2ftwOqSStKhn/L2aHSd/uDBco6LvvTWd3tPsTlSY2y9myvd7H5Aw==
| @@ -2003,7 2003,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc | |||
| WalletLogPrintf("Scanning current mempool transactions.\n"); | |||
| WITH_LOCK(cs_wallet, chain().requestMempoolTransactions(*this)); | |||
| } | |||
| ShowProgress(strprintf("%s " _("Rescanning…").translated, GetDisplayName()), 100); // hide progress dialog in GUI | |||
| ShowProgress(strprintf("%s %s", GetDisplayName(), _("Rescanning…").translated), 100); // hide progress dialog in GUI | |||
There was a problem hiding this comment.
nit in 4ce60d8: Could split the changes to make the format string a bit more const, and never exclusively translated in real code into a separate commit? The benefit would be that the doc changes and (constroversial?) c_str changes are in a separate commit. It would also allow easier cherry-picking into other pulls.
Edit: Done in commit fa788db (and the previous commit). Feel free to rebase on them, or not, or otherwise take them, and modify them as you like.
I removed |
|
Worth bringing back the |
|
I'm not in love with the ACK 49f7307
Diff1: 1f2d0ffd56 < -: ---------- move-only: ConstevalFormatString tfm::format to tinyformat.h
2: c937ebf1ce = 1: efa6f3f372 tinyformat: remove std::string overload
3: be7de807c8 = 2: 9329e334eb tinyformat: force compile-time checks for format string literals
4: 4f33e9a85c ! 3: 928538e378 tinyformat: don't throw format string errors
@@ Commit message
behaviour, so suppress run-time format errors by returning the error as
a string instead of throwing a tinyformat::format_error.
In DEBUG builds, tinyformat::format_error throwing behaviour is
preserved.
Note: most format string literals are partially validated at compile-time
through util::ConstevalFormatString. Notably, bilingual_str format strings
do not have this compile-time validation.
@@ src/logging.h: template <typename... Args>
- try {
- log_msg = tfm::format(fmt, args...);
- } catch (tinyformat::format_error& fmterr) {
-- /* Original format string will have newline so don't add one here */
- log_msg = "Error \"" std::string{fmterr.what()} "\" while formatting log message: " fmt.fmt;
- }
const auto log_msg{tfm::format(fmt, args...)};
@@ src/test/util_string_tests.cpp
#include <util/string.h>
#include <boost/test/unit_test.hpp>
#include <test/util/setup_common.h>
#include <sstream>
@@ src/test/util_string_tests.cpp: BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSp
BOOST_AUTO_TEST_CASE(tinyformat_ConstevalFormatString)
{
- // Ensure invalid format strings don't throw at run-time.
- BOOST_CHECK_EQUAL(tfm::format("%*c", "dummy"), R"(Error "tinyformat: Cannot convert from argument type to integer for use as variable width or precision" while formatting log message: %*c)");
- BOOST_CHECK_EQUAL(tfm::format("%2$*3$d", "dummy", "value"), R"(Error "tinyformat: Positional argument out of range" while formatting log message: %2$*3$d)");
// Ensure invalid format strings don't throw at run-time, when not in DEBUG
#ifndef DEBUG
BOOST_CHECK_EQUAL(tfm::format("%*c", "dummy"), R"(Error "tinyformat: Cannot convert from argument type to integer for use as variable width or precision" while formatting: %*c)");
BOOST_CHECK_EQUAL(tfm::format("%2$*3$d", "dummy", "value"), R"(Error "tinyformat: Positional argument out of range" while formatting: %2$*3$d)");
std::ostringstream oss;
tfm::format(oss, "%.*f", 5);
- BOOST_CHECK_EQUAL(oss.str(), R"(Error "tinyformat: Too many conversion specifiers in format string" while formatting log message: %.*f)");
BOOST_CHECK_EQUAL(oss.str(), R"(Error "tinyformat: Too many conversion specifiers in format string" while formatting: %.*f)");
#endif
}
@@ src/tinyformat.h: TINYFORMAT_FOREACH_ARGNUM(TINYFORMAT_MAKE_MAKEFORMATLIST)
try {
detail::formatImpl(out, fmt, list.m_args, list.m_N);
} catch (tinyformat::format_error& fmterr) {
- out << "Error \"" std::string{fmterr.what()} "\" while formatting log message: " fmt;
out << "Error \"" std::string{fmterr.what()} "\" while formatting: " fmt;
#ifdef DEBUG
throw;
#endif
}
}
We should definitely be able to see easily what the invalid values were (and add unit tests for those cases) |
| // Modified for Bitcoin Core to not throw for formatting errors | ||
| try { | ||
| detail::formatImpl(out, fmt, list.m_args, list.m_N); | ||
| } catch (tinyformat::format_error& fmterr) { |
There was a problem hiding this comment.
nit: we're already inside namespace tinyformat:
| } catch (tinyformat::format_error& fmterr) { | |
| } catch (format_error& fmterr) { |
(same for formatImpl)
| BOOST_AUTO_TEST_CASE(tinyformat_ConstevalFormatString) | ||
| { | ||
| // Ensure invalid format strings don't throw at run-time, when not in DEBUG | ||
| #ifndef DEBUG |
There was a problem hiding this comment.
might as well put the whole test inside, instead of having an empty passing test when DEBUG
|
I've carved out the first 3 commits into #31149, since other PRs (e.g. #31061 and #31072) somewhat rely on this, and everyone seems in agreement that adding compile-time checks is good. Existing ACKs should be trivially transferable. Converting this PR to draft until #31149 is merged to keep the controversial error suppression discussion until later. |
(Not sure that would add much value, we know the fuzzing will produce mostly garbage format strings that will trigger errors). |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
…ages 0184d33 scripted-diff: Replace strprintf(Untranslated) with Untranslated(strprintf) (Ryan Ofsky) 006e4d1 refactor: Use instead of strformat to concatenate translated & untranslated strings (Ryan Ofsky) 831d2bf refactor: Don't embed translated string in untranslated string. (Ryan Ofsky) 0580219 refactor: Avoid concatenation of format strings (Ryan Ofsky) Pull request description: This PR cleans up string formatting in the codebase so other PRs adding compile time checking can be simpler and easier to review (specifically #30928, #31061, #31074, and #31149). Currently these PRs are hard to review because in addition to changing formatting APIs, they have to update callers that are using the API's in unusual ways. Clean up these callers now so later PRs can be simpler. Specifically: - Use string literals instead of `std::string` format strings to enable more compile-time checking. - Avoid using untranslated bilingual strings as format strings. Use originals so they can by checked at compile time. - Favor `Untranslated(strprintf(...))` over `strprintf(Untranslated(...), ...)` for consistency and to prevent translated and untranslated strings from being unintentionally combined. ACKs for top commit: maflcko: lgtm ACK 0184d33 🔹 l0rinc: ACK 0184d33 - no overall difference because of the rebase Tree-SHA512: 37eb771187d30977f5f054eddb82af6dd69878ace89cede72032bb389e57200898119f9fe486ce6903ebb00cb24648be215ab3e44842b3d206b35e26038da750
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
Avoid unexpected run-time crashes from string formatting by enforcing compile-time format string checks for most*
tfm::formatandstrprintfusage, and by returning an error string instead of throwing for a run-time tfm::format_error so callsites don't need to implement this error handling.This PR should introduce no behaviour change.
The main changes are:
const std::string&tfm::formatoverload since it's not necessary. Usage of this overload is removed by one of:bilingual_stroverloadtfm::format_rawfunctions (only for tinyformat implementation or tests)tfm::formatoverloads totfm::format_raw, so existing callsites by default use the saferutil::ConstevalFormatStringtfm::formatoverloads. Callsites that for some reason don't pass the compile-time checks (such as inbitcoin-cli.cpp) can usetfm::format_raw.tfm::format_errorinternal totinyformatand just return error strings instead, so callsites don't need to implement error handling to prevent crashing.*The
bilingual_str format(const bilingual_str& fmt, const Args&... args)is not yet compile-time checked, which means thelint-format-strings.pytest can't be removed yet.Follow-up on #30889 (comment), an alternative to #15926.