Adding -Wformat-nonliteral check (#12763)

* 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.

* Address review comments

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

.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

@@ -32,8 +32,10 @@
 #include <lib/core/CHIPTLV.h>
 #include <lib/core/CHIPTLVDebug.hpp>
 #include <lib/core/CHIPTLVUtilities.hpp>
+#include <lib/support/EnforceFormat.h>
 #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 +91,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

@@ -37,7 +37,9 @@
 #include <lib/core/CHIPError.h>
 #include <lib/core/CHIPTLVDebug.hpp>
 #include <lib/support/CHIPMem.h>
+#include <lib/support/EnforceFormat.h>
 #include <lib/support/UnitTestRegistration.h>
+#include <lib/support/logging/Constants.h>
 #include <system/TLVPacketBufferBackingStore.h>
 
 #include <nlunit-test.h>
@@ -46,7 +48,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

@@ -27,7 +27,9 @@
 #include <app/tests/integration/common.h>
 #include <lib/core/CHIPCore.h>
 #include <lib/core/CHIPTLVDebug.hpp>
+#include <lib/support/EnforceFormat.h>
 #include <lib/support/ErrorStr.h>
+#include <lib/support/logging/Constants.h>
 #include <platform/CHIPDeviceLayer.h>
 
 chip::Messaging::ExchangeManager gExchangeManager;
@@ -65,7 +67,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

@@ -33,9 +33,11 @@
 
 #include <lib/support/BitFlags.h>
 #include <lib/support/DLLUtil.h>
+#include <lib/support/EnforceFormat.h>
 #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 +1616,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 +1664,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,8 @@
 
 #include "streamer.h"
 
+#include <lib/support/EnforceFormat.h>
+#include <lib/support/logging/Constants.h>
 #include <limits.h>
 #include <stdio.h>
 
@@ -48,7 +50,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 +66,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/BUILD.gn

@@ -40,6 +40,10 @@ source_set("logging_constants") {
   sources = [ "logging/Constants.h" ]
 }
 
+source_set("enforce_format") {
+  sources = [ "EnforceFormat.h" ]
+}
+
 source_set("chip_version_header") {
   sources = get_target_outputs(":gen_chip_version")
 
@@ -131,6 +135,7 @@ static_library("support") {
 
   public_deps = [
     ":chip_version_header",
+    ":enforce_format",
     ":logging_constants",
     "${chip_root}/src/lib/core:chip_config_header",
     "${chip_root}/src/platform:platform_buildconfig",

src/lib/support/CHIPArgParser.cpp

@@ -48,6 +48,8 @@
 
 #include <lib/support/CHIPMem.h>
 #include <lib/support/CHIPMemString.h>
+#include <lib/support/EnforceFormat.h>
+#include <lib/support/logging/Constants.h>
 
 /*
  * TODO: Revisit these if and when fabric ID and node ID support has
@@ -622,7 +624,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,8 @@
 #pragma once
 
 #include <app/util/basic-types.h>
+#include <lib/support/EnforceFormat.h>
+#include <lib/support/logging/Constants.h>
 #include <string.h>
 
 namespace chip {
@@ -48,7 +50,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/EnforceFormat.h

@@ -0,0 +1,34 @@
+/*
+ *    Copyright (c) 2022 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#pragma once
+
+/**
+ * gcc and clang provide a way to warn for a custom formatter when formats don't
+ * match arguments.  Use that 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__

src/lib/support/logging/CHIPLogging.h

@@ -39,6 +39,7 @@
 
 #include <platform/logging/LogV.h>
 
+#include <lib/support/EnforceFormat.h>
 #include <lib/support/logging/Constants.h>
 
 #include <inttypes.h>
@@ -79,20 +80,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);