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

[Jayson Yan via Phabricator via cfe-commits]

Jaysonyan added inline comments.


================
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"));
----------------
aaron.ballman wrote:
> 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.
Thanks! Will change to use `hasAnyName()` and will add the `wchar_t` variants.

Regarding the matching user-specified functions, I was interested in doing that but I'm struggling to find a way to match it. Do you have any suggestions? 


================
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");
+  }
----------------
aaron.ballman wrote:
> 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.
I think that's fair and changing the wording to just calling out the usage of the feature makes sense. The original motivation behind this change was because Fuchsia plans to disable usage of `%n` altogether. So we could possibly move this check to be under "fuchsia" rather than "bugprone".

That being said, I don't have full context behind the motivation to disable usage of `%n` but I believe that even explicit usage of the `%n` can be considered "bugprone" since it's difficult to guarantee that the pointer you are writing to comes from a reliable source.


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

https://reviews.llvm.org/D110436