From fa39b1ca63874db8ef8bc16b87e2699e8e1b67be Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 29 Jul 2024 11:09:58 +0200 Subject: [PATCH 1/3] doc: move-only logging warning Put the warning closer to where it is relevant. That is, put it close to the functions that actually do unconditional logging. Also, remove a stray empty line. --- src/logging.cpp | 3 +-- src/logging.h | 9 ++++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/logging.cpp b/src/logging.cpp index 9a54a12b426e5..d04db767e6e9b 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2022 The Bitcoin Core developers +// Copyright (c) 2009-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -103,7 +103,6 @@ void BCLog::Logger::DisconnectTestLogger() m_cur_buffer_memusage = 0; m_buffer_lines_discarded = 0; m_msgs_before_open.clear(); - } void BCLog::Logger::DisableLogging() diff --git a/src/logging.h b/src/logging.h index d583419d3a5c4..59b4f106d0ab5 100644 --- a/src/logging.h +++ b/src/logging.h @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2022 The Bitcoin Core developers +// Copyright (c) 2009-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -245,10 +245,6 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve /** Return true if str parses as a log category and set the flag */ bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str); -// Be conservative when using functions that -// unconditionally log to debug.log! It should not be the case that an inbound -// peer can fill up a user's disk with debug.log entries. - template static inline void LogPrintf_(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, const char* fmt, const Args&... args) { @@ -267,6 +263,9 @@ static inline void LogPrintf_(std::string_view logging_function, std::string_vie #define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__) // Log unconditionally. +// Be conservative when using functions that unconditionally log to debug.log! +// It should not be the case that an inbound peer can fill up a user's storage +// with debug.log entries. #define LogInfo(...) LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Info, __VA_ARGS__) #define LogWarning(...) LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Warning, __VA_ARGS__) #define LogError(...) LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Error, __VA_ARGS__) From fae9b60c4ffef38d9725f42f992b1f38765312a3 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 30 Jul 2024 10:27:18 +0200 Subject: [PATCH 2/3] test: Use LogPrintStr to test m_log_sourcelocations This test checks m_log_sourcelocations, not the formatting with format specifiers. Those are tested in logging_LogPrintMacros below. So just use LogPrintStr directly in this test, without format specifiers and format args. This is required for a follow-up commit. --- src/test/logging_tests.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp index a6fd44e395a02..8217a2385cfd4 100644 --- a/src/test/logging_tests.cpp +++ b/src/test/logging_tests.cpp @@ -83,15 +83,15 @@ BOOST_AUTO_TEST_CASE(logging_timer) BOOST_CHECK_EQUAL(micro_timer.LogMsg("msg").substr(0, result_prefix.size()), result_prefix); } -BOOST_FIXTURE_TEST_CASE(logging_LogPrintf_, LogSetup) +BOOST_FIXTURE_TEST_CASE(logging_LogPrintStr, LogSetup) { LogInstance().m_log_sourcelocations = true; - LogPrintf_("fn1", "src1", 1, BCLog::LogFlags::NET, BCLog::Level::Debug, "foo1: %s\n", "bar1"); - LogPrintf_("fn2", "src2", 2, BCLog::LogFlags::NET, BCLog::Level::Info, "foo2: %s\n", "bar2"); - LogPrintf_("fn3", "src3", 3, BCLog::LogFlags::ALL, BCLog::Level::Debug, "foo3: %s\n", "bar3"); - LogPrintf_("fn4", "src4", 4, BCLog::LogFlags::ALL, BCLog::Level::Info, "foo4: %s\n", "bar4"); - LogPrintf_("fn5", "src5", 5, BCLog::LogFlags::NONE, BCLog::Level::Debug, "foo5: %s\n", "bar5"); - LogPrintf_("fn6", "src6", 6, BCLog::LogFlags::NONE, BCLog::Level::Info, "foo6: %s\n", "bar6"); + LogInstance().LogPrintStr("foo1: bar1\n", "fn1", "src1", 1, BCLog::LogFlags::NET, BCLog::Level::Debug); + LogInstance().LogPrintStr("foo2: bar2\n", "fn2", "src2", 2, BCLog::LogFlags::NET, BCLog::Level::Info); + LogInstance().LogPrintStr("foo3: bar3\n", "fn3", "src3", 3, BCLog::LogFlags::ALL, BCLog::Level::Debug); + LogInstance().LogPrintStr("foo4: bar4\n", "fn4", "src4", 4, BCLog::LogFlags::ALL, BCLog::Level::Info); + LogInstance().LogPrintStr("foo5: bar5\n", "fn5", "src5", 5, BCLog::LogFlags::NONE, BCLog::Level::Debug); + LogInstance().LogPrintStr("foo6: bar6\n", "fn6", "src6", 6, BCLog::LogFlags::NONE, BCLog::Level::Info); std::ifstream file{tmp_log_path}; std::vector log_lines; for (std::string log; std::getline(file, log);) { From facbcd4cef8890ae18976fb53b67ea56b3c04454 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 29 Jul 2024 12:08:48 +0200 Subject: [PATCH 3/3] log: Use ConstevalFormatString This changes all logging (including the wallet logging) to produce a ConstevalFormatString at compile time, so that the format string can be validated at compile-time. Also, while touching the wallet logging, avoid a copy of the template Params by using const Params&. --- src/logging.h | 6 +++--- src/wallet/scriptpubkeyman.h | 4 ++-- src/wallet/wallet.h | 4 ++-- test/lint/lint-format-strings.py | 8 -------- test/lint/run-lint-format-strings.py | 5 ----- 5 files changed, 7 insertions(+), 20 deletions(-) diff --git a/src/logging.h b/src/logging.h index 59b4f106d0ab5..8605c8cd64f75 100644 --- a/src/logging.h +++ b/src/logging.h @@ -246,7 +246,7 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str); template -static inline void LogPrintf_(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, const char* fmt, const Args&... 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 fmt, const Args&... args) { if (LogInstance().Enabled()) { std::string log_msg; @@ -254,13 +254,13 @@ static inline void LogPrintf_(std::string_view logging_function, std::string_vie 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; + log_msg = "Error \"" + std::string{fmterr.what()} + "\" while formatting log message: " + fmt.fmt; } LogInstance().LogPrintStr(log_msg, logging_function, source_file, source_line, flag, level); } } -#define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__) +#define LogPrintLevel_(category, level, ...) LogPrintFormatInternal(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__) // Log unconditionally. // Be conservative when using functions that unconditionally log to debug.log! diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index ba3562c63856f..cf7b7eaf31de7 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -254,9 +254,9 @@ class ScriptPubKeyMan /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */ template - void WalletLogPrintf(const char* fmt, Params... parameters) const + void WalletLogPrintf(util::ConstevalFormatString wallet_fmt, const Params&... params) const { - LogPrintf(("%s " + std::string{fmt}).c_str(), m_storage.GetDisplayName(), parameters...); + LogInfo("%s %s", m_storage.GetDisplayName(), tfm::format(wallet_fmt, params...)); }; /** Watch-only address added */ diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 485eed11fabec..d3a7208b15e16 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -927,9 +927,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati /** Prepends the wallet name in logging output to ease debugging in multi-wallet use cases */ template - void WalletLogPrintf(const char* fmt, Params... parameters) const + void WalletLogPrintf(util::ConstevalFormatString wallet_fmt, const Params&... params) const { - LogPrintf(("%s " + std::string{fmt}).c_str(), GetDisplayName(), parameters...); + LogInfo("%s %s", GetDisplayName(), tfm::format(wallet_fmt, params...)); }; /** Upgrade the wallet */ diff --git a/test/lint/lint-format-strings.py b/test/lint/lint-format-strings.py index c30975fea7f7f..a809851ec6302 100755 --- a/test/lint/lint-format-strings.py +++ b/test/lint/lint-format-strings.py @@ -17,15 +17,7 @@ FUNCTION_NAMES_AND_NUMBER_OF_LEADING_ARGUMENTS = [ 'tfm::format,1', # Assuming tfm::::format(std::ostream&, ... - 'LogError,0', - 'LogWarning,0', - 'LogInfo,0', - 'LogDebug,1', - 'LogTrace,1', - 'LogPrintf,0', - 'LogPrintLevel,2', 'strprintf,0', - 'WalletLogPrintf,0', ] RUN_LINT_FILE = 'test/lint/run-lint-format-strings.py' diff --git a/test/lint/run-lint-format-strings.py b/test/lint/run-lint-format-strings.py index a32717653aaf8..d3c0ac92e5d69 100755 --- a/test/lint/run-lint-format-strings.py +++ b/test/lint/run-lint-format-strings.py @@ -15,11 +15,6 @@ FALSE_POSITIVES = [ ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION)"), ("src/test/translation_tests.cpp", "strprintf(format, arg)"), - ("src/validationinterface.cpp", "LogDebug(BCLog::VALIDATION, fmt \"\\n\", __VA_ARGS__)"), - ("src/wallet/wallet.h", "WalletLogPrintf(const char* fmt, Params... parameters)"), - ("src/wallet/wallet.h", "LogPrintf((\"%s \" + std::string{fmt}).c_str(), GetDisplayName(), parameters...)"), - ("src/wallet/scriptpubkeyman.h", "WalletLogPrintf(const char* fmt, Params... parameters)"), - ("src/wallet/scriptpubkeyman.h", "LogPrintf((\"%s \" + std::string{fmt}).c_str(), m_storage.GetDisplayName(), parameters...)"), ]