Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error #30928

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error #30928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
104c805
move-only: ConstevalFormatString tfm::format to tinyformat.h
stickies-v Sep 25, 2024
4ce60d8
tinyformat: remove std::string overload
stickies-v Sep 25, 2024
4cf1221
tinyformat: force compile-time checks for format string literals
stickies-v Sep 25, 2024
49f7307
tinyformat: don't throw format string errors
stickies-v Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion doc/developer-notes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,7 +950,9 @@ Strings and formatting

- Do not use it when passing strings to `tfm::format`, `strprintf`, `LogInfo`, `LogDebug`, etc.

- *Rationale*: This is redundant. Tinyformat handles strings.
- *Rationale*: Tinyformat handles string parameters. Format strings
should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
Comment on lines +953 to +955
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we could avoid mentioning compile-time twice:

Suggested change
- *Rationale*: Tinyformat handles string parameters. Format strings
should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
- *Rationale*: Tinyformat handles string parameters.
Using a string literal or `constexpr const char*` ensures
format strings are validated at compile time.

Copy link
Contributor Author

@stickies-v stickies-v Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this is an improvement, I find it less clear about the distinction between the compile-time format string and the run-time string parameters, so I'm going to leave this as is for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why it's less clear than before, but it's just a nit from my part anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree the current message is confusing. The distinction between format string and parameters to the format string should ideally be made clearer, as well as the actual string type.

Suggested change
- *Rationale*: Tinyformat handles string parameters. Format strings
should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
- *Rationale*: Tinyformat handles `std::string` parameters. Format strings
on the other hand should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used std::string at first too, but then it was inconsistent with the rest of the documentation, which I think is even more unclear. I'll reconsider updating all std::string references in this block if I have to force-push again unless you think this is blocking?

Copy link
Member

Choose a reason for hiding this comment

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

If Tinyformat handles string parameters. is too controversial, I think it can be removed in the follow-up that removes the linter.

Otherwise, if this is replaced with std::string, someone else is going to suggest to mention std::string_view as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changing tinyformat to support string_view is definitely out of scope for this PR.

I would at least want to make it clearer that format strings are separate from other parameters, like this or with other words to that effect:

Suggested change
- *Rationale*: Tinyformat handles string parameters. Format strings
should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
- *Rationale*: Tinyformat handles string parameters. Format strings
on the other hand should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.

Even though I decided to drop into l0rinc's thread here, I independently found it unclear when doing a review-pass on it.

More precise suggestion, but not blocking:

Suggested change
- *Rationale*: Tinyformat handles string parameters. Format strings
should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
- *Rationale*: Tinyformat handles string parameters. Format strings
on the other hand should (except for translations) be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would at least want to make it clearer that format strings are separate from other parameters

I agree with that, and I'll incorporate your first suggestion (I don't agree with the translations bit) if I have to force-push, thanks!


- Do not use it to convert to `QString`. Use `QString::fromStdString()`.

Expand Down
24 changes: 12 additions & 12 deletions src/bitcoin-cli.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,15 +540,15 @@ class NetinfoRequestHandler : public BaseRequestHandler
// Report detailed peer connections list sorted by direction and minimum ping time.
if (DetailsRequested() && !m_peers.empty()) {
std::sort(m_peers.begin(), m_peers.end());
result += strprintf("<-> type net v mping ping send recv txn blk hb %*s%*s%*s ",
m_max_addr_processed_length, "addrp",
m_max_addr_rate_limited_length, "addrl",
m_max_age_length, "age");
result += tfm::format_raw("<-> type net v mping ping send recv txn blk hb %*s%*s%*s ",
stickies-v marked this conversation as resolved.
Show resolved Hide resolved
m_max_addr_processed_length, "addrp",
m_max_addr_rate_limited_length, "addrl",
m_max_age_length, "age");
if (m_is_asmap_on) result += " asmap ";
result += strprintf("%*s %-*s%s\n", m_max_id_length, "id", IsAddressSelected() ? m_max_addr_length : 0, IsAddressSelected() ? "address" : "", IsVersionSelected() ? "version" : "");
result += tfm::format_raw("%*s %-*s%s\n", m_max_id_length, "id", IsAddressSelected() ? m_max_addr_length : 0, IsAddressSelected() ? "address" : "", IsVersionSelected() ? "version" : "");
for (const Peer& peer : m_peers) {
std::string version{ToString(peer.version) + peer.sub_version};
result += strprintf(
result += tfm::format_raw(
stickies-v marked this conversation as resolved.
Show resolved Hide resolved
"%3s %6s %5s %2s%7s%7s%5s%5s%5s%5s %2s %*s%*s%*s%*i %*s %-*s%s\n",
peer.is_outbound ? "out" : "in",
ConnectionTypeForNetinfo(peer.conn_type),
Expand All @@ -575,7 +575,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
IsAddressSelected() ? peer.addr : "",
IsVersionSelected() && version != "0" ? version : "");
}
result += strprintf(" ms ms sec sec min min %*s\n\n", m_max_age_length, "min");
result += tfm::format_raw(" ms ms sec sec min min %*s\n\n", m_max_age_length, "min");
}

// Report peer connection totals by type.
Expand Down Expand Up @@ -624,7 +624,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
max_addr_size = std::max(addr["address"].get_str().length() + 1, max_addr_size);
}
for (const UniValue& addr : local_addrs) {
result += strprintf("\n%-*s port %6i score %6i", max_addr_size, addr["address"].get_str(), addr["port"].getInt<int>(), addr["score"].getInt<int>());
result += tfm::format_raw("\n%-*s port %6i score %6i", max_addr_size, addr["address"].get_str(), addr["port"].getInt<int>(), addr["score"].getInt<int>());
}
}

Expand Down Expand Up @@ -1117,10 +1117,10 @@ static void ParseGetInfoResult(UniValue& result)
}

for (const std::string& wallet : result["balances"].getKeys()) {
result_string += strprintf("%*s %s\n",
max_balance_length,
result["balances"][wallet].getValStr(),
wallet.empty() ? "\"\"" : wallet);
result_string += tfm::format_raw("%*s %s\n",
max_balance_length,
result["balances"][wallet].getValStr(),
wallet.empty() ? "\"\"" : wallet);
}
result_string += "\n";
}
Expand Down
12 changes: 7 additions & 5 deletions src/clientversion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ std::string FormatSubVersion(const std::string& name, int nClientVersion, const

std::string CopyrightHolders(const std::string& strPrefix)
{
const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION);
const auto copyright_devs = strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION).translated;
std::string strCopyrightHolders = strPrefix + copyright_devs;

// Make sure Bitcoin Core copyright is not removed by accident
Expand All @@ -85,15 +85,17 @@ std::string LicenseInfo()
{
const std::string URL_SOURCE_CODE = "<https://github.com/bitcoin/bitcoin>";

return CopyrightHolders(strprintf(_("Copyright (C) %i-%i").translated, 2009, COPYRIGHT_YEAR) + " ") + "\n" +
return CopyrightHolders(strprintf(_("Copyright (C) %i-%i"), 2009, COPYRIGHT_YEAR).translated + " ") + "\n" +
stickies-v marked this conversation as resolved.
Show resolved Hide resolved
"\n" +
strprintf(_("Please contribute if you find %s useful. "
"Visit %s for further information about the software.").translated, PACKAGE_NAME, "<" PACKAGE_URL ">") +
"Visit %s for further information about the software."),
PACKAGE_NAME, "<" PACKAGE_URL ">")
.translated +
"\n" +
strprintf(_("The source code is available from %s.").translated, URL_SOURCE_CODE) +
strprintf(_("The source code is available from %s."), URL_SOURCE_CODE).translated +
"\n" +
"\n" +
_("This is experimental software.").translated + "\n" +
strprintf(_("Distributed under the MIT software license, see the accompanying file %s or %s").translated, "COPYING", "<https://opensource.org/licenses/MIT>") +
strprintf(_("Distributed under the MIT software license, see the accompanying file %s or %s"), "COPYING", "<https://opensource.org/licenses/MIT>").translated +
"\n";
}
2 changes: 1 addition & 1 deletion src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
strprintf("Maximum tip age in seconds to consider node in initial block download (default: %u)",
Ticks<std::chrono::seconds>(DEFAULT_MAX_TIP_AGE)),
ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-printpriority", strprintf("Log transaction fee rate in " + CURRENCY_UNIT + "/kvB when mining blocks (default: %u)", DEFAULT_PRINT_MODIFIED_FEE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-printpriority", strprintf("Log transaction fee rate in %s/kvB when mining blocks (default: %u)", CURRENCY_UNIT, DEFAULT_PRINT_MODIFIED_FEE), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-uacomment=<cmt>", "Append comment to the user agent string", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);

SetupChainParamsBaseOptions(argsman);
Expand Down
7 changes: 1 addition & 6 deletions src/logging.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,12 +242,7 @@ template <typename... Args>
inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
{
if (LogInstance().Enabled()) {
std::string log_msg;
try {
log_msg = tfm::format(fmt, args...);
} catch (tinyformat::format_error& fmterr) {
log_msg = "Error \"" + std::string{fmterr.what()} + "\" while formatting log message: " + fmt.fmt;
}
const auto log_msg{tfm::format(fmt, args...)};
LogInstance().LogPrintStr(log_msg, logging_function, source_file, source_line, flag, level);
}
}
Expand Down
126 changes: 60 additions & 66 deletions src/test/fuzz/strprintf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,35 +48,32 @@ FUZZ_TARGET(str_printf)
//
// Upstream bug report: https://github.com/c42f/tinyformat/issues/70
stickies-v marked this conversation as resolved.
Show resolved Hide resolved

try {
CallOneOf(
fuzzed_data_provider,
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeRandomLengthString(32));
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeRandomLengthString(32));
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeRandomLengthString(32).c_str());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeRandomLengthString(32).c_str());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<signed char>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<signed char>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<unsigned char>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<unsigned char>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<char>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<char>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeBool());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeBool());
});
} catch (const tinyformat::format_error&) {
}
CallOneOf(
fuzzed_data_provider,
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeRandomLengthString(32));
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeRandomLengthString(32));
},
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeRandomLengthString(32).c_str());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeRandomLengthString(32).c_str());
},
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<signed char>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<signed char>());
},
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<unsigned char>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<unsigned char>());
},
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<char>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<char>());
},
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeBool());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeBool());
});

if (format_string.find('%') != std::string::npos && format_string.find('c') != std::string::npos) {
// Avoid triggering the following:
Expand All @@ -94,41 +91,38 @@ FUZZ_TARGET(str_printf)
return;
}

try {
CallOneOf(
fuzzed_data_provider,
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeFloatingPoint<float>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeFloatingPoint<float>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeFloatingPoint<double>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeFloatingPoint<double>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<int16_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<int16_t>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<uint16_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<uint16_t>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<int32_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<int32_t>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<uint32_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<uint32_t>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<int64_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<int64_t>());
},
[&] {
(void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<uint64_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<uint64_t>());
});
} catch (const tinyformat::format_error&) {
}
CallOneOf(
fuzzed_data_provider,
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeFloatingPoint<float>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeFloatingPoint<float>());
},
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeFloatingPoint<double>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeFloatingPoint<double>());
},
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<int16_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<int16_t>());
},
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<uint16_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<uint16_t>());
},
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<int32_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<int32_t>());
},
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<uint32_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<uint32_t>());
},
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<int64_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<int64_t>());
},
[&] {
(void)tinyformat::format_raw(format_string.c_str(), fuzzed_data_provider.ConsumeIntegral<uint64_t>());
(void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<uint64_t>());
});
}
8 changes: 4 additions & 4 deletions src/test/txrequest_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,10 @@ class Scenario
size_t real_total = runner.txrequest.Count(peer);
size_t real_candidates = runner.txrequest.CountCandidates(peer);
size_t real_inflight = runner.txrequest.CountInFlight(peer);
BOOST_CHECK_MESSAGE(real_total == total, strprintf("[" + comment + "] total %i (%i expected)", real_total, total));
stickies-v marked this conversation as resolved.
Show resolved Hide resolved
BOOST_CHECK_MESSAGE(real_inflight == inflight, strprintf("[" + comment + "] inflight %i (%i expected)", real_inflight, inflight));
BOOST_CHECK_MESSAGE(real_candidates == candidates, strprintf("[" + comment + "] candidates %i (%i expected)", real_candidates, candidates));
BOOST_CHECK_MESSAGE(ret == expected, "[" + comment + "] mismatching requestables");
BOOST_CHECK_MESSAGE(real_total == total, strprintf("[%s] total %i (%i expected)", comment, real_total, total));
BOOST_CHECK_MESSAGE(real_inflight == inflight, strprintf("[%s] inflight %i (%i expected)", comment, real_inflight, inflight));
BOOST_CHECK_MESSAGE(real_candidates == candidates, strprintf("[%s] candidates %i (%i expected)", comment, real_candidates, candidates));
BOOST_CHECK_MESSAGE(ret == expected, strprintf("[%s] mismatching requestables", comment));
});
}

Expand Down
16 changes: 16 additions & 0 deletions src/test/util_string_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <tinyformat.h>
#include <util/string.h>

#include <boost/test/unit_test.hpp>
#include <test/util/setup_common.h>

#include <sstream>

using namespace util;

BOOST_AUTO_TEST_SUITE(util_string_tests)
Expand Down Expand Up @@ -82,4 +85,17 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
FailFmtWithError<1>("%1$", err_term);
}

stickies-v marked this conversation as resolved.
Show resolved Hide resolved
BOOST_AUTO_TEST_CASE(tinyformat_ConstevalFormatString)
stickies-v marked this conversation as resolved.
Show resolved Hide resolved
{
// Ensure invalid format strings don't throw at run-time, when not in DEBUG
#ifndef DEBUG
Copy link
Contributor

@l0rinc l0rinc Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

might as well put the whole test inside, instead of having an empty passing test when 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: %.*f)");
#endif
}


BOOST_AUTO_TEST_SUITE_END()
Loading
Loading