[clang-tidy] Add check of sprintf with fixed size buffer

[Yoorkin]

Summary

Hi, I have added a clang-tidy check which proposed in this issue.
The check can finds sprintf usage like:

char buff[50];
sprintf(buff,"Hello, %s!\n","world");

And suggest counted version function instead:

warning: string to be written may exceeds the size of buffer; use snprintf instead [bugprone-sprintf-with-fixed-size-buffer]
  sprintf(buff,"Hello, %s!\n","world");
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  snprintf(buff, sizeof(buff), "Hello, %s!\n", "world")

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/bugprone/SprintfWithFixedSizeBufferCheck.cpp

14

Unnecessary newline.

35

const auto * could be used because type is spelled in same statement.

36

Ditto.

clang-tools-extra/clang-tidy/bugprone/SprintfWithFixedSizeBufferCheck.h

30

Please add isLanguageVersionSupported (C and C++).

clang-tools-extra/docs/ReleaseNotes.rst

105

Please use first statement in documentation.

clang-tools-extra/docs/clang-tidy/checks/bugprone/sprintf-with-fixed-size-buffer.rst

7

Please omit The check and use double back-ticks for sprintf (language construct).

8

Ditto for snprintf.

15

Please separate with newline.

22

Ditto.

40

Please fix.

[Yoorkin]

clang-tools-extra/clang-tidy/bugprone/SprintfWithFixedSizeBufferCheck.h

30

can i just simply use

bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
  return LangOpts.C99 || LangOpts.CPlusPlus11;
}

I think the check should be supported after C99 and Cpp11 standards. But i don't know whether it return true when language version is c++20 or C11

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/bugprone/SprintfWithFixedSizeBufferCheck.h

30

snprintf is part of standard C library, so it could be used in early C and C++ standards.

[Yoorkin]

I fixed them all

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/bugprone/sprintf-with-fixed-size-buffer.rst

7

Should be Finds. Same in Release Notes.

[Yoorkin]

I fixed it. Thanks

[njames93]

Why are we only targeting sprintf, vsprintf would also suffer from the same pitfalls.
Is there any hard reason that this check always suggests snprintf instead of sprintf_s, maybe have that dynamically controlled with an option.

clang-tools-extra/clang-tidy/bugprone/SprintfWithFixedSizeBufferCheck.cpp

37

Prefer small little edits instead of one big replacement.
So create a fix it that turns sprintf into snprintf, then create a fix it that inserts , sizeof(Buff) after the first argument.

[Yoorkin]

Now it can find usage of both sprintf and vsprintf, and then change a small piece of code. Here is an option PreferSafe, when value is true, the check prefers snprintf_s vsnprintf_s over snprintf vsnprintf.

[Yoorkin]

I forgot to update comments in PrintfWithFixedSizeBufferCheck.h, sorry

[njames93]

Should this check be renamed to bugprone-sprintf-with-fixed-size-buffer?
Have you thought about an option to flag all calls to sprintf as they are typically not recommended by a lot of coding practice guides. Obviously we couldn't generate a fix-it as the buffer size may be unknown, but it could help fix someone's buffer overflow exploit.

clang-tools-extra/clang-tidy/bugprone/PrintfWithFixedSizeBufferCheck.cpp

23 ↗

(On Diff #454724)

29 ↗

(On Diff #454724)

Check out the mapAnyWith matcher, would simply a lot of this code.

33 ↗

(On Diff #454724)

Just use hasAnyName("::sprintf", "::vsprintf"), Then in the check you can call getName to find out which one.

58–70 ↗

(On Diff #454724)

Avoid string manipulation where possible.

clang-tools-extra/clang-tidy/bugprone/PrintfWithFixedSizeBufferCheck.h

27 ↗

(On Diff #454724)

Let template deduction handle this.

30 ↗

(On Diff #454724)

This check seems like it would have value in C code as well as C++.

[Yoorkin]

Maybe renamed to bugprone-sprintf-with-fixed-size-buffer would be better. I add another option FlagAllCalls to flag all calls to sprintf and vsprintf.

[Yoorkin]

I want to enable this check in both c and c++, but didn't find option LangOpt.C. For c++, we have option LangOpt.CPlusPlus. There is only some specific c standard option like C99 C11 C17 and C2X defined in LangOptions.def. Could you tell me how to do this?

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "PosixReturnCheck.h"
 #include "RedundantBranchConditionCheck.h"
 #include "ReservedIdentifierCheck.h"
 #include "SharedPtrArrayMismatchCheck.h"
 #include "SignalHandlerCheck.h"
 #include "SignedCharMisuseCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
+#include "SprintfWithFixedSizeBufferCheck.h"
 #include "SpuriouslyWakeUpFunctionsCheck.h"
 #include "StringConstructorCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "StringviewNullptrCheck.h"
 #include "SuspiciousEnumUsageCheck.h"
 #include "SuspiciousIncludeCheck.h"
 #include "SuspiciousMemoryComparisonCheck.h"
CheckFactories.registerCheck<SharedPtrArrayMismatchCheck>(
         "bugprone-shared-ptr-array-mismatch");
     CheckFactories.registerCheck<SignalHandlerCheck>("bugprone-signal-handler");
     CheckFactories.registerCheck<SignedCharMisuseCheck>(
         "bugprone-signed-char-misuse");
     CheckFactories.registerCheck<SizeofContainerCheck>(
         "bugprone-sizeof-container");
     CheckFactories.registerCheck<SizeofExpressionCheck>(
         "bugprone-sizeof-expression");
+    CheckFactories.registerCheck<SprintfWithFixedSizeBufferCheck>(
+        "bugprone-sprintf-with-fixed-size-buffer");
     CheckFactories.registerCheck<SpuriouslyWakeUpFunctionsCheck>(
         "bugprone-spuriously-wake-up-functions");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "bugprone-string-constructor");
     CheckFactories.registerCheck<StringIntegerAssignmentCheck>(
         "bugprone-string-integer-assignment");
     CheckFactories.registerCheck<StringLiteralWithEmbeddedNulCheck>(
         "bugprone-string-literal-with-embedded-nul");

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   RedundantBranchConditionCheck.cpp
   ReservedIdentifierCheck.cpp
   SharedPtrArrayMismatchCheck.cpp
   SmartPtrArrayMismatchCheck.cpp
   SignalHandlerCheck.cpp
   SignedCharMisuseCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp
+  SprintfWithFixedSizeBufferCheck.cpp
   SpuriouslyWakeUpFunctionsCheck.cpp
   StringConstructorCheck.cpp
   StringIntegerAssignmentCheck.cpp
   StringLiteralWithEmbeddedNulCheck.cpp
   StringviewNullptrCheck.cpp
   SuspiciousEnumUsageCheck.cpp
   SuspiciousIncludeCheck.cpp
   SuspiciousMemoryComparisonCheck.cpp

clang-tools-extra/clang-tidy/bugprone/SprintfWithFixedSizeBufferCheck.h

+//===--- SprintfWithFixedSizeBufferCheck.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_SPRINTFWITHFIXEDSIZEBUFFERCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SPRINTFWITHFIXEDSIZEBUFFERCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Check if `sprintf` or `vsprintf` is called with a fixed size buffer.
+/// Replaced with counted versions function.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/sprintf-with-fixed-size-buffer.html
+class SprintfWithFixedSizeBufferCheck : public ClangTidyCheck {
+public:
+  SprintfWithFixedSizeBufferCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        PreferSafe(Options.get("PreferSafe", false)),
+        FlagAllCalls(Options.get("FlagAllCalls", false)) {}
+
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+  /// Registering for sprintf and vsprintf calls.
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  bool PreferSafe;
+  bool FlagAllCalls;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SPRINTFWITHFIXEDSIZEBUFFERCHECK_H

clang-tools-extra/clang-tidy/bugprone/SprintfWithFixedSizeBufferCheck.cpp

+//===--- SprintfWithFixedSizeBufferCheck.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 "SprintfWithFixedSizeBufferCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/StringRef.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void SprintfWithFixedSizeBufferCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "PreferSafe", PreferSafe);
+  Options.store(Opts, "FlagAllCalls", FlagAllCalls);
+}
+
+void SprintfWithFixedSizeBufferCheck::registerMatchers(MatchFinder *Finder) {
+  auto SprintfCallee =
+      callee(functionDecl(hasAnyName("::sprintf", "::vsprintf")));
+
+  auto HasFixedSizeBuffer =
+      hasArgument(0, mapAnyOf(memberExpr, declRefExpr)
+                         .with(hasType(constantArrayType()))
+                         .bind("FixedSizeArray"));
+
+  Finder->addMatcher(
+      callExpr(allOf(HasFixedSizeBuffer, SprintfCallee)).bind("Call"), this);
+  if (FlagAllCalls) {
+    Finder->addMatcher(
+        callExpr(allOf(unless(HasFixedSizeBuffer), SprintfCallee)).bind("Call"),
+        this);
+  }
+}
+
+void SprintfWithFixedSizeBufferCheck::check(
+    const MatchFinder::MatchResult &Result) {
+
+  const auto *FA = Result.Nodes.getNodeAs<Expr>("FixedSizeArray");
+  const auto *Call = Result.Nodes.getNodeAs<CallExpr>("Call");
+  const auto *Callee = dyn_cast<FunctionDecl>(Call->getCalleeDecl());
+
+  StringRef Recommendation;
+  if (Callee->getName() == "sprintf")
+    Recommendation = "snprintf_s";
+  else if (Callee->getName() == "vsprintf")
+    Recommendation = "vsnprintf_s";
+
+  if (!PreferSafe) {
+    Recommendation = Recommendation.drop_back(2);
+  }
+
+  auto D =
+      diag(Call->getBeginLoc(), "string to be written may exceeds the size of "
+                                "buffer; use %0 instead")
+      << Recommendation << Call->getCallee()->getSourceRange();
+
+  // Generate a fix-it if detected fixed size buffer
+  if (FA != nullptr) {
+    const Expr *SecondArgument = Call->getArg(1);
+    ;
+    // Insert the size of buffer after first argument.
+    std::string SizeofText = "sizeof(";
+    SizeofText += Lexer::getSourceText(
+        CharSourceRange::getTokenRange(FA->getSourceRange()),
+        *Result.SourceManager, getLangOpts());
+    SizeofText += "), ";
+
+    FixItHint ReplacementHint = FixItHint::CreateReplacement(
+        Call->getCallee()->getSourceRange(), Recommendation);
+    FixItHint SizeofHint =
+        FixItHint::CreateInsertion(SecondArgument->getBeginLoc(), SizeofText);
+    D << SecondArgument->getSourceRange() << ReplacementHint << SizeofHint;
+  }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 New checks
 ^^^^^^^^^^
 
+- New :doc:`bugprone-sprintf-with-fixed-size-buffer
+  <clang-tidy/checks/bugprone/sprintf-with-fixed-size-buffer>` check.
+
+  Finds usage of ``sprintf`` or ``vsprintf``, which write output string into a fixed-size array. 
+
 - New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
   <clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members>` check.
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
 New check aliases
 ^^^^^^^^^^^^^^^^^
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/sprintf-with-fixed-size-buffer.rst

+.. title:: clang-tidy - bugprone-sprintf-with-fixed-size-buffer
+
+bugprone-sprintf-with-fixed-size-buffer
+=======================================
+
+Finds usage of ``sprintf`` or ``vsprintf``, which write output string into a fixed-size array. 
+It will suggest the counted versions instead.
+
+It's a common idiom to have a fixed-size buffer of characters allocated on 
+the stack and then to printf into the buffer. It may be leading to errors if the 
+string to be written exceeds the size of the array pointed to by buffer.
+
+Example:
+.. code-block:: c++
+
+    void f(){
+      char buff[80];
+      sprintf(buff, "Hello, %s!\n", "world");
+    }
+
+Becomes:
+.. code-block:: c++
+
+    void f(){
+      char buff[80];
+      snprintf(buff, sizeof(buff), "Hello, %s!\n", "world");
+    }
+
+Options
+-------
+
+.. option:: PreferSafe
+
+  When `true`, prefer to use ``snprintf_s`` ``vsnprintf_s`` over ``snprintf`` ``vsnprintf``.
+  Default is `false`.
+
+.. option:: FlagAllCalls
+    Flag all calls to ``sprintf`` and ``vsprintf``.
+    Default is `false`

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `bugprone-posix-return <bugprone/posix-return.html>`_, "Yes"
    `bugprone-redundant-branch-condition <bugprone/redundant-branch-condition.html>`_, "Yes"
    `bugprone-reserved-identifier <bugprone/reserved-identifier.html>`_, "Yes"
    `bugprone-shared-ptr-array-mismatch <bugprone/shared-ptr-array-mismatch.html>`_, "Yes"
    `bugprone-signal-handler <bugprone/signal-handler.html>`_,
    `bugprone-signed-char-misuse <bugprone/signed-char-misuse.html>`_,
    `bugprone-sizeof-container <bugprone/sizeof-container.html>`_,
    `bugprone-sizeof-expression <bugprone/sizeof-expression.html>`_,
+   `bugprone-sprintf-with-fixed-size-buffer <bugprone/sprintf-with-fixed-size-buffer.html>`_, "Yes"
    `bugprone-spuriously-wake-up-functions <bugprone/spuriously-wake-up-functions.html>`_,
    `bugprone-string-constructor <bugprone/string-constructor.html>`_, "Yes"
    `bugprone-string-integer-assignment <bugprone/string-integer-assignment.html>`_, "Yes"
    `bugprone-string-literal-with-embedded-nul <bugprone/string-literal-with-embedded-nul.html>`_,
    `bugprone-stringview-nullptr <bugprone/stringview-nullptr.html>`_, "Yes"
    `bugprone-suspicious-enum-usage <bugprone/suspicious-enum-usage.html>`_,
    `bugprone-suspicious-include <bugprone/suspicious-include.html>`_,
    `bugprone-suspicious-memory-comparison <bugprone/suspicious-memory-comparison.html>`_,

clang-tools-extra/test/clang-tidy/checkers/bugprone/sprintf-with-fixed-size-buffer.cpp

+// RUN: %check_clang_tidy %s bugprone-sprintf-with-fixed-size-buffer %t
+
+#include <stdio.h>
+
+void f(){
+  char buff[80];
+  sprintf(buff, "Hello, %s!\n", "world");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: string to be written may exceeds the size of buffer; use snprintf instead [bugprone-printf-with-fixed-size-buffer]
+  // CHECK-FIXES: snprintf(buff, sizeof(buff), "Hello, %s!\n", "world");
+}
+
+char sbuff[80];
+
+void f2(){
+  sprintf(sbuff, "Hello, %s!\n", "world");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: string to be written may exceeds the size of buffer; use snprintf instead [bugprone-printf-with-fixed-size-buffer]
+  // CHECK-FIXES: snprintf(sbuff, sizeof(sbuff), "Hello, %s!\n", "world");
+}
+
+void f3(){
+    char *dynamic_sized_buff = nullptr;
+    sprintf(dynamic_sized_buff, "Hello, %s!\n", "world");
+}
+
+void f4(const char * format, ... ){
+  char buff[100];
+  va_list args;
+  va_start(args, format);
+  vsprintf(buff, format, args);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: string to be written may exceeds the size of buffer; use vsnprintf instead [bugprone-printf-with-fixed-size-buffer]
+  // CHECK-FIXES: vsnprintf(buff, sizeof(buff), format, args);
+  va_end(args);
+}