Skip to content

Commit

Permalink
Turn on -Wformat-security and -Wformat-nonliteral.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
woody-apple authored and bzbarsky-apple committed Dec 21, 2021
1 parent 1d06bb2 commit e202c8d
Show file tree
Hide file tree
Showing 26 changed files with 82 additions and 36 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/examples-esp32.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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]'
Expand Down Expand Up @@ -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 \
Expand Down
3 changes: 3 additions & 0 deletions build/config/compiler/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,9 @@ config("strict_warnings") {
"-Wshadow",
"-Wunreachable-code",
"-Wvla",
"-Wformat",
"-Wformat-nonliteral",
"-Wformat-security",
]

cflags_cc = [ "-Wnon-virtual-dtor" ]
Expand Down
2 changes: 2 additions & 0 deletions config/ameba/chip.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ list(
-Wno-deprecated-declarations
-Wno-unused-parameter
-Wno-format
-Wno-format-nonliteral
-Wno-format-security
)

list(
Expand Down
3 changes: 3 additions & 0 deletions examples/all-clusters-app/esp32/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
2 changes: 1 addition & 1 deletion examples/platform/telink/util/src/ThreadUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 1 addition & 1 deletion src/app/MessageDef/MessageDefHelper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion src/app/tests/TestEventLogging.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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;

Expand Down
3 changes: 2 additions & 1 deletion src/app/tests/TestMessageDef.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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;

Expand Down
3 changes: 2 additions & 1 deletion src/app/tests/integration/common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down
7 changes: 7 additions & 0 deletions src/app/util/ember-print.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/controller/python/chip/logging/LoggingRedirect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
10 changes: 10 additions & 0 deletions src/darwin/Framework/CHIP.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
9 changes: 7 additions & 2 deletions src/lib/core/CHIPTLV.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
3 changes: 2 additions & 1 deletion src/lib/core/tests/TestCHIPTLV.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down Expand Up @@ -1628,7 +1629,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;

Expand Down
3 changes: 2 additions & 1 deletion src/lib/dnssd/Discovery_ImplPlatform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
5 changes: 3 additions & 2 deletions src/lib/shell/streamer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

#include "streamer.h"

#include <lib/support/logging/Constants.h>
#include <limits.h>
#include <stdio.h>

Expand All @@ -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;
Expand All @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion src/lib/support/CHIPArgParser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand Down
5 changes: 4 additions & 1 deletion src/lib/support/DefaultStorageKeyAllocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#pragma once

#include <app/util/basic-types.h>
#include <lib/support/logging/Constants.h>
#include <string.h>

namespace chip {
Expand Down Expand Up @@ -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);
Expand Down
15 changes: 1 addition & 14 deletions src/lib/support/logging/CHIPLogging.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
16 changes: 16 additions & 0 deletions src/lib/support/logging/Constants.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down
3 changes: 2 additions & 1 deletion src/lib/support/tests/TestCHIPArgParser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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;
Expand Down
3 changes: 1 addition & 2 deletions src/platform/Darwin/Logging.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#include <platform/logging/LogV.h>

#include <lib/core/CHIPConfig.h>
#include <lib/support/logging/Constants.h>

#include <os/log.h>

Expand All @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/platform/Linux/Logging.cpp
Original file line number Diff line number Diff line change
@@ -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>
Expand Down Expand Up @@ -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;

Expand Down
2 changes: 1 addition & 1 deletion src/platform/logging/impl/android/Logging.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading

0 comments on commit e202c8d

Please sign in to comment.