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.

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
1 parent 4cf1221 commit 49f7307
Show file tree
Hide file tree
Showing 5 changed files with 91 additions and 75 deletions.
7 changes: 1 addition & 6 deletions src/logging.h
View file
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
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>());
});
}
16 changes: 16 additions & 0 deletions src/test/util_string_tests.cpp
View file
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);
}

BOOST_AUTO_TEST_CASE(tinyformat_ConstevalFormatString)
{
// 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: %.*f)");
#endif
}


BOOST_AUTO_TEST_SUITE_END()
14 changes: 11 additions & 3 deletions src/tinyformat.h
View file
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) {
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
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 49f7307

Please sign in to comment.