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