[PATCH] D110436: [WIP] Add %n format specifier warning
Jaysonyan created this revision.
Jaysonyan added reviewers: leonardchan, phosek.
Jaysonyan requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This warning is to dissuade developers from using the potentially unsafe %n
format specifier. Currently this warning is surfaced under the flag
-Wformat-n-specifier
which is enabled under the group -Wformat. The way this information is surfaced
is pending
discussion from this RFC:
https://lists.llvm.org/pipermail/cfe-dev/2021-September/068986.html.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D110436
Files:
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaChecking.cpp
clang/test/FixIt/format.m
clang/test/Misc/warning-wall.c
clang/test/Sema/format-string-percentn.c
clang/test/Sema/format-strings-size_t.c
clang/test/Sema/format-strings.c
Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs -fno-signed-char %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-format-n-specifier -Wformat-nonliteral -isystem %S/Inputs %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-format-n-specifier -Wformat-nonliteral -isystem %S/Inputs -fno-signed-char %s
#include
#include
@@ -644,6 +644,7 @@
test14_bar("%", "%d", p); // expected-warning{{incomplete format specifier}}
}
+#pragma GCC diagnostic ignored "-Wformat-n-specifier"
void test_qualifiers(volatile int *vip, const int *cip,
const volatile int *cvip) {
printf("%n", cip); // expected-warning{{format specifies type 'int *' but the argument has type 'const int *'}}
Index: clang/test/Sema/format-strings-size_t.c
===
--- clang/test/Sema/format-strings-size_t.c
+++ clang/test/Sema/format-strings-size_t.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -Wno-format-n-specifier -fsyntax-only -verify %s
int printf(char const *, ...);
Index: clang/test/Sema/format-string-percentn.c
===
--- /dev/null
+++ clang/test/Sema/format-string-percentn.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int printf(const char *restrict, ...);
+void percentn(volatile int *n) {
+ printf("%n", n); //expected-warning{{usage of '%n' is unsafe}}
+}
Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -17,6 +17,7 @@
CHECK-NEXT: -Wformat-y2k
CHECK-NEXT: -Wformat-invalid-specifier
CHECK-NEXT: -Wformat-insufficient-args
+CHECK-NEXT: -Wformat-n-specifier
CHECK-NEXT:-Wfor-loop-analysis
CHECK-NEXT:-Wframe-address
CHECK-NEXT:-Wimplicit
Index: clang/test/FixIt/format.m
===
--- clang/test/FixIt/format.m
+++ clang/test/FixIt/format.m
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple %itanium_abi_triple -fsyntax-only -fblocks -verify %s
-// RUN: %clang_cc1 -triple %itanium_abi_triple -fdiagnostics-parseable-fixits -fblocks %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -Wno-format-n-specifier -fsyntax-only -fblocks -verify %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -Wno-format-n-specifier -fdiagnostics-parseable-fixits -fblocks %s 2>&1 | FileCheck %s
@class NSString;
extern void NSLog(NSString *, ...);
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -8965,6 +8965,13 @@
return true;
}
+ // %n is unsafe
+ if (CS.getKind() == ConversionSpecifier::nArg) {
+EmitFormatDiagnostic(
+ S.PDiag(diag::warn_printf_n_specifier), getLocationOfByte(CS.getStart()), /*IsStringLocation*/true,
+ getSpecifierRange(startSpecifier, specifierLen));
+ }
+
// Only scalars are allowed for os_trace.
if (FSType == Sema::FST_OSTrace &&
(CS.getKind() == ConversionSpecifier::PArg ||
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9226,6 +9226,8 @@
def warn_printf_insufficient_data_args : Warning<
"more '%%' conversions than data arguments">, InGroup;
+def warn_printf_n_specifier : Warning<
+ "usage of '%%n' is unsafe">, InGroup;
def warn_printf_data_arg
[PATCH] D110436: [WIP] Add %n format specifier warning
Jaysonyan updated this revision to Diff 375887.
Jaysonyan added a comment.
- Add more verbose warning message
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110436/new/
https://reviews.llvm.org/D110436
Files:
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaChecking.cpp
clang/test/FixIt/format.m
clang/test/Misc/warning-wall.c
clang/test/Sema/format-string-percentn.c
clang/test/Sema/format-strings-size_t.c
clang/test/Sema/format-strings.c
Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs %s
-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -isystem %S/Inputs -fno-signed-char %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-format-n-specifier -Wformat-nonliteral -isystem %S/Inputs %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-format-n-specifier -Wformat-nonliteral -isystem %S/Inputs -fno-signed-char %s
#include
#include
@@ -644,6 +644,7 @@
test14_bar("%", "%d", p); // expected-warning{{incomplete format specifier}}
}
+#pragma GCC diagnostic ignored "-Wformat-n-specifier"
void test_qualifiers(volatile int *vip, const int *cip,
const volatile int *cvip) {
printf("%n", cip); // expected-warning{{format specifies type 'int *' but the argument has type 'const int *'}}
Index: clang/test/Sema/format-strings-size_t.c
===
--- clang/test/Sema/format-strings-size_t.c
+++ clang/test/Sema/format-strings-size_t.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -Wno-format-n-specifier -fsyntax-only -verify %s
int printf(char const *, ...);
Index: clang/test/Sema/format-string-percentn.c
===
--- /dev/null
+++ clang/test/Sema/format-string-percentn.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int printf(const char *restrict, ...);
+void percentn(volatile int *n) {
+ printf("%n", n); // expected-warning{{usage of '%n' can lead to unsafe writing to memory}}
+}
Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -17,6 +17,7 @@
CHECK-NEXT: -Wformat-y2k
CHECK-NEXT: -Wformat-invalid-specifier
CHECK-NEXT: -Wformat-insufficient-args
+CHECK-NEXT: -Wformat-n-specifier
CHECK-NEXT:-Wfor-loop-analysis
CHECK-NEXT:-Wframe-address
CHECK-NEXT:-Wimplicit
Index: clang/test/FixIt/format.m
===
--- clang/test/FixIt/format.m
+++ clang/test/FixIt/format.m
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple %itanium_abi_triple -fsyntax-only -fblocks -verify %s
-// RUN: %clang_cc1 -triple %itanium_abi_triple -fdiagnostics-parseable-fixits -fblocks %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -Wno-format-n-specifier -fsyntax-only -fblocks -verify %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -Wno-format-n-specifier -fdiagnostics-parseable-fixits -fblocks %s 2>&1 | FileCheck %s
@class NSString;
extern void NSLog(NSString *, ...);
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -8965,6 +8965,14 @@
return true;
}
+ // %n can lead to unsafe writing to memory
+ if (CS.getKind() == ConversionSpecifier::nArg) {
+EmitFormatDiagnostic(S.PDiag(diag::warn_printf_n_specifier),
+ getLocationOfByte(CS.getStart()),
+ /*IsStringLocation*/ true,
+ getSpecifierRange(startSpecifier, specifierLen));
+ }
+
// Only scalars are allowed for os_trace.
if (FSType == Sema::FST_OSTrace &&
(CS.getKind() == ConversionSpecifier::PArg ||
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9226,6 +9226,8 @@
def warn_printf_insufficient_data_args : Warning<
"more '%%' conversions than data arguments">, InGroup;
+def warn_printf_n_specifier : Warning<
+ "usage of '%%n' can lead to unsafe writing to memory">, InGroup;
def warn_printf_data_arg_not_used : Warning<
"data argument not used by format string">, InGroup;
def warn_format_invalid_conversion : Warning<
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
[PATCH] D110436: Add %n format specifier warning
Jaysonyan added a comment. Since no discussion came out of the RFC I'll leave the warning under the `-Wformat-n-specifier` flag under `-Wformat` unless there's other ideas brought up. Would appreciate any reviews at this points! :) Repository: rG LLVM Github Monorepo 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
[PATCH] D110436: Add %n format specifier warning
Jaysonyan added inline comments.
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9230
+def warn_printf_n_specifier : Warning<
+ "usage of '%%n' can lead to unsafe writing to memory">,
InGroup;
def warn_printf_data_arg_not_used : Warning<
aaron.ballman wrote:
> Quuxplusone wrote:
> > FWIW, I don't understand why this is "unsafe" either. The problem with `%n`
> > is not that it might be used //intentionally//; the problem is that it
> > opens up an attack vector for //unintentional// (malicious) use. The
> > programmer writes `printf(buf, args...)` where `buf` is under the
> > attacker's control (for example a debug-log format string supplied in a
> > config file), and then the //attacker// configures something like `"%n"`
> > instead of `"%s%d"` (so the debug-logging routine ends up poking data
> > instead of peeking it). This vulnerable `printf(buf, ...)` is exactly what
> > `-Wformat-security` warns about.
> > I am not aware of any vulnerability from //intentional// use of `%n`. At
> > best, one could argue that there's a moral hazard: we might like to remove
> > `%n`-support from our libc's printf, but we can't do that as long as
> > there's any code out there in the wild that relies on intentional use of
> > `%n`. Therefore, this is essentially a "deprecation warning" — but for a
> > feature that AFAIK has never been deprecated, neither by C nor C++! (Am I
> > wrong? Has someone officially deprecated `%n`?)
> FWIW, that's effectively how I view this as well -- it's kinda like `-Wvla`
> -- a diagnostic to say "congrats, you're using the feature!". However, unlike
> `-Wvla`, no one accidentally uses `%n` when they were expecting something
> else.
>
> `%n` isn't deprecated in either C or C++.
That's a really good point, I think you're right that this wouldn't be
effective in catching string format vulnerabilities, because as you said, the
attacker would need to control some portion of the format string which would
already be caught by `-Wformat-security`.
Although in fuchsia I think we still have a need for warning against even
explicit usages of `%n` since it could allow an external user to pass a
pointers into `printf("%n", ptr)` and write to addresses that we don't want to
be written to.
Thank you for the suggestion of clang-tidy. I think moving this warning to the
`bugprone` module sounds like a good way to surface a warning while not
impacting intended usages of `%n`.
Repository:
rG LLVM Github Monorepo
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
[PATCH] D110436: Add %n format specifier warning to clang-tidy
Jaysonyan updated this revision to Diff 381103.
Jaysonyan retitled this revision from "Add %n format specifier warning" to "Add
%n format specifier warning to clang-tidy".
Jaysonyan added a comment.
Herald added a subscriber: mgorny.
Herald added a project: clang-tools-extra.
Move check for `%n` from clang to clang-tidy as a checker under
`bugprone-percent-n-format-specifier`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110436/new/
https://reviews.llvm.org/D110436
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/PercentNFormatSpecifierCheck.cpp
clang-tools-extra/clang-tidy/bugprone/PercentNFormatSpecifierCheck.h
clang-tools-extra/docs/clang-tidy/checks/bugprone-percent-n-format-specifier.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone-percent-n-format-specifier.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-percent-n-format-specifier.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-percent-n-format-specifier.cpp
@@ -0,0 +1,57 @@
+// RUN: %check_clang_tidy %s bugprone-percent-n-format-specifier %t
+
+#include
+
+void testVariableArgumentList(int *I, ...) {
+ char Buffer[100];
+ FILE *pFile;
+ pFile = fopen("myFile.txt", "w+");
+ va_list args;
+ va_start(args, I);
+
+ // CHECK-MESSAGES: [[@LINE+1]]:21: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ vfprintf(pFile, "%n", args);
+ // CHECK-MESSAGES: [[@LINE+1]]:13: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ vprintf("%n", args);
+ // CHECK-MESSAGES: [[@LINE+1]]:28: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ vsnprintf(Buffer, 100, "%n", args);
+ // CHECK-MESSAGES: [[@LINE+1]]:22: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ vsprintf(Buffer, "%n", args);
+
+ // CHECK-MESSAGES: [[@LINE+1]]:20: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ vfscanf(pFile, "%n", args);
+ // CHECK-MESSAGES: [[@LINE+1]]:12: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ vscanf("%n", args);
+ // CHECK-MESSAGES: [[@LINE+1]]:21: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ vsscanf(Buffer, "%n", args);
+
+ va_end(args);
+}
+
+void testPrintf() {
+ int *I = nullptr;
+ char Buffer[100];
+ FILE *pFile;
+ pFile = fopen("myFile.txt", "w+");
+ // CHECK-MESSAGES: [[@LINE+1]]:12: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ printf("%n", I);
+ // CHECK-MESSAGES: [[@LINE+1]]:22: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ fprintf(pFile, "I %n", I);
+ // CHECK-MESSAGES: [[@LINE+1]]:27: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ snprintf(Buffer, 100, "%n", I);
+ // CHECK-MESSAGES: [[@LINE+1]]:21: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ sprintf(Buffer, "%n", I);
+}
+
+void testScanf() {
+ int *I = nullptr;
+ char Buffer[100];
+ FILE *pFile;
+ pFile = fopen("myFile.txt", "w+");
+ // CHECK-MESSAGES: [[@LINE+1]]:19: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ fscanf(pFile, "%n", I);
+ // CHECK-MESSAGES: [[@LINE+1]]:11: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ scanf("%n", I);
+ // CHECK-MESSAGES: [[@LINE+1]]:20: warning: usage of %n can lead to unsafe writing to memory [bugprone-percent-n-format-specifier]
+ sscanf(Buffer, "%n", I);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-percent-n-format-specifier.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-percent-n-format-specifier.rst
@@ -0,0 +1,37 @@
+.. title:: clang-tidy - bugprone-percent-n-format-specifier
+
+bugprone-percent-n-format-specifier
+===
+
+Finds any usage of the %n format specifier inside the format string
+of a call to printf, scanf, or any of their derivatives. The %n format
+specifier can lead to unsafe writing to memory.
+
+.. code-block:: c++
+ void f(int * i, ...) {
+ char buffer [100];
+ FILE * pFile;
+ pFile = fopen("myFile.txt", "w+");
+ va_list args;
+ va_start(args, i);
+
+ // Will match any of these printf derivatives
+ printf("%n", i);
+ fprintf(pFile, "%n", i);
+ snprintf(buffer, 100, "%n", i);
+ sprintf(buffer, "%n", i);
+ vprintf("%n", args);
+ vfprintf(pFil
[PATCH] D110436: Add %n format specifier warning to clang-tidy
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
[PATCH] D110436: Add %n format specifier warning to clang-tidy
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
