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

Closed
wants to merge 4 commits into from
+144 −130
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
tinyformat: don"t throw format string errors 
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.
  • Loading branch information
@stickies-v
stickies-v committed Oct 8, 2024
commit 49f73071a8ec4131595090a619d6198a5f8b7c3c
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)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());
});
} 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)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>());
});
} 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>());
});
}
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()
14 changes: 11 additions & 3 deletions src/tinyformat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1049,7 +1049,15 @@ TINYFORMAT_FOREACH_ARGNUM(TINYFORMAT_MAKE_MAKEFORMATLIST)
/// list of format arguments is held in a single function argument.
inline void vformat(std::ostream& out, const char* fmt, FormatListRef list)
{
detail::formatImpl(out, fmt, list.m_args, list.m_N);
// 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) {
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.

nit: we"re already inside namespace tinyformat:

Suggested change
} catch (tinyformat::format_error& fmterr) {
} catch (format_error& fmterr) {

(same for formatImpl)

out << "Error \"" + std::string{fmterr.what()} + "\" while formatting: " + fmt;
#ifdef DEBUG
throw;
#endif
}
}


Expand Down Expand Up @@ -1150,12 +1158,12 @@ TINYFORMAT_FOREACH_ARGNUM(TINYFORMAT_MAKE_FORMAT_FUNCS)
template <typename... Args>
std::string format(util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
{
return format_raw(fmt.fmt, args...);
return tfm::format_raw(fmt.fmt, args...);
}
template <typename... Args>
void format(std::ostream& out, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
{
return format_raw(out, fmt.fmt, args...);
tfm::format_raw(out, fmt.fmt, args...);
}
} // namespace tinyformat

Expand Down
3 changes: 3 additions & 0 deletions test/lint/run-lint-format-strings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
FALSE_POSITIVES = [
("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION)"),
("src/test/translation_tests.cpp", "strprintf(format, arg)"),
("src/test/util_string_tests.cpp", r'tfm::format("%*c", "dummy")'),
("src/test/util_string_tests.cpp", r'tfm::format("%2$*3$d", "dummy", "value")'),
("src/test/util_string_tests.cpp", r'tfm::format(oss, "%.*f", 5)'),
]


Expand Down
Loading