aaron.ballman 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"));
----------------
Jaysonyan wrote:
> 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?
> 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?
I think something like `functionDecl(hasAttr(clang::attr::Format))` should get
you close -- that'll match the function declaration with the attribute, and
from there you can look at the attribute string in the `check()` function.
================
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");
+ }
----------------
Jaysonyan wrote:
> 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.
> So we could possibly move this check to be under "fuchsia" rather than
> "bugprone".
That would make me feel more comfortable.
> 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.
I disagree that this is a bugprone pattern; that's like suggesting that use of
`%s` is bugprone because you can't guarantee that the pointer being read from
comes from a reliable source. The programmer specifies the pointer in both
cases. There is absolutely nothing bugprone about:
```
int n = 0;
printf("%s, %s%n", "hello", "world", &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
