Skip to content

Commit

Permalink
tinyformat: don't throw format string errors
Browse files Browse the repository at this point in the history 
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.

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 Sep 30, 2024
1 parent be7de80 commit 4f33e9a
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 76 deletions.
8 changes: 1 addition & 7 deletions src/logging.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,13 +249,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) {
/* 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...)};
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
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

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>());
});
}
14 changes: 14 additions & 0 deletions src/test/util_string_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@
// 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 <sstream>

using namespace util;

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

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)");
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_AUTO_TEST_SUITE_END()
11 changes: 8 additions & 3 deletions src/tinyformat.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1049,7 +1049,12 @@ 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) {
out << "Error \"" + std::string{fmterr.what()} + "\" while formatting log message: " + fmt;
}
}


Expand Down Expand Up @@ -1150,12 +1155,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
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

0 comments on commit 4f33e9a

Please sign in to comment.