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

@@ -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 \

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" ]

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
 )
 

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()
 

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;
 

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;

src/app/tests/TestEventLogging.cpp

@@ -34,6 +34,7 @@
 #include <lib/core/CHIPTLVUtilities.hpp>
 #include <lib/support/ErrorStr.h>
 #include <lib/support/UnitTestRegistration.h>
+#include <lib/support/logging/Constants.h>
 #include <messaging/ExchangeContext.h>
 #include <messaging/Flags.h>
 #include <platform/CHIPDeviceLayer.h>
@@ -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;
 

src/app/tests/TestMessageDef.cpp

@@ -38,6 +38,7 @@
 #include <lib/core/CHIPTLVDebug.hpp>
 #include <lib/support/CHIPMem.h>
 #include <lib/support/UnitTestRegistration.h>
+#include <lib/support/logging/Constants.h>
 #include <system/TLVPacketBufferBackingStore.h>
 
 #include <nlunit-test.h>
@@ -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;
 

src/app/tests/integration/common.cpp

@@ -28,6 +28,7 @@
 #include <lib/core/CHIPCore.h>
 #include <lib/core/CHIPTLVDebug.hpp>
 #include <lib/support/ErrorStr.h>
+#include <lib/support/logging/Constants.h>
 #include <platform/CHIPDeviceLayer.h>
 
 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;
 

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);

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)
     {

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",

src/lib/core/CHIPTLV.h

@@ -36,6 +36,7 @@
 #include <lib/support/ScopedBuffer.h>
 #include <lib/support/Span.h>
 #include <lib/support/TypeTraits.h>
+#include <lib/support/logging/Constants.h>
 
 #include <stdarg.h>
 #include <stdlib.h>
@@ -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.

src/lib/core/tests/TestCHIPTLV.cpp

@@ -38,6 +38,7 @@
 #include <lib/support/ScopedBuffer.h>
 #include <lib/support/UnitTestRegistration.h>
 #include <lib/support/UnitTestUtils.h>
+#include <lib/support/logging/Constants.h>
 
 #include <system/TLVPacketBufferBackingStore.h>
 
@@ -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;
 

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);

src/lib/shell/streamer.cpp

@@ -23,6 +23,7 @@
 
 #include "streamer.h"
 
+#include <lib/support/logging/Constants.h>
 #include <limits.h>
 #include <stdio.h>
 
@@ -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;

src/lib/support/CHIPArgParser.cpp

@@ -48,6 +48,7 @@
 
 #include <lib/support/CHIPMem.h>
 #include <lib/support/CHIPMemString.h>
+#include <lib/support/logging/Constants.h>
 
 /*
  * 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;
 

src/lib/support/DefaultStorageKeyAllocator.h

@@ -17,6 +17,7 @@
 #pragma once
 
 #include <app/util/basic-types.h>
+#include <lib/support/logging/Constants.h>
 #include <string.h>
 
 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);

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);

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 {
 

src/lib/support/tests/TestCHIPArgParser.cpp

@@ -28,6 +28,7 @@
 #include <lib/support/CHIPMemString.h>
 #include <lib/support/ScopedBuffer.h>
 #include <lib/support/UnitTestRegistration.h>
+#include <lib/support/logging/Constants.h>
 
 #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;

src/platform/Darwin/Logging.cpp

@@ -4,7 +4,6 @@
 #include <platform/logging/LogV.h>
 
 #include <lib/core/CHIPConfig.h>
-#include <lib/support/logging/Constants.h>
 
 #include <os/log.h>
 
@@ -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);

src/platform/Linux/Logging.cpp

@@ -1,5 +1,6 @@
 /* See Project CHIP LICENSE file for licensing information. */
 
+#include <lib/support/logging/Constants.h>
 #include <platform/logging/LogV.h>
 
 #include <cinttypes>
@@ -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;
 

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);