From bad2fdcd03b38b401304b06c46719db1d4aca67e Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Thu, 30 Oct 2025 13:39:23 +0100 Subject: [PATCH 01/16] Initial commit of bugprone-unsafe-format-string check --- bugprone-format-string-specification.md | 66 ++++++ .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt | 1 + .../bugprone/UnsafeFormatStringCheck.cpp | 146 ++++++++++++ .../bugprone/UnsafeFormatStringCheck.h | 38 +++ .../bugprone/unsafe-format-string.cpp | 222 ++++++++++++++++++ 6 files changed, 476 insertions(+) create mode 100644 bugprone-format-string-specification.md create mode 100644 clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp diff --git a/bugprone-format-string-specification.md b/bugprone-format-string-specification.md new file mode 100644 index 0000000000000..730c13aa0c85e --- /dev/null +++ b/bugprone-format-string-specification.md @@ -0,0 +1,66 @@ +# Clang-Tidy Check Specification: `bugprone-unsafe-format-string` + +## Overview +Detects usage of vulnerable format string functions with unbounded `%s` specifiers that can cause buffer overflows. + +## Check Name +`bugprone-unsafe-format-string` + +## Description +Warns when format string functions without built-in buffer limits use `%s` specifier without field width restrictions. + +## Targeted Functions +```cpp +// Output functions +sprintf, vsprintf + +// Input functions +scanf, fscanf, sscanf, vscanf, vfscanf, vsscanf + +// Wide character functions +wscanf, fwscanf, swscanf, vwscanf, vfwscanf, vswscanf +``` + +## Detection Logic +1. Match calls to vulnerable functions +2. Extract format string (literal or traceable constant) +3. Parse format string for `%s` specifiers +4. Flag `%s` without field width (e.g., `%10s` is safe, `%s` is not) + +## Examples + +**Unsafe (triggers warning):** +```cpp +char buf[100]; +sprintf(buf, "Hello %s", name); // Warning +scanf("%s", buffer); // Warning +sscanf(input, "%s %d", str, &num); // Warning +``` + +**Safe (no warning):** +```cpp +char buf[100]; +snprintf(buf, sizeof(buf), "Hello %s", name); // Safe alternative +sprintf(buf, "Hello %.99s", name); // Field width specified +scanf("%99s", buffer); // Field width specified +``` + +## Diagnostic Message +``` +warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] +``` + +## Fix-It Hints +- Suggest `snprintf()` for `sprintf()` +- Suggest field width for `%s` specifiers +- Provide buffer size calculation when possible + +## Configuration Options +- `MaxFieldWidth`: Maximum reasonable field width (default: 4096) +- `SuggestAlternatives`: Enable fix-it suggestions (default: true) + +## Implementation Notes +- Handle both string literals and const char* format parameters +- Support variadic and va_list variants +- Consider format string concatenation patterns +- Integrate with existing `bugprone` module diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index e6115f67656bc..751282242d1cc 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -100,6 +100,7 @@ #include "UnintendedCharOstreamOutputCheck.h" #include "UniquePtrArrayMismatchCheck.h" #include "UnsafeFunctionsCheck.h" +#include "UnsafeFormatStringCheck.h" #include "UnusedLocalNonTrivialVariableCheck.h" #include "UnusedRaiiCheck.h" #include "UnusedReturnValueCheck.h" @@ -287,6 +288,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-crtp-constructor-accessibility"); CheckFactories.registerCheck( "bugprone-unsafe-functions"); + CheckFactories.registerCheck( + "bugprone-unsafe-format-string"); CheckFactories.registerCheck( "bugprone-unused-local-non-trivial-variable"); CheckFactories.registerCheck("bugprone-unused-raii"); diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index c8943e5b22ef8..0663e7c697169 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -101,6 +101,7 @@ add_clang_library(clangTidyBugproneModule STATIC UnhandledSelfAssignmentCheck.cpp UniquePtrArrayMismatchCheck.cpp UnsafeFunctionsCheck.cpp + UnsafeFormatStringCheck.cpp UnusedLocalNonTrivialVariableCheck.cpp UnusedRaiiCheck.cpp UnusedReturnValueCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp new file mode 100644 index 0000000000000..995376c70ede0 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp @@ -0,0 +1,146 @@ +//===--- UnsafeFormatStringCheck.cpp - clang-tidy -----------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "UnsafeFormatStringCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +UnsafeFormatStringCheck::UnsafeFormatStringCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + MaxFieldWidth(Options.get("MaxFieldWidth", 4096U)), + SuggestAlternatives(Options.get("SuggestAlternatives", true)) {} + +void UnsafeFormatStringCheck::registerMatchers(MatchFinder *Finder) { + // Match vulnerable format string functions + auto VulnerableFunctions = hasAnyName( + "sprintf", "vsprintf", "scanf", "fscanf", "sscanf", "vscanf", "vfscanf", + "vsscanf", "wscanf", "fwscanf", "swscanf", "vwscanf", "vfwscanf", "vswscanf"); + + Finder->addMatcher( + callExpr(callee(functionDecl(VulnerableFunctions)), + anyOf(hasArgument(0, stringLiteral().bind("format")), + hasArgument(1, stringLiteral().bind("format")))) + .bind("call"), + this); +} + +void UnsafeFormatStringCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Call = Result.Nodes.getNodeAs("call"); + const auto *Format = Result.Nodes.getNodeAs("format"); + + if (!Call || !Format) + return; + + StringRef FormatString = Format->getString(); + if (!hasUnboundedStringSpecifier(FormatString)) + return; + + const auto *Callee = cast(Call->getCalleeDecl()); + StringRef FunctionName = Callee->getName(); + + auto Diag = diag(Call->getBeginLoc(), + "format specifier '%%s' without field width may cause " + "buffer overflow") + << Call->getSourceRange(); + + if (SuggestAlternatives) { + std::string SafeAlternative = getSafeAlternative(FunctionName); + if (!SafeAlternative.empty()) { + Diag << FixItHint::CreateInsertion(Call->getBeginLoc(), + "/* Consider using " + SafeAlternative + " */ "); + } + } +} + +void UnsafeFormatStringCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "MaxFieldWidth", MaxFieldWidth); + Options.store(Opts, "SuggestAlternatives", SuggestAlternatives); +} + +bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString) { + size_t Pos = 0; + while ((Pos = FormatString.find('%', Pos)) != StringRef::npos) { + if (Pos + 1 >= FormatString.size()) + break; + + // Skip %% + if (FormatString[Pos + 1] == '%') { + Pos += 2; + continue; + } + + size_t SpecStart = Pos + 1; + size_t SpecPos = SpecStart; + + // Skip flags + while (SpecPos < FormatString.size() && + (FormatString[SpecPos] == '-' || FormatString[SpecPos] == '+' || + FormatString[SpecPos] == ' ' || FormatString[SpecPos] == '#' || + FormatString[SpecPos] == '0')) { + SpecPos++; + } + + // Check for field width + bool HasFieldWidth = false; + if (SpecPos < FormatString.size() && FormatString[SpecPos] == '*') { + HasFieldWidth = true; + SpecPos++; + } else { + while (SpecPos < FormatString.size() && isdigit(FormatString[SpecPos])) { + HasFieldWidth = true; + SpecPos++; + } + } + + // Skip precision + if (SpecPos < FormatString.size() && FormatString[SpecPos] == '.') { + SpecPos++; + if (SpecPos < FormatString.size() && FormatString[SpecPos] == '*') { + SpecPos++; + } else { + while (SpecPos < FormatString.size() && isdigit(FormatString[SpecPos])) { + SpecPos++; + } + } + } + + // Skip length modifiers + while (SpecPos < FormatString.size() && + (FormatString[SpecPos] == 'h' || FormatString[SpecPos] == 'l' || + FormatString[SpecPos] == 'L' || FormatString[SpecPos] == 'z' || + FormatString[SpecPos] == 'j' || FormatString[SpecPos] == 't')) { + SpecPos++; + } + + // Check for 's' specifier without field width + if (SpecPos < FormatString.size() && FormatString[SpecPos] == 's' && !HasFieldWidth) { + return true; + } + + Pos = SpecPos + 1; + } + + return false; +} + +std::string UnsafeFormatStringCheck::getSafeAlternative(StringRef FunctionName) { + if (FunctionName == "sprintf") + return "snprintf"; + if (FunctionName == "vsprintf") + return "vsnprintf"; + if (FunctionName.starts_with("scanf") || FunctionName.ends_with("scanf")) + return "add field width to %s specifiers"; + return ""; +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h new file mode 100644 index 0000000000000..68dfc826c9584 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h @@ -0,0 +1,38 @@ +//===--- UnsafeFormatStringCheck.h - clang-tidy ---------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFEFORMATSTRINGCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFEFORMATSTRINGCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Detects usage of vulnerable format string functions with unbounded %s +/// specifiers that can cause buffer overflows. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/unsafe-format-string.html +class UnsafeFormatStringCheck : public ClangTidyCheck { +public: + UnsafeFormatStringCheck(StringRef Name, ClangTidyContext *Context); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + bool hasUnboundedStringSpecifier(StringRef FormatString); + std::string getSafeAlternative(StringRef FunctionName); + + unsigned MaxFieldWidth; + bool SuggestAlternatives; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFEFORMATSTRINGCHECK_H diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp new file mode 100644 index 0000000000000..18589139c6d59 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp @@ -0,0 +1,222 @@ +// RUN: %check_clang_tidy %s bugprone-unsafe-format-string %t + +#include +#include +#include + +void test_sprintf() { + char buffer[100]; + const char* input = "user input"; + + // Positive: unsafe %s without field width + sprintf(buffer, "%s", input); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + // Negative: safe %s with field width + sprintf(buffer, "%99s", input); + // no-warning + + // Negative: other format specifiers are safe + sprintf(buffer, "%d %f", 42, 3.14); + // no-warning +} + +void test_vsprintf_wrapper(const char* format, ...) { + char buffer[100]; + va_list args; + va_start(args, format); + + // Positive: unsafe %s without field width + vsprintf(buffer, format, args); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + va_end(args); +} + +void test_vsprintf_safe_wrapper(const char* format, ...) { + char buffer[100]; + va_list args; + va_start(args, format); + + // Negative: vsnprintf is safe + vsnprintf(buffer, sizeof(buffer), format, args); + // no-warning + + va_end(args); +} + +void test_scanf() { + char buffer[100]; + + // Positive: unsafe %s without field width + scanf("%s", buffer); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + // Negative: safe %s with field width + scanf("%99s", buffer); + // no-warning +} + +void test_fscanf() { + char buffer[100]; + FILE* file = nullptr; + + // Positive: unsafe %s without field width + fscanf(file, "%s", buffer); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + // Negative: safe %s with field width + fscanf(file, "%99s", buffer); + // no-warning +} + +void test_sscanf() { + char buffer[100]; + const char* source = "input"; + + // Positive: unsafe %s without field width + sscanf(source, "%s", buffer); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + // Negative: safe %s with field width + sscanf(source, "%99s", buffer); + // no-warning +} + +void test_vfscanf_wrapper(FILE* file, const char* format, ...) { + va_list args; + va_start(args, format); + + // Positive: unsafe %s without field width + vfscanf(file, format, args); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + va_end(args); +} + +void test_vsscanf_wrapper(const char* source, const char* format, ...) { + va_list args; + va_start(args, format); + + // Positive: unsafe %s without field width + vsscanf(source, format, args); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + va_end(args); +} + +void test_vscanf_wrapper(const char* format, ...) { + va_list args; + va_start(args, format); + + // Positive: unsafe %s without field width + vscanf(format, args); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + va_end(args); +} + +void test_wscanf() { + wchar_t buffer[100]; + + // Positive: unsafe %s without field width + wscanf(L"%s", buffer); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + // Negative: safe %s with field width + wscanf(L"%99s", buffer); + // no-warning +} + +void test_fwscanf() { + wchar_t buffer[100]; + FILE* file = nullptr; + + // Positive: unsafe %s without field width + fwscanf(file, L"%s", buffer); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + // Negative: safe %s with field width + fwscanf(file, L"%99s", buffer); + // no-warning +} + +void test_swscanf() { + wchar_t buffer[100]; + const wchar_t* source = L"input"; + + // Positive: unsafe %s without field width + swscanf(source, L"%s", buffer); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + // Negative: safe %s with field width + swscanf(source, L"%99s", buffer); + // no-warning +} + +void test_vwscanf_wrapper(const wchar_t* format, ...) { + va_list args; + va_start(args, format); + + // Positive: unsafe %s without field width + vwscanf(format, args); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + va_end(args); +} + +void test_vfwscanf_wrapper(FILE* file, const wchar_t* format, ...) { + va_list args; + va_start(args, format); + + // Positive: unsafe %s without field width + vfwscanf(file, format, args); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + va_end(args); +} + +void test_vswscanf_wrapper(const wchar_t* source, const wchar_t* format, ...) { + va_list args; + va_start(args, format); + + // Positive: unsafe %s without field width + vswscanf(source, format, args); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + + va_end(args); +} + +void test_call_variadic_functions() { + char buffer[100]; + wchar_t wbuffer[100]; + FILE* file = fopen("test.txt", "r"); + + // Test calls to variadic wrapper functions + test_vsprintf_wrapper("%s", "unsafe"); + test_vfscanf_wrapper(file, "%s", buffer); + test_vsscanf_wrapper("source", "%s", buffer); + test_vscanf_wrapper("%s", buffer); + test_vwscanf_wrapper(L"%s", wbuffer); + test_vfwscanf_wrapper(file, L"%s", wbuffer); + test_vswscanf_wrapper(L"source", L"%s", wbuffer); + + if (file) fclose(file); +} + +void test_safe_alternatives() { + char buffer[100]; + const char* input = "user input"; + + // Negative: snprintf is inherently safe + snprintf(buffer, sizeof(buffer), "%s", input); + // no-warning + + // Negative: printf family doesn't write to buffers + printf("%s", input); + // no-warning + + // Negative: fprintf doesn't write to user buffers + fprintf(stderr, "%s", input); + // no-warning +} From e752e5462b1e772248739f3c1fb849f076d8ef44 Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Thu, 30 Oct 2025 14:28:01 +0100 Subject: [PATCH 02/16] remove MaxFieldWidth configuration option as it is not needed --- bugprone-format-string-specification.md | 1 - .../clang-tidy/bugprone/UnsafeFormatStringCheck.cpp | 2 -- clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h | 1 - 3 files changed, 4 deletions(-) diff --git a/bugprone-format-string-specification.md b/bugprone-format-string-specification.md index 730c13aa0c85e..57f3c5dc6b38e 100644 --- a/bugprone-format-string-specification.md +++ b/bugprone-format-string-specification.md @@ -56,7 +56,6 @@ warning: format specifier '%s' without field width may cause buffer overflow [bu - Provide buffer size calculation when possible ## Configuration Options -- `MaxFieldWidth`: Maximum reasonable field width (default: 4096) - `SuggestAlternatives`: Enable fix-it suggestions (default: true) ## Implementation Notes diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp index 995376c70ede0..b8333658bf0c6 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp @@ -17,7 +17,6 @@ namespace clang::tidy::bugprone { UnsafeFormatStringCheck::UnsafeFormatStringCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), - MaxFieldWidth(Options.get("MaxFieldWidth", 4096U)), SuggestAlternatives(Options.get("SuggestAlternatives", true)) {} void UnsafeFormatStringCheck::registerMatchers(MatchFinder *Finder) { @@ -63,7 +62,6 @@ void UnsafeFormatStringCheck::check(const MatchFinder::MatchResult &Result) { } void UnsafeFormatStringCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { - Options.store(Opts, "MaxFieldWidth", MaxFieldWidth); Options.store(Opts, "SuggestAlternatives", SuggestAlternatives); } diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h index 68dfc826c9584..24e6c40ee8fbb 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h @@ -29,7 +29,6 @@ class UnsafeFormatStringCheck : public ClangTidyCheck { bool hasUnboundedStringSpecifier(StringRef FormatString); std::string getSafeAlternative(StringRef FunctionName); - unsigned MaxFieldWidth; bool SuggestAlternatives; }; From 688c58294cace84b664dffc038fc82fdd581fe74 Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Thu, 30 Oct 2025 14:35:44 +0100 Subject: [PATCH 03/16] Remove SuggestAlternatives configuration option as it is not needed --- bugprone-format-string-specification.md | 3 --- .../bugprone/UnsafeFormatStringCheck.cpp | 16 +++++----------- .../bugprone/UnsafeFormatStringCheck.h | 3 --- 3 files changed, 5 insertions(+), 17 deletions(-) diff --git a/bugprone-format-string-specification.md b/bugprone-format-string-specification.md index 57f3c5dc6b38e..58e5dfcedb669 100644 --- a/bugprone-format-string-specification.md +++ b/bugprone-format-string-specification.md @@ -55,9 +55,6 @@ warning: format specifier '%s' without field width may cause buffer overflow [bu - Suggest field width for `%s` specifiers - Provide buffer size calculation when possible -## Configuration Options -- `SuggestAlternatives`: Enable fix-it suggestions (default: true) - ## Implementation Notes - Handle both string literals and const char* format parameters - Support variadic and va_list variants diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp index b8333658bf0c6..4ee8f8e8ab799 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp @@ -16,8 +16,7 @@ namespace clang::tidy::bugprone { UnsafeFormatStringCheck::UnsafeFormatStringCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context), - SuggestAlternatives(Options.get("SuggestAlternatives", true)) {} + : ClangTidyCheck(Name, Context) {} void UnsafeFormatStringCheck::registerMatchers(MatchFinder *Finder) { // Match vulnerable format string functions @@ -52,18 +51,13 @@ void UnsafeFormatStringCheck::check(const MatchFinder::MatchResult &Result) { "buffer overflow") << Call->getSourceRange(); - if (SuggestAlternatives) { - std::string SafeAlternative = getSafeAlternative(FunctionName); - if (!SafeAlternative.empty()) { - Diag << FixItHint::CreateInsertion(Call->getBeginLoc(), - "/* Consider using " + SafeAlternative + " */ "); - } + std::string SafeAlternative = getSafeAlternative(FunctionName); + if (!SafeAlternative.empty()) { + Diag << FixItHint::CreateInsertion(Call->getBeginLoc(), + "/* Consider using " + SafeAlternative + " */ "); } } -void UnsafeFormatStringCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { - Options.store(Opts, "SuggestAlternatives", SuggestAlternatives); -} bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString) { size_t Pos = 0; diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h index 24e6c40ee8fbb..4fd9e0d863f76 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h @@ -23,13 +23,10 @@ class UnsafeFormatStringCheck : public ClangTidyCheck { UnsafeFormatStringCheck(StringRef Name, ClangTidyContext *Context); void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; - void storeOptions(ClangTidyOptions::OptionMap &Opts) override; private: bool hasUnboundedStringSpecifier(StringRef FormatString); std::string getSafeAlternative(StringRef FunctionName); - - bool SuggestAlternatives; }; } // namespace clang::tidy::bugprone From 9947b3cee99ac6b3bfd270c7f7ba21f5cfb5d80f Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Thu, 30 Oct 2025 15:29:29 +0100 Subject: [PATCH 04/16] Changing the test from C++ to C --- ...rmat-string.cpp => unsafe-format-string.c} | 116 +++++++++--------- 1 file changed, 58 insertions(+), 58 deletions(-) rename clang-tools-extra/test/clang-tidy/checkers/bugprone/{unsafe-format-string.cpp => unsafe-format-string.c} (56%) diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c similarity index 56% rename from clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp rename to clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c index 18589139c6d59..f7d976391ab64 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c @@ -1,24 +1,24 @@ // RUN: %check_clang_tidy %s bugprone-unsafe-format-string %t -#include -#include -#include +#include +#include +#include void test_sprintf() { char buffer[100]; const char* input = "user input"; - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ sprintf(buffer, "%s", input); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - // Negative: safe %s with field width + /* Negative: safe %s with field width */ sprintf(buffer, "%99s", input); - // no-warning + /* no-warning */ - // Negative: other format specifiers are safe + /* Negative: other format specifiers are safe */ sprintf(buffer, "%d %f", 42, 3.14); - // no-warning + /* no-warning */ } void test_vsprintf_wrapper(const char* format, ...) { @@ -26,9 +26,9 @@ void test_vsprintf_wrapper(const char* format, ...) { va_list args; va_start(args, format); - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ vsprintf(buffer, format, args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] va_end(args); } @@ -38,9 +38,9 @@ void test_vsprintf_safe_wrapper(const char* format, ...) { va_list args; va_start(args, format); - // Negative: vsnprintf is safe + /* Negative: vsnprintf is safe */ vsnprintf(buffer, sizeof(buffer), format, args); - // no-warning + /* no-warning */ va_end(args); } @@ -48,48 +48,48 @@ void test_vsprintf_safe_wrapper(const char* format, ...) { void test_scanf() { char buffer[100]; - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ scanf("%s", buffer); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - // Negative: safe %s with field width + /* Negative: safe %s with field width */ scanf("%99s", buffer); - // no-warning + /* no-warning */ } void test_fscanf() { char buffer[100]; - FILE* file = nullptr; + FILE* file = 0; - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ fscanf(file, "%s", buffer); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - // Negative: safe %s with field width + /* Negative: safe %s with field width */ fscanf(file, "%99s", buffer); - // no-warning + /* no-warning */ } void test_sscanf() { char buffer[100]; const char* source = "input"; - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ sscanf(source, "%s", buffer); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - // Negative: safe %s with field width + /* Negative: safe %s with field width */ sscanf(source, "%99s", buffer); - // no-warning + /* no-warning */ } void test_vfscanf_wrapper(FILE* file, const char* format, ...) { va_list args; va_start(args, format); - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ vfscanf(file, format, args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] va_end(args); } @@ -98,9 +98,9 @@ void test_vsscanf_wrapper(const char* source, const char* format, ...) { va_list args; va_start(args, format); - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ vsscanf(source, format, args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] va_end(args); } @@ -109,9 +109,9 @@ void test_vscanf_wrapper(const char* format, ...) { va_list args; va_start(args, format); - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ vscanf(format, args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] va_end(args); } @@ -119,48 +119,48 @@ void test_vscanf_wrapper(const char* format, ...) { void test_wscanf() { wchar_t buffer[100]; - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ wscanf(L"%s", buffer); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - // Negative: safe %s with field width + /* Negative: safe %s with field width */ wscanf(L"%99s", buffer); - // no-warning + /* no-warning */ } void test_fwscanf() { wchar_t buffer[100]; - FILE* file = nullptr; + FILE* file = 0; - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ fwscanf(file, L"%s", buffer); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - // Negative: safe %s with field width + /* Negative: safe %s with field width */ fwscanf(file, L"%99s", buffer); - // no-warning + /* no-warning */ } void test_swscanf() { wchar_t buffer[100]; const wchar_t* source = L"input"; - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ swscanf(source, L"%s", buffer); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - // Negative: safe %s with field width + /* Negative: safe %s with field width */ swscanf(source, L"%99s", buffer); - // no-warning + /* no-warning */ } void test_vwscanf_wrapper(const wchar_t* format, ...) { va_list args; va_start(args, format); - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ vwscanf(format, args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] va_end(args); } @@ -169,9 +169,9 @@ void test_vfwscanf_wrapper(FILE* file, const wchar_t* format, ...) { va_list args; va_start(args, format); - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ vfwscanf(file, format, args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] va_end(args); } @@ -180,9 +180,9 @@ void test_vswscanf_wrapper(const wchar_t* source, const wchar_t* format, ...) { va_list args; va_start(args, format); - // Positive: unsafe %s without field width + /* Positive: unsafe %s without field width */ vswscanf(source, format, args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' is unsafe without field width constraint; consider using '%99s' or snprintf [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] va_end(args); } @@ -192,7 +192,7 @@ void test_call_variadic_functions() { wchar_t wbuffer[100]; FILE* file = fopen("test.txt", "r"); - // Test calls to variadic wrapper functions + /* Test calls to variadic wrapper functions */ test_vsprintf_wrapper("%s", "unsafe"); test_vfscanf_wrapper(file, "%s", buffer); test_vsscanf_wrapper("source", "%s", buffer); @@ -208,15 +208,15 @@ void test_safe_alternatives() { char buffer[100]; const char* input = "user input"; - // Negative: snprintf is inherently safe + /* Negative: snprintf is inherently safe */ snprintf(buffer, sizeof(buffer), "%s", input); - // no-warning + /* no-warning */ - // Negative: printf family doesn't write to buffers + /* Negative: printf family doesn't write to buffers */ printf("%s", input); - // no-warning + /* no-warning */ - // Negative: fprintf doesn't write to user buffers + /* Negative: fprintf doesn't write to user buffers */ fprintf(stderr, "%s", input); - // no-warning + /* no-warning */ } From a5f6717c86d53f0a0a54a424fd715d79c8353adf Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Thu, 30 Oct 2025 15:30:10 +0100 Subject: [PATCH 05/16] Add wide character handling in the format string --- .../clang-tidy/bugprone/UnsafeFormatStringCheck.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp index 4ee8f8e8ab799..7e8d34057121c 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp @@ -39,7 +39,14 @@ void UnsafeFormatStringCheck::check(const MatchFinder::MatchResult &Result) { if (!Call || !Format) return; - StringRef FormatString = Format->getString(); + std::string FormatString; + if (Format->getCharByteWidth() == 1) { + FormatString = Format->getString().str(); + } else { + // Handle wide strings by converting to narrow string for analysis + FormatString = Format->getBytes().str(); + } + if (!hasUnboundedStringSpecifier(FormatString)) return; From dd5eaca42aff675862422cf7e92770155a1bf604 Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Thu, 30 Oct 2025 15:51:47 +0100 Subject: [PATCH 06/16] Fixing the test cases to use string literals in the va_list tests --- .../checkers/bugprone/unsafe-format-string.c | 70 +++++-------------- 1 file changed, 18 insertions(+), 52 deletions(-) diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c index f7d976391ab64..9a1c451c35b79 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c @@ -21,16 +21,13 @@ void test_sprintf() { /* no-warning */ } -void test_vsprintf_wrapper(const char* format, ...) { +void test_vsprintf() { char buffer[100]; va_list args; - va_start(args, format); /* Positive: unsafe %s without field width */ - vsprintf(buffer, format, args); + vsprintf(buffer, "%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - - va_end(args); } void test_vsprintf_safe_wrapper(const char* format, ...) { @@ -83,37 +80,30 @@ void test_sscanf() { /* no-warning */ } -void test_vfscanf_wrapper(FILE* file, const char* format, ...) { +void test_vfscanf() { + FILE* file = 0; va_list args; - va_start(args, format); /* Positive: unsafe %s without field width */ - vfscanf(file, format, args); + vfscanf(file, "%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - - va_end(args); } -void test_vsscanf_wrapper(const char* source, const char* format, ...) { +void test_vsscanf() { + const char* source = "input"; va_list args; - va_start(args, format); /* Positive: unsafe %s without field width */ - vsscanf(source, format, args); + vsscanf(source, "%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - - va_end(args); } -void test_vscanf_wrapper(const char* format, ...) { +void test_vscanf() { va_list args; - va_start(args, format); /* Positive: unsafe %s without field width */ - vscanf(format, args); + vscanf("%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - - va_end(args); } void test_wscanf() { @@ -154,54 +144,30 @@ void test_swscanf() { /* no-warning */ } -void test_vwscanf_wrapper(const wchar_t* format, ...) { +void test_vwscanf() { va_list args; - va_start(args, format); /* Positive: unsafe %s without field width */ - vwscanf(format, args); + vwscanf(L"%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - - va_end(args); } -void test_vfwscanf_wrapper(FILE* file, const wchar_t* format, ...) { +void test_vfwscanf() { + FILE* file = 0; va_list args; - va_start(args, format); /* Positive: unsafe %s without field width */ - vfwscanf(file, format, args); + vfwscanf(file, L"%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - - va_end(args); } -void test_vswscanf_wrapper(const wchar_t* source, const wchar_t* format, ...) { +void test_vswscanf() { + const wchar_t* source = L"input"; va_list args; - va_start(args, format); /* Positive: unsafe %s without field width */ - vswscanf(source, format, args); + vswscanf(source, L"%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] - - va_end(args); -} - -void test_call_variadic_functions() { - char buffer[100]; - wchar_t wbuffer[100]; - FILE* file = fopen("test.txt", "r"); - - /* Test calls to variadic wrapper functions */ - test_vsprintf_wrapper("%s", "unsafe"); - test_vfscanf_wrapper(file, "%s", buffer); - test_vsscanf_wrapper("source", "%s", buffer); - test_vscanf_wrapper("%s", buffer); - test_vwscanf_wrapper(L"%s", wbuffer); - test_vfwscanf_wrapper(file, L"%s", wbuffer); - test_vswscanf_wrapper(L"source", L"%s", wbuffer); - - if (file) fclose(file); } void test_safe_alternatives() { From 73a64ff1bba54ce2ae9e0f1be431b48cedba71a3 Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Thu, 30 Oct 2025 17:07:03 +0100 Subject: [PATCH 07/16] Handling wchar_t properly. Solving it manually (Q could not do it). --- .../bugprone/UnsafeFormatStringCheck.cpp | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp index 7e8d34057121c..445f08dbcd533 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp @@ -9,6 +9,8 @@ #include "UnsafeFormatStringCheck.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Lex/Lexer.h" +#include "llvm/Support/ConvertUTF.h" +#include "llvm/Support/raw_ostream.h" using namespace clang::ast_matchers; @@ -33,18 +35,22 @@ void UnsafeFormatStringCheck::registerMatchers(MatchFinder *Finder) { } void UnsafeFormatStringCheck::check(const MatchFinder::MatchResult &Result) { + llvm::errs()<<"UnsafeFormatStringCheck::check"; const auto *Call = Result.Nodes.getNodeAs("call"); const auto *Format = Result.Nodes.getNodeAs("format"); - + if (!Call || !Format) return; std::string FormatString; if (Format->getCharByteWidth() == 1) { FormatString = Format->getString().str(); - } else { + } else if (Format->getCharByteWidth() == 2) { + // Handle wide strings by converting to narrow string for analysis + convertUTF16ToUTF8String(Format->getBytes(), FormatString); + } else if (Format->getCharByteWidth() == 4) { // Handle wide strings by converting to narrow string for analysis - FormatString = Format->getBytes().str(); + convertUTF32ToUTF8String(Format->getBytes(), FormatString); } if (!hasUnboundedStringSpecifier(FormatString)) @@ -71,24 +77,24 @@ bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString while ((Pos = FormatString.find('%', Pos)) != StringRef::npos) { if (Pos + 1 >= FormatString.size()) break; - + // Skip %% if (FormatString[Pos + 1] == '%') { Pos += 2; continue; } - + size_t SpecStart = Pos + 1; size_t SpecPos = SpecStart; - + // Skip flags - while (SpecPos < FormatString.size() && + while (SpecPos < FormatString.size() && (FormatString[SpecPos] == '-' || FormatString[SpecPos] == '+' || FormatString[SpecPos] == ' ' || FormatString[SpecPos] == '#' || FormatString[SpecPos] == '0')) { SpecPos++; } - + // Check for field width bool HasFieldWidth = false; if (SpecPos < FormatString.size() && FormatString[SpecPos] == '*') { @@ -100,7 +106,7 @@ bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString SpecPos++; } } - + // Skip precision if (SpecPos < FormatString.size() && FormatString[SpecPos] == '.') { SpecPos++; @@ -112,23 +118,23 @@ bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString } } } - + // Skip length modifiers - while (SpecPos < FormatString.size() && + while (SpecPos < FormatString.size() && (FormatString[SpecPos] == 'h' || FormatString[SpecPos] == 'l' || FormatString[SpecPos] == 'L' || FormatString[SpecPos] == 'z' || FormatString[SpecPos] == 'j' || FormatString[SpecPos] == 't')) { SpecPos++; } - + // Check for 's' specifier without field width if (SpecPos < FormatString.size() && FormatString[SpecPos] == 's' && !HasFieldWidth) { return true; } - + Pos = SpecPos + 1; } - + return false; } From 3902f9a71cae4bb3ac45e74b11b2c3f05ce0fd85 Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Thu, 30 Oct 2025 17:11:20 +0100 Subject: [PATCH 08/16] Adding checker documentation --- .../checks/bugprone/unsafe-format-string.rst | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-format-string.rst diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-format-string.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-format-string.rst new file mode 100644 index 0000000000000..a7a6c6d2b71c0 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-format-string.rst @@ -0,0 +1,49 @@ +.. title:: clang-tidy - bugprone-unsafe-format-string + +bugprone-unsafe-format-string +============================== + +Detects usage of vulnerable format string functions with unbounded ``%s`` +specifiers that can cause buffer overflows. + +The check identifies calls to format string functions like ``sprintf``, ``scanf``, +and their variants that use ``%s`` format specifiers without field width limits. +This can lead to buffer overflow vulnerabilities when the input string is longer +than the destination buffer. + +Examples +-------- + +.. code-block:: c + + char buffer[100]; + const char* input = "user input"; + + // Unsafe: no field width limit + sprintf(buffer, "%s", input); + scanf("%s", buffer); + + // Safe: field width specified + sprintf(buffer, "%.99s", input); + scanf("%99s", buffer); + + // Safe alternative: use safer functions + snprintf(buffer, sizeof(buffer), "%s", input); + +Checked Functions +----------------- + +The check detects unsafe format strings in these functions: + +* ``sprintf``, ``vsprintf`` +* ``scanf``, ``fscanf``, ``sscanf`` +* ``vscanf``, ``vfscanf``, ``vsscanf`` +* ``wscanf``, ``fwscanf``, ``swscanf`` +* ``vwscanf``, ``vfwscanf``, ``vswscanf`` + +Recommendations +--------------- + +* Use ``snprintf`` instead of ``sprintf`` to prevent buffer overflows +* Add field width specifiers to ``%s`` format specifiers (e.g., ``%99s``) +* Consider using safer string handling functions when possible From a3b354671b24c30d9f38a145e0af1dd63297ee73 Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Thu, 30 Oct 2025 17:16:14 +0100 Subject: [PATCH 09/16] adding negative test cases --- .../checkers/bugprone/unsafe-format-string.c | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c index 9a1c451c35b79..9dfcfb88cfaa0 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c @@ -28,6 +28,10 @@ void test_vsprintf() { /* Positive: unsafe %s without field width */ vsprintf(buffer, "%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + + /* Negative: safe %s with field width */ + vsprintf(buffer, "%99s", args); + /* no-warning */ } void test_vsprintf_safe_wrapper(const char* format, ...) { @@ -87,6 +91,10 @@ void test_vfscanf() { /* Positive: unsafe %s without field width */ vfscanf(file, "%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + + /* Negative: safe %s with field width */ + vfscanf(file, "%99s", args); + /* no-warning */ } void test_vsscanf() { @@ -96,6 +104,10 @@ void test_vsscanf() { /* Positive: unsafe %s without field width */ vsscanf(source, "%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + + /* Negative: safe %s with field width */ + vsscanf(source, "%99s", args); + /* no-warning */ } void test_vscanf() { @@ -104,6 +116,10 @@ void test_vscanf() { /* Positive: unsafe %s without field width */ vscanf("%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + + /* Negative: safe %s with field width */ + vscanf("%99s", args); + /* no-warning */ } void test_wscanf() { @@ -150,6 +166,10 @@ void test_vwscanf() { /* Positive: unsafe %s without field width */ vwscanf(L"%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + + /* Negative: safe %s with field width */ + vwscanf(L"%99s", args); + /* no-warning */ } void test_vfwscanf() { @@ -159,6 +179,10 @@ void test_vfwscanf() { /* Positive: unsafe %s without field width */ vfwscanf(file, L"%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + + /* Negative: safe %s with field width */ + vfwscanf(file, L"%99s", args); + /* no-warning */ } void test_vswscanf() { @@ -168,6 +192,10 @@ void test_vswscanf() { /* Positive: unsafe %s without field width */ vswscanf(source, L"%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + + /* Negative: safe %s with field width */ + vswscanf(source, L"%99s", args); + /* no-warning */ } void test_safe_alternatives() { From e6199a5cbb40b59d60a9f9e2ebb396ff4cc7158e Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Thu, 30 Oct 2025 18:21:00 +0100 Subject: [PATCH 10/16] handle sprintf and scanf differently --- .../bugprone/UnsafeFormatStringCheck.cpp | 40 +++++++++++------ .../bugprone/UnsafeFormatStringCheck.h | 2 +- .../checks/bugprone/unsafe-format-string.rst | 42 ++++++++++++++---- .../checkers/bugprone/unsafe-format-string.c | 44 ++++++++++++------- 4 files changed, 89 insertions(+), 39 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp index 445f08dbcd533..f4263c034e423 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp @@ -35,7 +35,6 @@ void UnsafeFormatStringCheck::registerMatchers(MatchFinder *Finder) { } void UnsafeFormatStringCheck::check(const MatchFinder::MatchResult &Result) { - llvm::errs()<<"UnsafeFormatStringCheck::check"; const auto *Call = Result.Nodes.getNodeAs("call"); const auto *Format = Result.Nodes.getNodeAs("format"); @@ -53,15 +52,18 @@ void UnsafeFormatStringCheck::check(const MatchFinder::MatchResult &Result) { convertUTF32ToUTF8String(Format->getBytes(), FormatString); } - if (!hasUnboundedStringSpecifier(FormatString)) - return; - const auto *Callee = cast(Call->getCalleeDecl()); StringRef FunctionName = Callee->getName(); + + bool IsScanfFamily = FunctionName.contains("scanf"); + + if (!hasUnboundedStringSpecifier(FormatString, IsScanfFamily)) + return; auto Diag = diag(Call->getBeginLoc(), - "format specifier '%%s' without field width may cause " - "buffer overflow") + IsScanfFamily + ? "format specifier '%%s' without field width may cause buffer overflow; consider using '%%Ns' where N limits input length" + : "format specifier '%%s' without precision may cause buffer overflow; consider using '%%.Ns' where N limits output length") << Call->getSourceRange(); std::string SafeAlternative = getSafeAlternative(FunctionName); @@ -72,7 +74,7 @@ void UnsafeFormatStringCheck::check(const MatchFinder::MatchResult &Result) { } -bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString) { +bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString, bool IsScanfFamily) { size_t Pos = 0; while ((Pos = FormatString.find('%', Pos)) != StringRef::npos) { if (Pos + 1 >= FormatString.size()) @@ -84,8 +86,7 @@ bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString continue; } - size_t SpecStart = Pos + 1; - size_t SpecPos = SpecStart; + size_t SpecPos = Pos + 1; // Skip flags while (SpecPos < FormatString.size() && @@ -107,13 +108,16 @@ bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString } } - // Skip precision + // Check for precision + bool HasPrecision = false; if (SpecPos < FormatString.size() && FormatString[SpecPos] == '.') { SpecPos++; if (SpecPos < FormatString.size() && FormatString[SpecPos] == '*') { + HasPrecision = true; SpecPos++; } else { while (SpecPos < FormatString.size() && isdigit(FormatString[SpecPos])) { + HasPrecision = true; SpecPos++; } } @@ -127,9 +131,19 @@ bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString SpecPos++; } - // Check for 's' specifier without field width - if (SpecPos < FormatString.size() && FormatString[SpecPos] == 's' && !HasFieldWidth) { - return true; + // Check for 's' specifier + if (SpecPos < FormatString.size() && FormatString[SpecPos] == 's') { + if (IsScanfFamily) { + // For scanf family, field width provides protection + if (!HasFieldWidth) { + return true; + } + } else { + // For sprintf family, only precision provides protection + if (!HasPrecision) { + return true; + } + } } Pos = SpecPos + 1; diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h index 4fd9e0d863f76..b61c7345a0484 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.h @@ -25,7 +25,7 @@ class UnsafeFormatStringCheck : public ClangTidyCheck { void check(const ast_matchers::MatchFinder::MatchResult &Result) override; private: - bool hasUnboundedStringSpecifier(StringRef FormatString); + bool hasUnboundedStringSpecifier(StringRef FormatString, bool IsScanfFamily); std::string getSafeAlternative(StringRef FunctionName); }; diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-format-string.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-format-string.rst index a7a6c6d2b71c0..9f7ee63fb57f9 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-format-string.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-format-string.rst @@ -7,10 +7,25 @@ Detects usage of vulnerable format string functions with unbounded ``%s`` specifiers that can cause buffer overflows. The check identifies calls to format string functions like ``sprintf``, ``scanf``, -and their variants that use ``%s`` format specifiers without field width limits. +and their variants that use ``%s`` format specifiers without proper limits. This can lead to buffer overflow vulnerabilities when the input string is longer than the destination buffer. +Format Specifier Behavior +-------------------------- + +The check distinguishes between different function families: + +**scanf family functions**: Field width limits input length + - ``%s`` - unsafe (no limit) + - ``%99s`` - safe (reads at most 99 characters) + +**sprintf family functions**: Precision limits output length + - ``%s`` - unsafe (no limit) + - ``%99s`` - unsafe (minimum width, no maximum) + - ``%.99s`` - safe (outputs at most 99 characters) + - ``%10.99s`` - safe (minimum 10 chars, maximum 99 chars) + Examples -------- @@ -19,13 +34,19 @@ Examples char buffer[100]; const char* input = "user input"; - // Unsafe: no field width limit - sprintf(buffer, "%s", input); - scanf("%s", buffer); + // Unsafe sprintf usage + sprintf(buffer, "%s", input); // No limit + sprintf(buffer, "%99s", input); // Field width is minimum, not maximum - // Safe: field width specified - sprintf(buffer, "%.99s", input); - scanf("%99s", buffer); + // Safe sprintf usage + sprintf(buffer, "%.99s", input); // Precision limits to 99 chars + sprintf(buffer, "%10.99s", input); // Min 10, max 99 chars + + // Unsafe scanf usage + scanf("%s", buffer); // No limit + + // Safe scanf usage + scanf("%99s", buffer); // Field width limits to 99 chars // Safe alternative: use safer functions snprintf(buffer, sizeof(buffer), "%s", input); @@ -35,7 +56,10 @@ Checked Functions The check detects unsafe format strings in these functions: +**sprintf family** (precision ``.N`` provides safety): * ``sprintf``, ``vsprintf`` + +**scanf family** (field width ``N`` provides safety): * ``scanf``, ``fscanf``, ``sscanf`` * ``vscanf``, ``vfscanf``, ``vsscanf`` * ``wscanf``, ``fwscanf``, ``swscanf`` @@ -44,6 +68,6 @@ The check detects unsafe format strings in these functions: Recommendations --------------- -* Use ``snprintf`` instead of ``sprintf`` to prevent buffer overflows -* Add field width specifiers to ``%s`` format specifiers (e.g., ``%99s``) +* For ``sprintf`` family: Use precision specifiers (``%.Ns``) or ``snprintf`` +* For ``scanf`` family: Use field width specifiers (``%Ns``) * Consider using safer string handling functions when possible diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c index 9dfcfb88cfaa0..4a1c98d8f1f69 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c @@ -10,10 +10,18 @@ void test_sprintf() { /* Positive: unsafe %s without field width */ sprintf(buffer, "%s", input); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] - /* Negative: safe %s with field width */ + /* Positive: field width doesn't prevent overflow in sprintf */ sprintf(buffer, "%99s", input); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] + + /* Negative: precision limits string length */ + sprintf(buffer, "%.99s", input); + /* no-warning */ + + /* Negative: precision with field width */ + sprintf(buffer, "%1.99s", input); /* no-warning */ /* Negative: other format specifiers are safe */ @@ -27,10 +35,14 @@ void test_vsprintf() { /* Positive: unsafe %s without field width */ vsprintf(buffer, "%s", args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] - /* Negative: safe %s with field width */ + /* Positive: field width doesn't prevent overflow in vsprintf */ vsprintf(buffer, "%99s", args); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] + + /* Negative: precision limits string length */ + vsprintf(buffer, "%.99s", args); /* no-warning */ } @@ -51,7 +63,7 @@ void test_scanf() { /* Positive: unsafe %s without field width */ scanf("%s", buffer); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] /* Negative: safe %s with field width */ scanf("%99s", buffer); @@ -64,7 +76,7 @@ void test_fscanf() { /* Positive: unsafe %s without field width */ fscanf(file, "%s", buffer); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] /* Negative: safe %s with field width */ fscanf(file, "%99s", buffer); @@ -77,7 +89,7 @@ void test_sscanf() { /* Positive: unsafe %s without field width */ sscanf(source, "%s", buffer); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] /* Negative: safe %s with field width */ sscanf(source, "%99s", buffer); @@ -90,7 +102,7 @@ void test_vfscanf() { /* Positive: unsafe %s without field width */ vfscanf(file, "%s", args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] /* Negative: safe %s with field width */ vfscanf(file, "%99s", args); @@ -103,7 +115,7 @@ void test_vsscanf() { /* Positive: unsafe %s without field width */ vsscanf(source, "%s", args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] /* Negative: safe %s with field width */ vsscanf(source, "%99s", args); @@ -115,7 +127,7 @@ void test_vscanf() { /* Positive: unsafe %s without field width */ vscanf("%s", args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] /* Negative: safe %s with field width */ vscanf("%99s", args); @@ -127,7 +139,7 @@ void test_wscanf() { /* Positive: unsafe %s without field width */ wscanf(L"%s", buffer); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] /* Negative: safe %s with field width */ wscanf(L"%99s", buffer); @@ -140,7 +152,7 @@ void test_fwscanf() { /* Positive: unsafe %s without field width */ fwscanf(file, L"%s", buffer); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] /* Negative: safe %s with field width */ fwscanf(file, L"%99s", buffer); @@ -153,7 +165,7 @@ void test_swscanf() { /* Positive: unsafe %s without field width */ swscanf(source, L"%s", buffer); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] /* Negative: safe %s with field width */ swscanf(source, L"%99s", buffer); @@ -165,7 +177,7 @@ void test_vwscanf() { /* Positive: unsafe %s without field width */ vwscanf(L"%s", args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] /* Negative: safe %s with field width */ vwscanf(L"%99s", args); @@ -178,7 +190,7 @@ void test_vfwscanf() { /* Positive: unsafe %s without field width */ vfwscanf(file, L"%s", args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] /* Negative: safe %s with field width */ vfwscanf(file, L"%99s", args); @@ -191,7 +203,7 @@ void test_vswscanf() { /* Positive: unsafe %s without field width */ vswscanf(source, L"%s", args); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] /* Negative: safe %s with field width */ vswscanf(source, L"%99s", args); From fc91d7492050b5688ddbf49ecf547d3304a29960 Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Fri, 31 Oct 2025 16:21:51 +0100 Subject: [PATCH 11/16] Adding test cases with dynamic field width --- .../checkers/bugprone/unsafe-format-string.c | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c index 4a1c98d8f1f69..a763420e228a1 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c @@ -16,6 +16,10 @@ void test_sprintf() { sprintf(buffer, "%99s", input); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] + /* Positive: dynamic field width doesn't prevent overflow */ + sprintf(buffer, "%*s", 10, input); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] + /* Negative: precision limits string length */ sprintf(buffer, "%.99s", input); /* no-warning */ @@ -24,6 +28,22 @@ void test_sprintf() { sprintf(buffer, "%1.99s", input); /* no-warning */ + /* Negative: dynamic precision */ + sprintf(buffer, "%.*s", 99, input); + /* no-warning */ + + /* Negative: field width with dynamic precision */ + sprintf(buffer, "%1.*s", 99, input); + /* no-warning */ + + /* Negative: dynamic field width with fixed precision */ + sprintf(buffer, "%*.99s", 10, input); + /* no-warning */ + + /* Negative: dynamic field width and precision */ + sprintf(buffer, "%*.*s", 10, 99, input); + /* no-warning */ + /* Negative: other format specifiers are safe */ sprintf(buffer, "%d %f", 42, 3.14); /* no-warning */ From 69c6a137746e7301ce461cf12af1a561f26a59b7 Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Fri, 31 Oct 2025 16:31:59 +0100 Subject: [PATCH 12/16] Marking those tests negatve which expect a warning --- .../checkers/bugprone/unsafe-format-string.c | 164 +++++++++--------- 1 file changed, 82 insertions(+), 82 deletions(-) diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c index a763420e228a1..b5e1613c08c45 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c @@ -7,44 +7,44 @@ void test_sprintf() { char buffer[100]; const char* input = "user input"; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ sprintf(buffer, "%s", input); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] - - /* Positive: field width doesn't prevent overflow in sprintf */ + + /* Negative: field width doesn't prevent overflow in sprintf */ sprintf(buffer, "%99s", input); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] - - /* Positive: dynamic field width doesn't prevent overflow */ + + /* Negative: dynamic field width doesn't prevent overflow */ sprintf(buffer, "%*s", 10, input); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] - - /* Negative: precision limits string length */ + + /* Positive: precision limits string length */ sprintf(buffer, "%.99s", input); /* no-warning */ - - /* Negative: precision with field width */ + + /* Positive: precision with field width */ sprintf(buffer, "%1.99s", input); /* no-warning */ - - /* Negative: dynamic precision */ + + /* Positive: dynamic precision */ sprintf(buffer, "%.*s", 99, input); /* no-warning */ - - /* Negative: field width with dynamic precision */ + + /* Positive: field width with dynamic precision */ sprintf(buffer, "%1.*s", 99, input); /* no-warning */ - - /* Negative: dynamic field width with fixed precision */ + + /* Positive: dynamic field width with fixed precision */ sprintf(buffer, "%*.99s", 10, input); /* no-warning */ - - /* Negative: dynamic field width and precision */ + + /* Positive: dynamic field width and precision */ sprintf(buffer, "%*.*s", 10, 99, input); /* no-warning */ - - /* Negative: other format specifiers are safe */ + + /* Positive: other format specifiers are safe */ sprintf(buffer, "%d %f", 42, 3.14); /* no-warning */ } @@ -52,15 +52,15 @@ void test_sprintf() { void test_vsprintf() { char buffer[100]; va_list args; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ vsprintf(buffer, "%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] - - /* Positive: field width doesn't prevent overflow in vsprintf */ + + /* Negative: field width doesn't prevent overflow in vsprintf */ vsprintf(buffer, "%99s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] - + /* Negative: precision limits string length */ vsprintf(buffer, "%.99s", args); /* no-warning */ @@ -70,22 +70,22 @@ void test_vsprintf_safe_wrapper(const char* format, ...) { char buffer[100]; va_list args; va_start(args, format); - - /* Negative: vsnprintf is safe */ + + /* Positive: vsnprintf is safe */ vsnprintf(buffer, sizeof(buffer), format, args); /* no-warning */ - + va_end(args); } void test_scanf() { char buffer[100]; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ scanf("%s", buffer); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - - /* Negative: safe %s with field width */ + + /* Positive: safe %s with field width */ scanf("%99s", buffer); /* no-warning */ } @@ -93,12 +93,12 @@ void test_scanf() { void test_fscanf() { char buffer[100]; FILE* file = 0; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ fscanf(file, "%s", buffer); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - - /* Negative: safe %s with field width */ + + /* Positive: safe %s with field width */ fscanf(file, "%99s", buffer); /* no-warning */ } @@ -106,12 +106,12 @@ void test_fscanf() { void test_sscanf() { char buffer[100]; const char* source = "input"; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ sscanf(source, "%s", buffer); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - - /* Negative: safe %s with field width */ + + /* Positive: safe %s with field width */ sscanf(source, "%99s", buffer); /* no-warning */ } @@ -119,12 +119,12 @@ void test_sscanf() { void test_vfscanf() { FILE* file = 0; va_list args; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ vfscanf(file, "%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - - /* Negative: safe %s with field width */ + + /* Positive: safe %s with field width */ vfscanf(file, "%99s", args); /* no-warning */ } @@ -132,36 +132,36 @@ void test_vfscanf() { void test_vsscanf() { const char* source = "input"; va_list args; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ vsscanf(source, "%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - - /* Negative: safe %s with field width */ + + /* Positive: safe %s with field width */ vsscanf(source, "%99s", args); /* no-warning */ } void test_vscanf() { va_list args; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ vscanf("%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - - /* Negative: safe %s with field width */ + + /* Positive: safe %s with field width */ vscanf("%99s", args); /* no-warning */ } void test_wscanf() { wchar_t buffer[100]; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ wscanf(L"%s", buffer); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - - /* Negative: safe %s with field width */ + + /* Positive: safe %s with field width */ wscanf(L"%99s", buffer); /* no-warning */ } @@ -169,12 +169,12 @@ void test_wscanf() { void test_fwscanf() { wchar_t buffer[100]; FILE* file = 0; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ fwscanf(file, L"%s", buffer); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - - /* Negative: safe %s with field width */ + + /* Positive: safe %s with field width */ fwscanf(file, L"%99s", buffer); /* no-warning */ } @@ -182,24 +182,24 @@ void test_fwscanf() { void test_swscanf() { wchar_t buffer[100]; const wchar_t* source = L"input"; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ swscanf(source, L"%s", buffer); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - - /* Negative: safe %s with field width */ + + /* Positive: safe %s with field width */ swscanf(source, L"%99s", buffer); /* no-warning */ } void test_vwscanf() { va_list args; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ vwscanf(L"%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - - /* Negative: safe %s with field width */ + + /* Positive: safe %s with field width */ vwscanf(L"%99s", args); /* no-warning */ } @@ -207,12 +207,12 @@ void test_vwscanf() { void test_vfwscanf() { FILE* file = 0; va_list args; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ vfwscanf(file, L"%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - - /* Negative: safe %s with field width */ + + /* Positive: safe %s with field width */ vfwscanf(file, L"%99s", args); /* no-warning */ } @@ -220,12 +220,12 @@ void test_vfwscanf() { void test_vswscanf() { const wchar_t* source = L"input"; va_list args; - - /* Positive: unsafe %s without field width */ + + /* Negative: unsafe %s without field width */ vswscanf(source, L"%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - - /* Negative: safe %s with field width */ + + /* Positive: safe %s with field width */ vswscanf(source, L"%99s", args); /* no-warning */ } @@ -233,16 +233,16 @@ void test_vswscanf() { void test_safe_alternatives() { char buffer[100]; const char* input = "user input"; - - /* Negative: snprintf is inherently safe */ + + /* Positive: snprintf is inherently safe */ snprintf(buffer, sizeof(buffer), "%s", input); /* no-warning */ - - /* Negative: printf family doesn't write to buffers */ + + /* Positive: printf family doesn't write to buffers */ printf("%s", input); /* no-warning */ - - /* Negative: fprintf doesn't write to user buffers */ + + /* Positive: fprintf doesn't write to user buffers */ fprintf(stderr, "%s", input); /* no-warning */ } From 75402507daf76e07e138fbf97e8defc9bdaa7d46 Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Fri, 7 Nov 2025 19:08:42 +0100 Subject: [PATCH 13/16] Changing back positive/negative test nomenclature. Fixing up some test cases --- .../checkers/bugprone/unsafe-format-string.c | 99 +++++++++---------- 1 file changed, 48 insertions(+), 51 deletions(-) diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c index b5e1613c08c45..c418bba478d2e 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c @@ -8,43 +8,43 @@ void test_sprintf() { char buffer[100]; const char* input = "user input"; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ sprintf(buffer, "%s", input); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] - /* Negative: field width doesn't prevent overflow in sprintf */ + /* Positive: field width doesn't prevent overflow in sprintf */ sprintf(buffer, "%99s", input); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] - /* Negative: dynamic field width doesn't prevent overflow */ + /* Positive: dynamic field width doesn't prevent overflow */ sprintf(buffer, "%*s", 10, input); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] - /* Positive: precision limits string length */ + /*Negative: precision limits string length */ sprintf(buffer, "%.99s", input); /* no-warning */ - /* Positive: precision with field width */ + /*Negative: precision with field width */ sprintf(buffer, "%1.99s", input); /* no-warning */ - /* Positive: dynamic precision */ + /*Negative: dynamic precision */ sprintf(buffer, "%.*s", 99, input); /* no-warning */ - /* Positive: field width with dynamic precision */ + /*Negative: field width with dynamic precision */ sprintf(buffer, "%1.*s", 99, input); /* no-warning */ - /* Positive: dynamic field width with fixed precision */ + /*Negative: dynamic field width with fixed precision */ sprintf(buffer, "%*.99s", 10, input); /* no-warning */ - /* Positive: dynamic field width and precision */ + /*Negative: dynamic field width and precision */ sprintf(buffer, "%*.*s", 10, 99, input); /* no-warning */ - /* Positive: other format specifiers are safe */ + /*Negative: other format specifiers are safe */ sprintf(buffer, "%d %f", 42, 3.14); /* no-warning */ } @@ -53,26 +53,26 @@ void test_vsprintf() { char buffer[100]; va_list args; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ vsprintf(buffer, "%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] - /* Negative: field width doesn't prevent overflow in vsprintf */ + /* Positive: field width doesn't prevent overflow in vsprintf */ vsprintf(buffer, "%99s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] - /* Negative: precision limits string length */ + /* Positive: precision limits string length */ vsprintf(buffer, "%.99s", args); /* no-warning */ } -void test_vsprintf_safe_wrapper(const char* format, ...) { - char buffer[100]; +void test_vsnprintf(int count, ...) { va_list args; - va_start(args, format); + va_start(args, count); + char buffer[100]; - /* Positive: vsnprintf is safe */ - vsnprintf(buffer, sizeof(buffer), format, args); + /*Negative: vsnprintf is safe */ + vsnprintf(buffer, sizeof(buffer), "%99s", args); /* no-warning */ va_end(args); @@ -81,11 +81,11 @@ void test_vsprintf_safe_wrapper(const char* format, ...) { void test_scanf() { char buffer[100]; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ scanf("%s", buffer); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - /* Positive: safe %s with field width */ + /*Negative: safe %s with field width */ scanf("%99s", buffer); /* no-warning */ } @@ -94,24 +94,23 @@ void test_fscanf() { char buffer[100]; FILE* file = 0; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ fscanf(file, "%s", buffer); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - /* Positive: safe %s with field width */ + /*Negative: safe %s with field width */ fscanf(file, "%99s", buffer); /* no-warning */ } -void test_sscanf() { +void test_sscanf(char *source) { char buffer[100]; - const char* source = "input"; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ sscanf(source, "%s", buffer); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - /* Positive: safe %s with field width */ + /*Negative: safe %s with field width */ sscanf(source, "%99s", buffer); /* no-warning */ } @@ -120,24 +119,23 @@ void test_vfscanf() { FILE* file = 0; va_list args; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ vfscanf(file, "%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - /* Positive: safe %s with field width */ + /*Negative: safe %s with field width */ vfscanf(file, "%99s", args); /* no-warning */ } -void test_vsscanf() { - const char* source = "input"; +void test_vsscanf(char * source) { va_list args; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ vsscanf(source, "%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - /* Positive: safe %s with field width */ + /*Negative: safe %s with field width */ vsscanf(source, "%99s", args); /* no-warning */ } @@ -145,11 +143,11 @@ void test_vsscanf() { void test_vscanf() { va_list args; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ vscanf("%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - /* Positive: safe %s with field width */ + /*Negative: safe %s with field width */ vscanf("%99s", args); /* no-warning */ } @@ -157,11 +155,11 @@ void test_vscanf() { void test_wscanf() { wchar_t buffer[100]; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ wscanf(L"%s", buffer); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - /* Positive: safe %s with field width */ + /*Negative: safe %s with field width */ wscanf(L"%99s", buffer); /* no-warning */ } @@ -170,24 +168,23 @@ void test_fwscanf() { wchar_t buffer[100]; FILE* file = 0; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ fwscanf(file, L"%s", buffer); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - /* Positive: safe %s with field width */ + /*Negative: safe %s with field width */ fwscanf(file, L"%99s", buffer); /* no-warning */ } -void test_swscanf() { +void test_swscanf(wchar_t *source) { wchar_t buffer[100]; - const wchar_t* source = L"input"; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ swscanf(source, L"%s", buffer); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - /* Positive: safe %s with field width */ + /*Negative: safe %s with field width */ swscanf(source, L"%99s", buffer); /* no-warning */ } @@ -195,11 +192,11 @@ void test_swscanf() { void test_vwscanf() { va_list args; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ vwscanf(L"%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - /* Positive: safe %s with field width */ + /*Negative: safe %s with field width */ vwscanf(L"%99s", args); /* no-warning */ } @@ -208,11 +205,11 @@ void test_vfwscanf() { FILE* file = 0; va_list args; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ vfwscanf(file, L"%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - /* Positive: safe %s with field width */ + /*Negative: safe %s with field width */ vfwscanf(file, L"%99s", args); /* no-warning */ } @@ -221,11 +218,11 @@ void test_vswscanf() { const wchar_t* source = L"input"; va_list args; - /* Negative: unsafe %s without field width */ + /* Positive: unsafe %s without field width */ vswscanf(source, L"%s", args); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without field width may cause buffer overflow; consider using '%Ns' where N limits input length [bugprone-unsafe-format-string] - /* Positive: safe %s with field width */ + /*Negative: safe %s with field width */ vswscanf(source, L"%99s", args); /* no-warning */ } @@ -234,15 +231,15 @@ void test_safe_alternatives() { char buffer[100]; const char* input = "user input"; - /* Positive: snprintf is inherently safe */ + /*Negative: snprintf is inherently safe */ snprintf(buffer, sizeof(buffer), "%s", input); /* no-warning */ - /* Positive: printf family doesn't write to buffers */ + /*Negative: printf family doesn't write to buffers */ printf("%s", input); /* no-warning */ - /* Positive: fprintf doesn't write to user buffers */ + /*Negative: fprintf doesn't write to user buffers */ fprintf(stderr, "%s", input); /* no-warning */ } From a6c6ad7ad447166522fb6f33518e94e0fd3586ec Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Fri, 7 Nov 2025 19:10:31 +0100 Subject: [PATCH 14/16] no need for the spec md file --- bugprone-format-string-specification.md | 62 ------------------------- 1 file changed, 62 deletions(-) delete mode 100644 bugprone-format-string-specification.md diff --git a/bugprone-format-string-specification.md b/bugprone-format-string-specification.md deleted file mode 100644 index 58e5dfcedb669..0000000000000 --- a/bugprone-format-string-specification.md +++ /dev/null @@ -1,62 +0,0 @@ -# Clang-Tidy Check Specification: `bugprone-unsafe-format-string` - -## Overview -Detects usage of vulnerable format string functions with unbounded `%s` specifiers that can cause buffer overflows. - -## Check Name -`bugprone-unsafe-format-string` - -## Description -Warns when format string functions without built-in buffer limits use `%s` specifier without field width restrictions. - -## Targeted Functions -```cpp -// Output functions -sprintf, vsprintf - -// Input functions -scanf, fscanf, sscanf, vscanf, vfscanf, vsscanf - -// Wide character functions -wscanf, fwscanf, swscanf, vwscanf, vfwscanf, vswscanf -``` - -## Detection Logic -1. Match calls to vulnerable functions -2. Extract format string (literal or traceable constant) -3. Parse format string for `%s` specifiers -4. Flag `%s` without field width (e.g., `%10s` is safe, `%s` is not) - -## Examples - -**Unsafe (triggers warning):** -```cpp -char buf[100]; -sprintf(buf, "Hello %s", name); // Warning -scanf("%s", buffer); // Warning -sscanf(input, "%s %d", str, &num); // Warning -``` - -**Safe (no warning):** -```cpp -char buf[100]; -snprintf(buf, sizeof(buf), "Hello %s", name); // Safe alternative -sprintf(buf, "Hello %.99s", name); // Field width specified -scanf("%99s", buffer); // Field width specified -``` - -## Diagnostic Message -``` -warning: format specifier '%s' without field width may cause buffer overflow [bugprone-unsafe-format-string] -``` - -## Fix-It Hints -- Suggest `snprintf()` for `sprintf()` -- Suggest field width for `%s` specifiers -- Provide buffer size calculation when possible - -## Implementation Notes -- Handle both string literals and const char* format parameters -- Support variadic and va_list variants -- Consider format string concatenation patterns -- Integrate with existing `bugprone` module From 77e2d97c5c4d37d2078a305ade5b3176c1a96076 Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Tue, 11 Nov 2025 23:02:43 +0100 Subject: [PATCH 15/16] Fixing review comments -Only matching global functions in c and std namespace in c++ -Adding C++ namespace tests -Removing fixit hints -Removing standard header includes from the test files and adding function/type definitions -Other small fixes --- .../bugprone/UnsafeFormatStringCheck.cpp | 76 ++++++++----------- .../checkers/bugprone/unsafe-format-string.c | 31 +++++++- .../bugprone/unsafe-format-string.cpp | 60 +++++++++++++++ 3 files changed, 119 insertions(+), 48 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp index f4263c034e423..cd8f6b85ee1e2 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFormatStringCheck.cpp @@ -8,9 +8,7 @@ #include "UnsafeFormatStringCheck.h" #include "clang/ASTMatchers/ASTMatchFinder.h" -#include "clang/Lex/Lexer.h" #include "llvm/Support/ConvertUTF.h" -#include "llvm/Support/raw_ostream.h" using namespace clang::ast_matchers; @@ -21,13 +19,16 @@ UnsafeFormatStringCheck::UnsafeFormatStringCheck(StringRef Name, : ClangTidyCheck(Name, Context) {} void UnsafeFormatStringCheck::registerMatchers(MatchFinder *Finder) { - // Match vulnerable format string functions - auto VulnerableFunctions = hasAnyName( - "sprintf", "vsprintf", "scanf", "fscanf", "sscanf", "vscanf", "vfscanf", - "vsscanf", "wscanf", "fwscanf", "swscanf", "vwscanf", "vfwscanf", "vswscanf"); - + // Matches sprintf and scanf family functions in std namespace in C++ and + // globally in C. + auto VulnerableFunctions = + hasAnyName("sprintf", "vsprintf", "scanf", "fscanf", "sscanf", "vscanf", + "vfscanf", "vsscanf", "wscanf", "fwscanf", "swscanf", + "vwscanf", "vfwscanf", "vswscanf"); Finder->addMatcher( - callExpr(callee(functionDecl(VulnerableFunctions)), + callExpr(callee(functionDecl(VulnerableFunctions, + anyOf(isInStdNamespace(), + hasParent(translationUnitDecl())))), anyOf(hasArgument(0, stringLiteral().bind("format")), hasArgument(1, stringLiteral().bind("format")))) .bind("call"), @@ -54,34 +55,29 @@ void UnsafeFormatStringCheck::check(const MatchFinder::MatchResult &Result) { const auto *Callee = cast(Call->getCalleeDecl()); StringRef FunctionName = Callee->getName(); - + bool IsScanfFamily = FunctionName.contains("scanf"); - + if (!hasUnboundedStringSpecifier(FormatString, IsScanfFamily)) return; auto Diag = diag(Call->getBeginLoc(), - IsScanfFamily + IsScanfFamily ? "format specifier '%%s' without field width may cause buffer overflow; consider using '%%Ns' where N limits input length" : "format specifier '%%s' without precision may cause buffer overflow; consider using '%%.Ns' where N limits output length") << Call->getSourceRange(); - - std::string SafeAlternative = getSafeAlternative(FunctionName); - if (!SafeAlternative.empty()) { - Diag << FixItHint::CreateInsertion(Call->getBeginLoc(), - "/* Consider using " + SafeAlternative + " */ "); - } } - -bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString, bool IsScanfFamily) { +bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef Fmt, + bool IsScanfFamily) { size_t Pos = 0; - while ((Pos = FormatString.find('%', Pos)) != StringRef::npos) { - if (Pos + 1 >= FormatString.size()) + size_t N = Fmt.size(); + while ((Pos = Fmt.find('%', Pos)) != StringRef::npos) { + if (Pos + 1 >= N) break; // Skip %% - if (FormatString[Pos + 1] == '%') { + if (Fmt[Pos + 1] == '%') { Pos += 2; continue; } @@ -89,20 +85,19 @@ bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString size_t SpecPos = Pos + 1; // Skip flags - while (SpecPos < FormatString.size() && - (FormatString[SpecPos] == '-' || FormatString[SpecPos] == '+' || - FormatString[SpecPos] == ' ' || FormatString[SpecPos] == '#' || - FormatString[SpecPos] == '0')) { + while (SpecPos < N && + (Fmt[SpecPos] == '-' || Fmt[SpecPos] == '+' || Fmt[SpecPos] == ' ' || + Fmt[SpecPos] == '#' || Fmt[SpecPos] == '0')) { SpecPos++; } // Check for field width bool HasFieldWidth = false; - if (SpecPos < FormatString.size() && FormatString[SpecPos] == '*') { + if (SpecPos < N && Fmt[SpecPos] == '*') { HasFieldWidth = true; SpecPos++; } else { - while (SpecPos < FormatString.size() && isdigit(FormatString[SpecPos])) { + while (SpecPos < N && isdigit(Fmt[SpecPos])) { HasFieldWidth = true; SpecPos++; } @@ -110,13 +105,13 @@ bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString // Check for precision bool HasPrecision = false; - if (SpecPos < FormatString.size() && FormatString[SpecPos] == '.') { + if (SpecPos < N && Fmt[SpecPos] == '.') { SpecPos++; - if (SpecPos < FormatString.size() && FormatString[SpecPos] == '*') { + if (SpecPos < N && Fmt[SpecPos] == '*') { HasPrecision = true; SpecPos++; } else { - while (SpecPos < FormatString.size() && isdigit(FormatString[SpecPos])) { + while (SpecPos < N && isdigit(Fmt[SpecPos])) { HasPrecision = true; SpecPos++; } @@ -124,15 +119,14 @@ bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString } // Skip length modifiers - while (SpecPos < FormatString.size() && - (FormatString[SpecPos] == 'h' || FormatString[SpecPos] == 'l' || - FormatString[SpecPos] == 'L' || FormatString[SpecPos] == 'z' || - FormatString[SpecPos] == 'j' || FormatString[SpecPos] == 't')) { + while (SpecPos < N && (Fmt[SpecPos] == 'h' || Fmt[SpecPos] == 'l' || + Fmt[SpecPos] == 'L' || Fmt[SpecPos] == 'z' || + Fmt[SpecPos] == 'j' || Fmt[SpecPos] == 't')) { SpecPos++; } // Check for 's' specifier - if (SpecPos < FormatString.size() && FormatString[SpecPos] == 's') { + if (SpecPos < N && Fmt[SpecPos] == 's') { if (IsScanfFamily) { // For scanf family, field width provides protection if (!HasFieldWidth) { @@ -152,14 +146,4 @@ bool UnsafeFormatStringCheck::hasUnboundedStringSpecifier(StringRef FormatString return false; } -std::string UnsafeFormatStringCheck::getSafeAlternative(StringRef FunctionName) { - if (FunctionName == "sprintf") - return "snprintf"; - if (FunctionName == "vsprintf") - return "vsnprintf"; - if (FunctionName.starts_with("scanf") || FunctionName.ends_with("scanf")) - return "add field width to %s specifiers"; - return ""; -} - } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c index c418bba478d2e..c44701f52f90a 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c @@ -1,9 +1,36 @@ // RUN: %check_clang_tidy %s bugprone-unsafe-format-string %t -#include -#include #include +typedef __SIZE_TYPE__ size_t; +typedef __WCHAR_TYPE__ wchar_t; +typedef void *FILE; +extern FILE *stdin; +extern FILE *stderr; + +extern int fscanf ( FILE * stream, const char * format, ... ); +extern int scanf ( const char * format, ... ); +extern int sscanf ( const char * s, const char * format, ...); +extern int vscanf( const char *restrict format, va_list vlist ); +extern int vfscanf ( FILE * stream, const char * format, va_list arg ); + +extern int vsscanf( const char *restrict buffer, const char *restrict format, va_list vlist ); +extern int vwscanf( const wchar_t* format, va_list vlist ); +extern int vfwscanf( FILE* stream, const wchar_t* format, va_list vlist ); +extern int vswscanf( const wchar_t* buffer, const wchar_t* format, va_list vlist ); +extern int swscanf (const wchar_t* ws, const wchar_t* format, ...); +extern int wscanf( const wchar_t *format, ... ); +extern int fwscanf( FILE *stream, const wchar_t *format, ... ); + +extern int printf( const char* format, ... ); +extern int sprintf( char* buffer, const char* format, ... ); +extern int vsprintf (char * s, const char * format, va_list arg ); +extern int vsnprintf (char * s, size_t n, const char * format, va_list arg ); +extern int fprintf( FILE* stream, const char* format, ... ); +extern int snprintf( char* restrict buffer, size_t bufsz, + const char* restrict format, ... ); + + void test_sprintf() { char buffer[100]; const char* input = "user input"; diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp new file mode 100644 index 0000000000000..19c54e6489da0 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp @@ -0,0 +1,60 @@ +// RUN: %check_clang_tidy %s bugprone-unsafe-format-string %t + +namespace std { + int sprintf( char* buffer, const char* format, ... ); +} + +class TestClass{ + int sprintf( char* buffer, const char* format, ... ); + void test_sprintf() { + char buffer[100]; + const char* input = "user input"; + /* Negative: no warning for calling member functions */ + sprintf(buffer, "%s", input); + } +}; + +void test_sprintf() { + char buffer[100]; + const char* input = "user input"; + + /* Positive: unsafe %s without field width */ + std::sprintf(buffer, "%s", input); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] + + /* Positive: field width doesn't prevent overflow in sprintf */ + std::sprintf(buffer, "%99s", input); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] + + /* Positive: dynamic field width doesn't prevent overflow */ + std::sprintf(buffer, "%*s", 10, input); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: format specifier '%s' without precision may cause buffer overflow; consider using '%.Ns' where N limits output length [bugprone-unsafe-format-string] + + /*Negative: precision limits string length */ + std::sprintf(buffer, "%.99s", input); + /* no-warning */ + + /*Negative: precision with field width */ + std::sprintf(buffer, "%1.99s", input); + /* no-warning */ + + /*Negative: dynamic precision */ + std::sprintf(buffer, "%.*s", 99, input); + /* no-warning */ + + /*Negative: field width with dynamic precision */ + std::sprintf(buffer, "%1.*s", 99, input); + /* no-warning */ + + /*Negative: dynamic field width with fixed precision */ + std::sprintf(buffer, "%*.99s", 10, input); + /* no-warning */ + + /*Negative: dynamic field width and precision */ + std::sprintf(buffer, "%*.*s", 10, 99, input); + /* no-warning */ + + /*Negative: other format specifiers are safe */ + std::sprintf(buffer, "%d %f", 42, 3.14); + /* no-warning */ +} From 795834aef5e02c12c9ee5a066a70800918ba76f1 Mon Sep 17 00:00:00 2001 From: Daniel Krupp Date: Wed, 19 Nov 2025 10:21:48 +0100 Subject: [PATCH 16/16] Add system header simulator to the tests --- .../system-header-simulator.h | 57 +++++++++++++++++++ .../checkers/bugprone/unsafe-format-string.c | 33 +---------- .../bugprone/unsafe-format-string.cpp | 6 +- 3 files changed, 61 insertions(+), 35 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unsafe-format-string/system-header-simulator.h diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unsafe-format-string/system-header-simulator.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unsafe-format-string/system-header-simulator.h new file mode 100644 index 0000000000000..fef2c10ae339a --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unsafe-format-string/system-header-simulator.h @@ -0,0 +1,57 @@ +#pragma clang system_header + +#ifdef __cplusplus +#define restrict /*restrict*/ +#endif + +#ifndef __cplusplus +typedef __WCHAR_TYPE__ wchar_t; +#endif + +typedef __typeof(sizeof(int)) size_t; +typedef long long __int64_t; +typedef __int64_t __darwin_off_t; +typedef __darwin_off_t fpos_t; +typedef int off_t; +typedef long ssize_t; + +typedef struct _FILE FILE; + +extern FILE *stdin; +extern FILE *stdout; +extern FILE *stderr; + +typedef __builtin_va_list va_list; +#define va_start(ap, param) __builtin_va_start(ap, param) +#define va_end(ap) __builtin_va_end(ap) +#define va_arg(ap, type) __builtin_va_arg(ap, type) +#define va_copy(dst, src) __builtin_va_copy(dst, src) + + +#ifdef __cplusplus +namespace std { +#endif +extern int fscanf ( FILE *restrict stream, const char *restrict format, ... ); +extern int scanf ( const char *restrict format, ... ); +extern int sscanf ( const char *restrict s, const char *restrict format, ...); +extern int vscanf( const char *restrict format, va_list vlist ); +extern int vfscanf ( FILE *restrict stream, const char *restrict format, va_list arg ); + +extern int vsscanf( const char *restrict buffer, const char *restrict format, va_list vlist ); +extern int vwscanf( const wchar_t* format, va_list vlist ); +extern int vfwscanf( FILE* stream, const wchar_t* format, va_list vlist ); +extern int vswscanf( const wchar_t* buffer, const wchar_t* format, va_list vlist ); +extern int swscanf (const wchar_t* ws, const wchar_t* format, ...); +extern int wscanf( const wchar_t *format, ... ); +extern int fwscanf( FILE *stream, const wchar_t *format, ... ); + +extern int printf( const char* format, ... ); +extern int sprintf( char* buffer, const char* format, ... ); +extern int vsprintf (char * s, const char * format, va_list arg ); +extern int vsnprintf (char * s, size_t n, const char * format, va_list arg ); +extern int fprintf( FILE* stream, const char* format, ... ); +extern int snprintf( char* restrict buffer, size_t bufsz, + const char* restrict format, ... ); +#ifdef __cplusplus +} //namespace std { +#endif \ No newline at end of file diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c index c44701f52f90a..2e3077ecab35a 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.c @@ -1,35 +1,6 @@ -// RUN: %check_clang_tidy %s bugprone-unsafe-format-string %t - -#include - -typedef __SIZE_TYPE__ size_t; -typedef __WCHAR_TYPE__ wchar_t; -typedef void *FILE; -extern FILE *stdin; -extern FILE *stderr; - -extern int fscanf ( FILE * stream, const char * format, ... ); -extern int scanf ( const char * format, ... ); -extern int sscanf ( const char * s, const char * format, ...); -extern int vscanf( const char *restrict format, va_list vlist ); -extern int vfscanf ( FILE * stream, const char * format, va_list arg ); - -extern int vsscanf( const char *restrict buffer, const char *restrict format, va_list vlist ); -extern int vwscanf( const wchar_t* format, va_list vlist ); -extern int vfwscanf( FILE* stream, const wchar_t* format, va_list vlist ); -extern int vswscanf( const wchar_t* buffer, const wchar_t* format, va_list vlist ); -extern int swscanf (const wchar_t* ws, const wchar_t* format, ...); -extern int wscanf( const wchar_t *format, ... ); -extern int fwscanf( FILE *stream, const wchar_t *format, ... ); - -extern int printf( const char* format, ... ); -extern int sprintf( char* buffer, const char* format, ... ); -extern int vsprintf (char * s, const char * format, va_list arg ); -extern int vsnprintf (char * s, size_t n, const char * format, va_list arg ); -extern int fprintf( FILE* stream, const char* format, ... ); -extern int snprintf( char* restrict buffer, size_t bufsz, - const char* restrict format, ... ); +// RUN: %check_clang_tidy %s bugprone-unsafe-format-string %t -- -- -isystem %S/Inputs/unsafe-format-string +#include void test_sprintf() { char buffer[100]; diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp index 19c54e6489da0..07117e37b7521 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-format-string.cpp @@ -1,8 +1,6 @@ -// RUN: %check_clang_tidy %s bugprone-unsafe-format-string %t +// RUN: %check_clang_tidy %s bugprone-unsafe-format-string %t -- -- -isystem %S/Inputs/unsafe-format-string -namespace std { - int sprintf( char* buffer, const char* format, ... ); -} +#include class TestClass{ int sprintf( char* buffer, const char* format, ... );