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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
