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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

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.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/PrintfWithFixedSizeBufferCheck.cpp:23
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store<bool>(Opts, "PreferSafe", PreferSafe);
+}
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/PrintfWithFixedSizeBufferCheck.cpp:29
+  auto HasFixedSizeBufferMatcher =
+      hasArgument(0, anyOf(memberExpr(HasConstantArrayType).bind("MemberExpr"),
+                           declRefExpr(HasConstantArrayType).bind("DeclRef")));
----------------
Check out the `mapAnyWith` matcher, would simply a lot of this code.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/PrintfWithFixedSizeBufferCheck.cpp:33
+  Finder->addMatcher(callExpr(allOf(HasFixedSizeBufferMatcher,
+                                    callee(functionDecl(hasName("::sprintf")))))
+                         .bind("sprintf"),
----------------
Just use `hasAnyName("::sprintf", "::vsprintf")`, Then in the check you can call `getName` to find out which one.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/PrintfWithFixedSizeBufferCheck.cpp:58-70
+  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");
----------------
Avoid string manipulation where possible.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/PrintfWithFixedSizeBufferCheck.h:27
+      : ClangTidyCheck(Name, Context),
+        PreferSafe(Options.get<bool>("PreferSafe", false)) {}
+
----------------
Let template deduction handle this.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/PrintfWithFixedSizeBufferCheck.h:30
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
----------------
This check seems like it would have value in C code as well as C++.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132294/new/

https://reviews.llvm.org/D132294