From f219eeeb69ed30cc6b797b401381e97ca9905eaa Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Wed, 8 Dec 2021 17:04:46 -0800 Subject: [PATCH] Turn on -Wformat-security and -Wformat-nonliteral. The ENFORCE_FORMAT annotations help make it clear to the compiler which seemingly-nonliteral values are actually checked to be literals further up the callstack, because we disallow passing a non-literal as the format arg indicated by ENFORCE_FORMAT. --- .github/workflows/examples-esp32.yaml | 4 ++-- build/config/compiler/BUILD.gn | 3 +++ config/ameba/chip.cmake | 2 ++ examples/all-clusters-app/esp32/CMakeLists.txt | 3 +++ examples/platform/telink/util/src/ThreadUtil.cpp | 2 +- src/app/MessageDef/MessageDefHelper.cpp | 2 +- src/app/tests/TestEventLogging.cpp | 3 ++- src/app/tests/TestMessageDef.cpp | 3 ++- src/app/tests/integration/common.cpp | 3 ++- src/app/util/ember-print.cpp | 7 +++++++ .../python/chip/logging/LoggingRedirect.cpp | 2 +- .../Framework/CHIP.xcodeproj/project.pbxproj | 10 ++++++++++ src/lib/core/CHIPTLV.h | 9 +++++++-- src/lib/core/tests/TestCHIPTLV.cpp | 3 ++- src/lib/dnssd/Discovery_ImplPlatform.cpp | 3 ++- src/lib/shell/streamer.cpp | 5 +++-- src/lib/support/CHIPArgParser.cpp | 3 ++- src/lib/support/DefaultStorageKeyAllocator.h | 5 ++++- src/lib/support/logging/CHIPLogging.h | 15 +-------------- src/lib/support/logging/Constants.h | 16 ++++++++++++++++ src/lib/support/tests/TestCHIPArgParser.cpp | 3 ++- src/platform/Darwin/Logging.cpp | 3 +-- src/platform/Linux/Logging.cpp | 3 ++- src/platform/logging/impl/android/Logging.cpp | 2 +- src/platform/logging/impl/stdio/Logging.cpp | 3 ++- third_party/mbedtls/mbedtls.gni | 1 + 26 files changed, 82 insertions(+), 36 deletions(-) diff --git a/.github/workflows/examples-esp32.yaml b/.github/workflows/examples-esp32.yaml index 023d1b2bdbb611..54298cf4320eb5 100644 --- a/.github/workflows/examples-esp32.yaml +++ b/.github/workflows/examples-esp32.yaml @@ -26,7 +26,7 @@ jobs: # TODO ESP32 https://github.com/project-chip/connectedhomeip/issues/1510 esp32: name: ESP32 - timeout-minutes: 85 + timeout-minutes: 95 runs-on: ubuntu-latest if: github.actor != 'restyled-io[bot]' @@ -60,7 +60,7 @@ jobs: .environment/gn_out/.ninja_log .environment/pigweed-venv/*.log - name: Build some M5Stack variations - timeout-minutes: 20 + timeout-minutes: 30 run: | ./scripts/run_in_build_env.sh \ "./scripts/build/build_examples.py \ diff --git a/build/config/compiler/BUILD.gn b/build/config/compiler/BUILD.gn index 81fb08caa8afdb..9df167b4f83234 100644 --- a/build/config/compiler/BUILD.gn +++ b/build/config/compiler/BUILD.gn @@ -202,6 +202,9 @@ config("strict_warnings") { "-Wshadow", "-Wunreachable-code", "-Wvla", + "-Wformat", + "-Wformat-nonliteral", + "-Wformat-security", ] cflags_cc = [ "-Wnon-virtual-dtor" ] diff --git a/config/ameba/chip.cmake b/config/ameba/chip.cmake index 9d39d92c72698f..b7986d5d1a6f53 100644 --- a/config/ameba/chip.cmake +++ b/config/ameba/chip.cmake @@ -42,6 +42,8 @@ list( -Wno-unused-parameter -Wno-format -Wno-stringop-truncation + -Wno-format-nonliteral + -Wno-format-security -std=c++17 ) diff --git a/examples/all-clusters-app/esp32/CMakeLists.txt b/examples/all-clusters-app/esp32/CMakeLists.txt index 9685eac322e0b3..3753bddafad077 100644 --- a/examples/all-clusters-app/esp32/CMakeLists.txt +++ b/examples/all-clusters-app/esp32/CMakeLists.txt @@ -34,6 +34,9 @@ endif() project(chip-all-clusters-app) idf_build_set_property(CXX_COMPILE_OPTIONS "-std=gnu++17;-Os;-DLWIP_IPV6_SCOPES=0;-DCHIP_HAVE_CONFIG_H" APPEND) idf_build_set_property(C_COMPILE_OPTIONS "-Os;-DLWIP_IPV6_SCOPES=0" APPEND) +# For the C3, project_include.cmake sets -Wno-format, but does not clear various +# flags that depend on -Wformat +idf_build_set_property(COMPILE_OPTIONS "-Wno-format-nonliteral;-Wno-format-security" APPEND) flashing_script() diff --git a/examples/platform/telink/util/src/ThreadUtil.cpp b/examples/platform/telink/util/src/ThreadUtil.cpp index c501cd943cf91e..bbe95d81d66192 100644 --- a/examples/platform/telink/util/src/ThreadUtil.cpp +++ b/examples/platform/telink/util/src/ThreadUtil.cpp @@ -51,7 +51,7 @@ void StartDefaultThreadNetwork(void) chip::DeviceLayer::ThreadStackMgr().SetThreadEnabled(true); } -void tlOtPlatLog(otLogLevel aLogLevel, otLogRegion aLogRegion, const char * aFormat, ...) +void ENFORCE_FORMAT(3, 4) tlOtPlatLog(otLogLevel aLogLevel, otLogRegion aLogRegion, const char * aFormat, ...) { va_list args; diff --git a/src/app/MessageDef/MessageDefHelper.cpp b/src/app/MessageDef/MessageDefHelper.cpp index a8f6c338074867..f57feed0c35356 100644 --- a/src/app/MessageDef/MessageDefHelper.cpp +++ b/src/app/MessageDef/MessageDefHelper.cpp @@ -41,7 +41,7 @@ char gLineBuffer[256]; size_t gCurLineBufferSize = 0; } // namespace -void PrettyPrintIM(bool aIsNewLine, const char * aFmt, ...) +void ENFORCE_FORMAT(2, 3) PrettyPrintIM(bool aIsNewLine, const char * aFmt, ...) { va_list args; size_t ret; diff --git a/src/app/tests/TestEventLogging.cpp b/src/app/tests/TestEventLogging.cpp index 016df1659dcc2f..87f522baa55ca0 100644 --- a/src/app/tests/TestEventLogging.cpp +++ b/src/app/tests/TestEventLogging.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -89,7 +90,7 @@ class TestContext : public chip::Test::AppContext } }; -void SimpleDumpWriter(const char * aFormat, ...) +void ENFORCE_FORMAT(1, 2) SimpleDumpWriter(const char * aFormat, ...) { va_list args; diff --git a/src/app/tests/TestMessageDef.cpp b/src/app/tests/TestMessageDef.cpp index 9451e2061de198..9f08ba6b31bc73 100644 --- a/src/app/tests/TestMessageDef.cpp +++ b/src/app/tests/TestMessageDef.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include #include @@ -46,7 +47,7 @@ namespace { using namespace chip::app; -void TLVPrettyPrinter(const char * aFormat, ...) +void ENFORCE_FORMAT(1, 2) TLVPrettyPrinter(const char * aFormat, ...) { va_list args; diff --git a/src/app/tests/integration/common.cpp b/src/app/tests/integration/common.cpp index d0ad7da5f45587..69bb8b903d923b 100644 --- a/src/app/tests/integration/common.cpp +++ b/src/app/tests/integration/common.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include chip::Messaging::ExchangeManager gExchangeManager; @@ -65,7 +66,7 @@ void ShutdownChip(void) chip::DeviceLayer::PlatformMgr().Shutdown(); } -void TLVPrettyPrinter(const char * aFormat, ...) +void ENFORCE_FORMAT(1, 2) TLVPrettyPrinter(const char * aFormat, ...) { va_list args; diff --git a/src/app/util/ember-print.cpp b/src/app/util/ember-print.cpp index 3f21c53468761e..51326ca3716a86 100644 --- a/src/app/util/ember-print.cpp +++ b/src/app/util/ember-print.cpp @@ -76,7 +76,14 @@ void emberAfPrintBuffer(int category, const uint8_t * buffer, uint16_t length, b const uint32_t outStringEnd = segmentLength * perByteCharCount; for (uint32_t dst_idx = 0; dst_idx < outStringEnd && index < segmentEnd; dst_idx += perByteCharCount, index++) { + // The perByteFormatStr is in fact a literal (one of two), but + // the compiler does not realize that. We could branch on + // perByteFormatStr and have separate snprintf calls, but this + // seems like it might lead to smaller code. +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat-nonliteral" snprintf(result + dst_idx, outStringEnd - dst_idx + 1, perByteFormatStr, buffer[index]); +#pragma GCC diagnostic pop } result[outStringEnd] = 0; emberAfPrint(category, "%s", result); diff --git a/src/controller/python/chip/logging/LoggingRedirect.cpp b/src/controller/python/chip/logging/LoggingRedirect.cpp index e66f663649335b..c290fa30e8b7cc 100644 --- a/src/controller/python/chip/logging/LoggingRedirect.cpp +++ b/src/controller/python/chip/logging/LoggingRedirect.cpp @@ -26,7 +26,7 @@ using PythonLogCallback = void (*)(uint8_t category, const char * module, const PythonLogCallback sPythonLogCallback; -void NativeLoggingCallback(const char * module, uint8_t category, const char * msg, va_list args) +void ENFORCE_FORMAT(3, 0) NativeLoggingCallback(const char * module, uint8_t category, const char * msg, va_list args) { if (sPythonLogCallback == nullptr) { diff --git a/src/darwin/Framework/CHIP.xcodeproj/project.pbxproj b/src/darwin/Framework/CHIP.xcodeproj/project.pbxproj index 312dd839b20624..00dd58da64c5ac 100644 --- a/src/darwin/Framework/CHIP.xcodeproj/project.pbxproj +++ b/src/darwin/Framework/CHIP.xcodeproj/project.pbxproj @@ -663,6 +663,11 @@ INSTALL_PATH = "$(LOCAL_LIBRARY_DIR)/Frameworks"; IPHONEOS_DEPLOYMENT_TARGET = 13.4; LIBRARY_SEARCH_PATHS = "$(TEMP_DIR)/out/lib"; + OTHER_CFLAGS = ( + "-Wformat", + "-Wformat-nonliteral", + "-Wformat-security", + ); OTHER_LDFLAGS = ""; "OTHER_LDFLAGS[sdk=*]" = ( "-framework", @@ -800,6 +805,11 @@ INSTALL_PATH = "$(LOCAL_LIBRARY_DIR)/Frameworks"; IPHONEOS_DEPLOYMENT_TARGET = 13.4; LIBRARY_SEARCH_PATHS = "$(TEMP_DIR)/out/lib"; + OTHER_CFLAGS = ( + "-Wformat", + "-Wformat-nonliteral", + "-Wformat-security", + ); OTHER_LDFLAGS = ""; "OTHER_LDFLAGS[arch=*]" = ( "-lnetwork", diff --git a/src/lib/core/CHIPTLV.h b/src/lib/core/CHIPTLV.h index 141049372b0e5f..8509c8a417e866 100644 --- a/src/lib/core/CHIPTLV.h +++ b/src/lib/core/CHIPTLV.h @@ -36,6 +36,7 @@ #include #include #include +#include #include #include @@ -1614,7 +1615,9 @@ class DLL_EXPORT TLVWriter * `WriteElementHead` or `GetNewBuffer` -- failed, their * error is immediately forwarded up the call stack. */ - CHIP_ERROR PutStringF(Tag tag, const char * fmt, ...); + // The ENFORCE_FORMAT args are "off by one" because this is a class method, + // with an implicit "this" as first arg. + CHIP_ERROR PutStringF(Tag tag, const char * fmt, ...) ENFORCE_FORMAT(3, 4); /** * @brief @@ -1660,7 +1663,9 @@ class DLL_EXPORT TLVWriter * `WriteElementHead` or `GetNewBuffer` -- failed, their * error is immediately forwarded up the call stack. */ - CHIP_ERROR VPutStringF(Tag tag, const char * fmt, va_list ap); + // The ENFORCE_FORMAT args are "off by one" because this is a class method, + // with an implicit "this" as first arg. + CHIP_ERROR VPutStringF(Tag tag, const char * fmt, va_list ap) ENFORCE_FORMAT(3, 0); /** * Encodes a TLV null value. diff --git a/src/lib/core/tests/TestCHIPTLV.cpp b/src/lib/core/tests/TestCHIPTLV.cpp index 112a721c85678b..3cc08001126467 100644 --- a/src/lib/core/tests/TestCHIPTLV.cpp +++ b/src/lib/core/tests/TestCHIPTLV.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include @@ -1632,7 +1633,7 @@ static void TestIntMinMax(nlTestSuite * inSuite, void * inContext) * to the format specifiers in @a aFormat. * */ -void SimpleDumpWriter(const char * aFormat, ...) +void ENFORCE_FORMAT(1, 2) SimpleDumpWriter(const char * aFormat, ...) { va_list args; diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp index ca32481b5ca8b9..916bf27bc78c05 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.cpp +++ b/src/lib/dnssd/Discovery_ImplPlatform.cpp @@ -174,7 +174,8 @@ CHIP_ERROR AddPtrRecord(DiscoveryFilterType type, const char ** entries, size_t return AddPtrRecord(type, entries, entriesCount, buffer, bufferLen, value.Value()); } -CHIP_ERROR CopyTextRecordValue(char * buffer, size_t bufferLen, int minCharactersWritten, const char * format, ...) +CHIP_ERROR ENFORCE_FORMAT(4, 5) + CopyTextRecordValue(char * buffer, size_t bufferLen, int minCharactersWritten, const char * format, ...) { va_list args; va_start(args, format); diff --git a/src/lib/shell/streamer.cpp b/src/lib/shell/streamer.cpp index 4bb11ebb509b7d..5b8e2e5e91c46c 100644 --- a/src/lib/shell/streamer.cpp +++ b/src/lib/shell/streamer.cpp @@ -23,6 +23,7 @@ #include "streamer.h" +#include #include #include @@ -48,7 +49,7 @@ ssize_t streamer_write(streamer_t * self, const char * buf, size_t len) return self->write_cb(self, buf, len); } -ssize_t streamer_vprintf(streamer_t * self, const char * fmt, va_list ap) +ssize_t ENFORCE_FORMAT(2, 0) streamer_vprintf(streamer_t * self, const char * fmt, va_list ap) { char buf[CONSOLE_DEFAULT_MAX_LINE]; unsigned len; @@ -64,7 +65,7 @@ ssize_t streamer_vprintf(streamer_t * self, const char * fmt, va_list ap) return streamer_write(self, buf, len); } -ssize_t streamer_printf(streamer_t * self, const char * fmt, ...) +ssize_t ENFORCE_FORMAT(2, 3) streamer_printf(streamer_t * self, const char * fmt, ...) { va_list ap; ssize_t rc; diff --git a/src/lib/support/CHIPArgParser.cpp b/src/lib/support/CHIPArgParser.cpp index a4384ba55fb2df..2af19e854b02c5 100644 --- a/src/lib/support/CHIPArgParser.cpp +++ b/src/lib/support/CHIPArgParser.cpp @@ -48,6 +48,7 @@ #include #include +#include /* * TODO: Revisit these if and when fabric ID and node ID support has @@ -622,7 +623,7 @@ void PrintOptionHelp(OptionSet * optSets[], FILE * s) * Applications should call through the PrintArgError function pointer, rather * than calling this function directly. */ -void DefaultPrintArgError(const char * msg, ...) +void ENFORCE_FORMAT(1, 2) DefaultPrintArgError(const char * msg, ...) { va_list ap; diff --git a/src/lib/support/DefaultStorageKeyAllocator.h b/src/lib/support/DefaultStorageKeyAllocator.h index 4a8ada7e653b72..8ac8d14670cfe7 100644 --- a/src/lib/support/DefaultStorageKeyAllocator.h +++ b/src/lib/support/DefaultStorageKeyAllocator.h @@ -17,6 +17,7 @@ #pragma once #include +#include #include namespace chip { @@ -48,7 +49,9 @@ class DefaultStorageKeyAllocator private: static const size_t kKeyLengthMax = 32; - const char * Format(const char * format...) + // The ENFORCE_FORMAT args are "off by one" because this is a class method, + // with an implicit "this" as first arg. + const char * ENFORCE_FORMAT(2, 3) Format(const char * format, ...) { va_list args; va_start(args, format); diff --git a/src/lib/support/logging/CHIPLogging.h b/src/lib/support/logging/CHIPLogging.h index 8dfffd21288d1e..b23b47a31f665f 100644 --- a/src/lib/support/logging/CHIPLogging.h +++ b/src/lib/support/logging/CHIPLogging.h @@ -79,20 +79,7 @@ using LogRedirectCallback_t = void (*)(const char * module, uint8_t category, co void SetLogRedirectCallback(LogRedirectCallback_t callback); -/** - * gcc and clang provide a way to warn for a custom formatter when formats don't - * match arguments. Use that for Log() so we catch mistakes. The "format" - * attribute takes the type of format, which arg is the format string, and which - * arg is the first variadic arg, with both arg numbers 1-based. - */ - -#if defined(__GNUC__) -#define ENFORCE_FORMAT(n, m) __attribute__((format(printf, n, m))) -#else // __GNUC__ -#define ENFORCE_FORMAT(n, m) /* How to do with MSVC? */ -#endif // __GNUC__ - -void LogV(uint8_t module, uint8_t category, const char * msg, va_list args); +void LogV(uint8_t module, uint8_t category, const char * msg, va_list args) ENFORCE_FORMAT(3, 0); void Log(uint8_t module, uint8_t category, const char * msg, ...) ENFORCE_FORMAT(3, 4); void LogByteSpan(uint8_t module, uint8_t category, const ByteSpan & span); diff --git a/src/lib/support/logging/Constants.h b/src/lib/support/logging/Constants.h index 0214fcaa2dbab8..1dffc2b6467404 100644 --- a/src/lib/support/logging/Constants.h +++ b/src/lib/support/logging/Constants.h @@ -2,6 +2,22 @@ #pragma once +/** + * gcc and clang provide a way to warn for a custom formatter when formats don't + * match arguments. Use that for Log() so we catch mistakes. The "format" + * attribute takes the type of format, which arg is the format string, and which + * arg is the first variadic arg, with both arg numbers 1-based. + * + * The second arg should be set to 0 if the function takes a va_list instead of + * varargs. + */ + +#if defined(__GNUC__) +#define ENFORCE_FORMAT(n, m) __attribute__((format(printf, n, m))) +#else // __GNUC__ +#define ENFORCE_FORMAT(n, m) /* How to do with MSVC? */ +#endif // __GNUC__ + namespace chip { namespace Logging { diff --git a/src/lib/support/tests/TestCHIPArgParser.cpp b/src/lib/support/tests/TestCHIPArgParser.cpp index 128b5d7aff2378..092194e33332df 100644 --- a/src/lib/support/tests/TestCHIPArgParser.cpp +++ b/src/lib/support/tests/TestCHIPArgParser.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #if CHIP_CONFIG_ENABLE_ARG_PARSER @@ -730,7 +731,7 @@ static bool HandleNonOptionArgs(const char * progName, int argc, char * argv[]) return true; } -static void HandleArgError(const char * msg, ...) +static void ENFORCE_FORMAT(1, 2) HandleArgError(const char * msg, ...) { size_t msgLen; int status; diff --git a/src/platform/Darwin/Logging.cpp b/src/platform/Darwin/Logging.cpp index 180ec87b4c302c..362977fbd0a3ec 100644 --- a/src/platform/Darwin/Logging.cpp +++ b/src/platform/Darwin/Logging.cpp @@ -4,7 +4,6 @@ #include #include -#include #include @@ -19,7 +18,7 @@ namespace chip { namespace Logging { namespace Platform { -void LogV(const char * module, uint8_t category, const char * msg, va_list v) +void ENFORCE_FORMAT(3, 0) LogV(const char * module, uint8_t category, const char * msg, va_list v) { timeval time; gettimeofday(&time, NULL); diff --git a/src/platform/Linux/Logging.cpp b/src/platform/Linux/Logging.cpp index b5a15f5bb3a59f..ebf3e21b8cab50 100644 --- a/src/platform/Linux/Logging.cpp +++ b/src/platform/Linux/Logging.cpp @@ -1,5 +1,6 @@ /* See Project CHIP LICENSE file for licensing information. */ +#include #include #include @@ -27,7 +28,7 @@ namespace Platform { /** * CHIP log output functions. */ -void LogV(const char * module, uint8_t category, const char * msg, va_list v) +void ENFORCE_FORMAT(3, 0) LogV(const char * module, uint8_t category, const char * msg, va_list v) { struct timeval tv; diff --git a/src/platform/logging/impl/android/Logging.cpp b/src/platform/logging/impl/android/Logging.cpp index fcfc397ea1bbaa..f89044b34d095a 100644 --- a/src/platform/logging/impl/android/Logging.cpp +++ b/src/platform/logging/impl/android/Logging.cpp @@ -9,7 +9,7 @@ namespace chip { namespace Logging { namespace Platform { -void LogV(const char * module, uint8_t category, const char * msg, va_list v) +void ENFORCE_FORMAT(3, 0) LogV(const char * module, uint8_t category, const char * msg, va_list v) { int priority = (category == kLogCategory_Error) ? ANDROID_LOG_ERROR : ANDROID_LOG_DEBUG; __android_log_vprint(priority, module, msg, v); diff --git a/src/platform/logging/impl/stdio/Logging.cpp b/src/platform/logging/impl/stdio/Logging.cpp index 95d2093b4b4262..74beb7212b51b9 100644 --- a/src/platform/logging/impl/stdio/Logging.cpp +++ b/src/platform/logging/impl/stdio/Logging.cpp @@ -1,5 +1,6 @@ /* See Project CHIP LICENSE file for licensing information. */ +#include #include #include @@ -8,7 +9,7 @@ namespace chip { namespace Logging { namespace Platform { -void LogV(const char * module, uint8_t category, const char * msg, va_list v) +void ENFORCE_FORMAT(3, 0) LogV(const char * module, uint8_t category, const char * msg, va_list v) { printf("CHIP:%s: ", module); vprintf(msg, v); diff --git a/third_party/mbedtls/mbedtls.gni b/third_party/mbedtls/mbedtls.gni index bea9519074d330..9bde362d010cab 100644 --- a/third_party/mbedtls/mbedtls.gni +++ b/third_party/mbedtls/mbedtls.gni @@ -24,6 +24,7 @@ template("mbedtls_target") { "-Wno-maybe-uninitialized", "-Wno-string-concatenation", "-Wno-unused-but-set-parameter", + "-Wno-format-nonliteral", # Because of mbedtls_debug_print_msg ] }