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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Oct 21 09:39:20 PDT 2021


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



More information about the cfe-commits mailing list