[PATCH] D110436: Add %n format specifier warning to clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/PercentNFormatSpecifierCheck.cpp:26-27
+  bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS,
+                             const char *startSpecifier,
+                             unsigned specifierLen) override {
+    const analyze_printf::PrintfConversionSpecifier &CS =
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/PercentNFormatSpecifierCheck.cpp:43-57
+  auto PrintfDecl = functionDecl(hasName("::printf"));
+  auto FprintfDecl = functionDecl(hasName("::fprintf"));
+  auto VfprintfDecl = functionDecl(hasName("::vfprintf"));
+  auto SprintfDecl = functionDecl(hasName("::sprintf"));
+  auto SnprintfDecl = functionDecl(hasName("::snprintf"));
+  auto VprintfDecl = functionDecl(hasName("::vprintf"));
+  auto VsprintfDecl = functionDecl(hasName("::vsprintf"));
----------------
Rather than separate all these into individual matchers, I think it's better to use `hasAnyName()`. e.g.,
```
  Finder->addMatcher(
      callExpr(callee(functionDecl(
                   hasAnyName("::printf", "::vprintf", "::scanf", "::vscanf"))),
               hasArgument(0, stringLiteral().bind("StringLiteral"))),
      this);
```
Also, it looks like this misses the `wchar_t` variants.

One additional design question is whether we should consider user-specified functions which use the `format` attribute in general. Using that attribute implies the function handles format specifier strings, so it seems like those functions would also want to flag %n for this particular check.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/PercentNFormatSpecifierCheck.cpp:89
+        Result.Context->getTargetInfo());
+    diag(loc, "usage of %%n can lead to unsafe writing to memory");
+  }
----------------
FWIW, this diagnostic sounds more scary than I think it should. This implies to me that tidy has found an unsafe usage when in fact, tidy is only identifying that you have used the feature at all.

Personally, I think it's more useful to limit the check to problematic situations. Use of `%n` by itself is not unsafe (in fact, I cannot think of a situation where use of `%n` in a *string literal* format specifier is ever a problem by itself. Generally, the safety concerns come from having a *non string literal* format specifier where an attacker can insert their own `%n`.

If you want this check to be "did you use `%n` at all", then I think the diagnostic should read more along the lines of `'%n' used as a format specifier` instead. However, I question whether "bugprone" is the right place for it at that point, because it's not really pointing out buggy code.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-percent-n-format-specifier.rst:7-8
+Finds any usage of the %n format specifier inside the format string
+of a call to printf, scanf, or any of their derivatives. The %n format
+specifier can lead to unsafe writing to memory.
+
----------------
Similar concerns about overselling the safety aspect of using `%n`.


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

https://reviews.llvm.org/D110436