[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

13 ↗

(On Diff #454202)

Unnecessary newline.

34 ↗

(On Diff #454202)

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

35 ↗

(On Diff #454202)

Ditto.

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

29 ↗

(On Diff #454202)

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

6 ↗

(On Diff #454202)

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

7 ↗

(On Diff #454202)

Ditto for snprintf.

14 ↗

(On Diff #454202)

Please separate with newline.

21 ↗

(On Diff #454202)

Ditto.

26 ↗

(On Diff #454202)

Please fix.

[Yoorkin]

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

29 ↗

(On Diff #454202)

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

29 ↗

(On Diff #454202)

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

6 ↗

(On Diff #454226)

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

36 ↗

(On Diff #454228)

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

24

30

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

34

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

59–71

Avoid string manipulation where possible.

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

28

Let template deduction handle this.

31

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 "MisplacedPointerArithmeticInAllocCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveForwardingReferenceCheck.h"
 #include "MultipleStatementMacroCheck.h"
 #include "NoEscapeCheck.h"
 #include "NotNullTerminatedResultCheck.h"
 #include "ParentVirtualCallCheck.h"
 #include "PosixReturnCheck.h"
+#include "PrintfWithFixedSizeBufferCheck.h"
 #include "RedundantBranchConditionCheck.h"
 #include "ReservedIdentifierCheck.h"
 #include "SharedPtrArrayMismatchCheck.h"
 #include "SignalHandlerCheck.h"
 #include "SignedCharMisuseCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "SpuriouslyWakeUpFunctionsCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MisplacedPointerArithmeticInAllocCheck>(
         "bugprone-misplaced-pointer-arithmetic-in-alloc");
     CheckFactories.registerCheck<MisplacedWideningCastCheck>(
         "bugprone-misplaced-widening-cast");
     CheckFactories.registerCheck<MoveForwardingReferenceCheck>(
         "bugprone-move-forwarding-reference");
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
+    CheckFactories.registerCheck<PrintfWithFixedSizeBufferCheck>(
+        "bugprone-printf-with-fixed-size-buffer");
     CheckFactories.registerCheck<RedundantBranchConditionCheck>(
         "bugprone-redundant-branch-condition");
     CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
         "bugprone-narrowing-conversions");
     CheckFactories.registerCheck<NoEscapeCheck>("bugprone-no-escape");
     CheckFactories.registerCheck<NotNullTerminatedResultCheck>(
         "bugprone-not-null-terminated-result");
     CheckFactories.registerCheck<ParentVirtualCallCheck>(

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

add_clang_library(clangTidyBugproneModule
   MisplacedPointerArithmeticInAllocCheck.cpp
   MisplacedWideningCastCheck.cpp
   MoveForwardingReferenceCheck.cpp
   MultipleStatementMacroCheck.cpp
   NoEscapeCheck.cpp
   NotNullTerminatedResultCheck.cpp
   ParentVirtualCallCheck.cpp
   PosixReturnCheck.cpp
+  PrintfWithFixedSizeBufferCheck.cpp
   RedundantBranchConditionCheck.cpp
   ReservedIdentifierCheck.cpp
   SharedPtrArrayMismatchCheck.cpp
   SmartPtrArrayMismatchCheck.cpp
   SignalHandlerCheck.cpp
   SignedCharMisuseCheck.cpp
   SizeofContainerCheck.cpp
   SizeofExpressionCheck.cpp

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

+//===--- PrintfWithFixedSizeBufferCheck.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_PRINTFWITHFIXEDSIZEBUFFERCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PRINTFWITHFIXEDSIZEBUFFERCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Check if `sprintf` is called with a fixed size buffer. Replaced with counted
+/// version `snprintf`.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/sprintf-with-fixed-size-buffer.html
+class PrintfWithFixedSizeBufferCheck : public ClangTidyCheck {
+public:
+  PrintfWithFixedSizeBufferCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context),
+        PreferSafe(Options.get<bool>("PreferSafe", false)) {}
+
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  
+  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;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PRINTFWITHFIXEDSIZEBUFFERCHECK_H

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

+//===--- PrintfWithFixedSizeBufferCheck.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 "PrintfWithFixedSizeBufferCheck.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 PrintfWithFixedSizeBufferCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store<bool>(Opts, "PreferSafe", PreferSafe);
+}
+
+void PrintfWithFixedSizeBufferCheck::registerMatchers(MatchFinder *Finder) {
+  auto HasConstantArrayType = hasType(constantArrayType());
+  auto HasFixedSizeBufferMatcher =
+      hasArgument(0, anyOf(memberExpr(HasConstantArrayType).bind("MemberExpr"),
+                           declRefExpr(HasConstantArrayType).bind("DeclRef")));
+
+  Finder->addMatcher(callExpr(allOf(HasFixedSizeBufferMatcher,
+                                    callee(functionDecl(hasName("::sprintf")))))
+                         .bind("sprintf"),
+                     this);
+
+  Finder->addMatcher(
+      callExpr(allOf(HasFixedSizeBufferMatcher,
+                     callee(functionDecl(hasName("::vsprintf")))))
+          .bind("vsprintf"),
+      this);
+}
+
+void PrintfWithFixedSizeBufferCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const Expr *CA;
+  const auto *ME = Result.Nodes.getNodeAs<MemberExpr>("MemberExpr");
+  const auto *DR = Result.Nodes.getNodeAs<DeclRefExpr>("DeclRef");
+
+  if (ME != nullptr)
+    CA = ME;
+  else
+    CA = DR;
+
+  const CallExpr *Call = nullptr;
+  const Expr *SecondArgument = nullptr;
+
+  std::string Recommendation;
+  if (Call = Result.Nodes.getNodeAs<CallExpr>("sprintf"))
+    Recommendation = "snprintf";
+  else if (Call = Result.Nodes.getNodeAs<CallExpr>("vsprintf"))
+    Recommendation = "vsnprintf";
+
+  assert(Call && "Unhandled binding in the Matcher");
+
+  SecondArgument = Call->getArg(1);
+
+  if (PreferSafe) {
+    Recommendation += "_s";
+  }
+
+  // Insert the size of buffer after first argument.
+  std::string SizeofText = "sizeof(";
+  SizeofText +=
+      Lexer::getSourceText(CharSourceRange::getTokenRange(CA->getSourceRange()),
+                           *Result.SourceManager, getLangOpts());
+  SizeofText += "), ";
+
+  FixItHint ReplacementHint = FixItHint::CreateReplacement(
+      Call->getCallee()->getSourceRange(), Recommendation);
+  FixItHint SizeofHint =
+      FixItHint::CreateInsertion(SecondArgument->getBeginLoc(), SizeofText);
+  auto D =
+      diag(Call->getBeginLoc(), "string to be written may exceeds the size of "
+                                "buffer; use %0 instead")
+      << Recommendation << Call->getCallee()->getSourceRange()
+      << 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-printf-with-fixed-size-buffer
+  <clang-tidy/checks/bugprone/printf-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/printf-with-fixed-size-buffer.rst

+.. title:: clang-tidy - bugprone-printf-with-fixed-size-buffer
+
+
+bugprone-printf-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`.

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

.. csv-table::
    `bugprone-misplaced-pointer-arithmetic-in-alloc <bugprone/misplaced-pointer-arithmetic-in-alloc.html>`_, "Yes"
    `bugprone-misplaced-widening-cast <bugprone/misplaced-widening-cast.html>`_,
    `bugprone-move-forwarding-reference <bugprone/move-forwarding-reference.html>`_, "Yes"
    `bugprone-multiple-statement-macro <bugprone/multiple-statement-macro.html>`_,
    `bugprone-no-escape <bugprone/no-escape.html>`_,
    `bugprone-not-null-terminated-result <bugprone/not-null-terminated-result.html>`_, "Yes"
    `bugprone-parent-virtual-call <bugprone/parent-virtual-call.html>`_, "Yes"
    `bugprone-posix-return <bugprone/posix-return.html>`_, "Yes"
+   `bugprone-printf-with-fixed-size-buffer <bugprone/printf-with-fixed-size-buffer.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-spuriously-wake-up-functions <bugprone/spuriously-wake-up-functions.html>`_,

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

+// RUN: %check_clang_tidy %s bugprone-printf-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);
+}