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