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

[Jayson Yan via Phabricator via cfe-commits]

Jaysonyan added inline comments.
Herald added a subscriber: carlosgalvezp.


================
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:
> 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);
> ```
Ok that seems reasonable, I'll move this check to fuchsia then.


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

https://reviews.llvm.org/D110436