[clang][Sema] Make format size estimator aware of %p's existence in format string #65969

hazohelet · 2023-09-11T15:54:03Z

GCC stops counting format string's size when it sees %p format in order to avoid Wformat-truncation false positive against Linux kernel's format extension (%pOF, for example).
This change makes clang's behavior align with GCC's.
As requested in #64871

llvmbot · 2023-09-11T15:55:15Z

@llvm/pr-subscribers-clang

Changes

GCC stops counting format string's size when it sees %p format in order to avoid Wformat-truncation false positive against Linux kernel's format extension (%pOF, for example).
This change makes clang's behavior align with GCC's.
As requested in #64871

--
Full diff: https://github.com/llvm/llvm-project/pull/65969.diff

4 Files Affected:

(modified) clang/include/clang/AST/FormatString.h (+6)
(modified) clang/lib/AST/PrintfFormatString.cpp (+7-1)
(modified) clang/lib/Sema/SemaChecking.cpp (+10)
(modified) clang/test/Sema/warn-fortify-source.c (+13-3)

diff --git a/clang/include/clang/AST/FormatString.h b/clang/include/clang/AST/FormatString.h
index 5c4ad9baaef608c..97ce3cfe0b25c68 100644
--- a/clang/include/clang/AST/FormatString.h
+++ b/clang/include/clang/AST/FormatString.h
@@ -742,6 +742,12 @@ class FormatStringHandler {
     return true;
   }
 
+  virtual bool
+  ShouldStopOnFormatSpecifier(const analyze_printf::PrintfSpecifier &FS,
+                              unsigned RemainLen) {
+    return false;
+  }
+
   /// Handle mask types whose sizes are not between one and eight bytes.
   virtual void handleInvalidMaskType(StringRef MaskType) {}
 
diff --git a/clang/lib/AST/PrintfFormatString.cpp b/clang/lib/AST/PrintfFormatString.cpp
index f0b9d0ecaf23461..750f584d8d37a76 100644
--- a/clang/lib/AST/PrintfFormatString.cpp
+++ b/clang/lib/AST/PrintfFormatString.cpp
@@ -429,7 +429,13 @@ bool clang::analyze_format_string::ParsePrintfString(FormatStringHandler &H,
     // we can recover from?
     if (!FSR.hasValue())
       continue;
-    // We have a format specifier.  Pass it to the callback.
+    // We have a format specifier.
+    // In case the handler needs to abort parsing upon specific specifiers
+    if (H.ShouldStopOnFormatSpecifier(
+            FSR.getValue(), static_cast(E - FSR.getStart()))) {
+      return false;
+    }
+    // Pass the format specifier to the callback.
     if (!H.HandlePrintfSpecifier(FSR.getValue(), FSR.getStart(),
                                  I - FSR.getStart(), Target))
       return true;
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 3b4ac613da76aa8..1a6ff6cfa19b661 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -860,6 +860,16 @@ class EstimateSizeFormatHandler
       : Size(std::min(Format.find(0), Format.size()) +
              1 /* null byte always written by sprintf */) {}
 
+  bool ShouldStopOnFormatSpecifier(const analyze_printf::PrintfSpecifier &FS,
+                                   unsigned RemainLen) override {
+    if (FS.getConversionSpecifier().getKind() ==
+        analyze_printf::PrintfConversionSpecifier::pArg) {
+      assert(Size >= RemainLen && "no underflow");
+      Size -= RemainLen;
+      return true;
+    }
+    return false;
+  }
   bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS,
                              const char *, unsigned SpecifierLen,
                              const TargetInfo &) override {
diff --git a/clang/test/Sema/warn-fortify-source.c b/clang/test/Sema/warn-fortify-source.c
index 5ed9782b26fb788..fba6c718be4a33a 100644
--- a/clang/test/Sema/warn-fortify-source.c
+++ b/clang/test/Sema/warn-fortify-source.c
@@ -85,7 +85,7 @@ void call_memset(void) {
   __builtin_memset(buf, 0xff, 11); // expected-warning {{'memset' will always overflow; destination buffer has size 10, but size argument is 11}}
 }
 
-void call_snprintf(double d) {
+void call_snprintf(double d, int *ptr) {
   char buf[10];
   __builtin_snprintf(buf, 10, "merp");
   __builtin_snprintf(buf, 11, "merp"); // expected-warning {{'snprintf' size argument is too large; destination buffer has size 10, but size argument is 11}}
@@ -96,6 +96,11 @@ void call_snprintf(double d) {
   __builtin_snprintf(buf, 1, "%.1000g", d); // expected-warning {{'snprintf' will always be truncated; specified size is 1, but format string expands to at least 2}}
   __builtin_snprintf(buf, 5, "%.1000g", d);
   __builtin_snprintf(buf, 5, "%.1000G", d);
+  char node_name[6];
+  __builtin_snprintf(node_name, sizeof(node_name), "%pOFn", ptr);
+  __builtin_snprintf(node_name, sizeof(node_name), "12345%pOFn", ptr);
+  __builtin_snprintf(node_name, sizeof(node_name), "123456%pOFn", ptr); // expected-warning {{'snprintf' will always be truncated; specified size is 6, but format string expands to at least 7}}
+  __builtin_snprintf(node_name, sizeof(node_name), "1234567%pOFn", ptr); // expected-warning {{'snprintf' will always be truncated; specified size is 6, but format string expands to at least 8}}
 }
 
 void call_vsnprintf(void) {
@@ -110,6 +115,11 @@ void call_vsnprintf(void) {
   __builtin_vsnprintf(buf, 1, "%.1000g", list); // expected-warning {{'vsnprintf' will always be truncated; specified size is 1, but format string expands to at least 2}}
   __builtin_vsnprintf(buf, 5, "%.1000g", list);
   __builtin_vsnprintf(buf, 5, "%.1000G", list);
+  char node_name[6];
+  __builtin_snprintf(node_name, sizeof(node_name), "%pOFn", list);
+  __builtin_snprintf(node_name, sizeof(node_name), "12345%pOFn", list);
+  __builtin_snprintf(node_name, sizeof(node_name), "123456%pOFn", list); // expected-warning {{'snprintf' will always be truncated; specified size is 6, but format string expands to at least 7}}
+  __builtin_snprintf(node_name, sizeof(node_name), "1234567%pOFn", list); // expected-warning {{'snprintf' will always be truncated; specified size is 6, but format string expands to at least 8}}
 }
 
 void call_sprintf_chk(char *buf) {
@@ -149,7 +159,7 @@ void call_sprintf_chk(char *buf) {
   __builtin___sprintf_chk(buf, 1, 4, "%#x", 9);
   __builtin___sprintf_chk(buf, 1, 3, "%#x", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 3, but format string expands to at least 4}}
   __builtin___sprintf_chk(buf, 1, 4, "%p", (void *)9);
-  __builtin___sprintf_chk(buf, 1, 3, "%p", (void *)9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 3, but format string expands to at least 4}}
+  __builtin___sprintf_chk(buf, 1, 3, "%p", (void *)9);
   __builtin___sprintf_chk(buf, 1, 3, "%+d", 9);
   __builtin___sprintf_chk(buf, 1, 2, "%+d", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 2, but format string expands to at least 3}}
   __builtin___sprintf_chk(buf, 1, 3, "% i", 9);
@@ -186,7 +196,7 @@ void call_sprintf(void) {
   sprintf(buf, "12%#x", 9);
   sprintf(buf, "123%#x", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}
   sprintf(buf, "12%p", (void *)9);
-  sprintf(buf, "123%p", (void *)9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}
+  sprintf(buf, "123%p", (void *)9);
   sprintf(buf, "123%+d", 9);
   sprintf(buf, "1234%+d", 9); // expected-warning {{'sprintf' will always overflow; destination buffer has size 6, but format string expands to at least 7}}
   sprintf(buf, "123% i", 9);

clang/test/Sema/warn-fortify-source.c

nickdesaulniers

Thanks for the patch!

Can we document this behavior somewhere?

clang/lib/Sema/SemaChecking.cpp

clang/lib/AST/PrintfFormatString.cpp

zygoloid · 2023-09-11T16:21:06Z

Is there some way we can narrow the scope of this patch so we don't lose warnings for normal snprintfs, only for the kernel one? If we really can't tell the difference from the source code, we could move the affected warnings to a different warning group instead, so the kernel can turn them off.

hazohelet · 2023-09-12T12:36:13Z

Is there some way we can narrow the scope of this patch so we don't lose warnings for normal snprintfs, only for the kernel one? If we really can't tell the difference from the source code, we could move the affected warnings to a different warning group instead, so the kernel can turn them off.

Thanks for the feedback. I narrowed the scope by checking whether the next character of %p can be part of the kernel extended format specifier.
If people write %pM or a similar outside kernel, it still aborts the format size estimator and could lead to false negative. But I think it would hurt people much less because people usually put non-alphanumeric character after %p.

clang/lib/Sema/SemaChecking.cpp

nickdesaulniers · 2023-09-12T16:23:11Z

Is there some way we can narrow the scope of this patch so we don't lose warnings for normal snprintfs, only for the kernel one? If we really can't tell the difference from the source code, we could move the affected warnings to a different warning group instead, so the kernel can turn them off.

@zygoloid is this approach acceptable?

One thing I do worry about not having an explicit flag; we support clang-11+ for building the Linux kernel.

Let's say the kernel adds a new extension format specifiers %pn. We need to fix up clang to recognize that now, too. Then we fix clang in say clang-19. clang-18 still warns and we can't turn it off.

Perhaps it would be better to have the flag to disable %p without disabling the rest of -Wfortify-source, than try to keep these two in sync.

nickdesaulniers · 2023-09-12T16:14:56Z

clang/docs/ReleaseNotes.rst

+  if the succeeding character can be a part of the format specifier in linux's
+  format extension in order to avoid false positive warnings against the linux
+  code base.


perhaps worth referring to "the Linux kernel's ..." rather than just "linux".

clang/lib/Sema/SemaChecking.cpp

zygoloid

This looks like the wrong approach to me. I really think you should be splitting the warning into two parts, so that only the kernel developers see a change in diagnostic output. That would seem to make this patch simpler as well as avoiding a (probably unimportant) regression for all other Clang users.

clang/lib/Sema/SemaChecking.cpp

hazohelet · 2023-09-13T07:24:00Z

Thanks for the detailed suggestion! I agree it's the appropriate approach.

I was planning to separate Wformat-truncation from Wfortify-source sometime after this PR (as was requested in the same issue), but now that we are going to introduce a new warning group, we should finish the separation before proceeding further on this.
Sadly we don't have any way to make stacked patches as of now, so I'll merge the separation change to this PR. If reviewers think it's unreasonable, let's make another PR for the separation then.

… from Wfortify-source Newly introduces `Wformat-overflow` and `Wformat-truncation` and imports the existing warning from `Wfortify-source` that correspond to GCC's counterpart so that they can be disabled separately from `Wfortify-source`.

…n-kprintf` for format string that contains `%p`

zygoloid

I'm happy with the overall approach here; please don't block on further review from me.

zygoloid · 2023-09-13T18:54:39Z

clang/include/clang/Basic/DiagnosticGroups.td

+def FormatOverflowNonKprintf: DiagGroup<"format-overflow-non-kprintf">;
+def FormatOverflow: DiagGroup<"format-overflow", [FormatOverflowNonKprintf]>;
+def FormatTruncationNonKprintf: DiagGroup<"format-truncation-non-kprintf">;
+def FormatTruncation: DiagGroup<"format-truncation", [FormatTruncationNonKprintf]>;


Should these be added to the FortifySource group in addition to adding them to Format?

Thanks. It makes sense to add this to FortifySource for backward compatibility.

clang/lib/Sema/SemaChecking.cpp

Requested changes have been made.

nickdesaulniers · 2023-09-13T20:22:53Z

@nathanchance and I have done a bunch of testing of this revision (see comments ClangBuiltLinux/linux#1923 (comment) and down); this looks like something that will work nicely for the Linux kernel's use case! Thanks for all of the work here @hazohelet !

`-Wfortify-source` as well as `-Wformat`

nickdesaulniers

Thanks for the fix!

zygoloid

Thank you!

nickdesaulniers · 2023-09-22T16:04:09Z

ping @hazohelet . This should be good to land and should help clear some red in our CI for Linux kernel builds.

hazohelet · 2023-09-25T01:00:40Z

ping @hazohelet . This should be good to land and should help clear some red in our CI for Linux kernel builds.

Thanks for the ping. I was away for a while. I'm merging this after updating the PR descriptions.

Recently, clang added support for -Wformat-overflow and -Wformat-truncation. When building the kernel, it was discovered that clang's implementation of these warnings handles the '%p' specifier, which differs from GCC's implementation. This results in false positive warnings due to the kernel's various '%p' extensions. Fortunately, the clang developers placed this warning difference into a separate flag, allowing the kernel to turn off the warning for '%p' unconditionally. This is not currently an issue for a normal build, as -Wformat-overflow and -Wformat-truncation are unconditionally disabled, which includes this sub-warning. However, ever since commit 6d4ab2e ("extrawarn: enable format and stringop overflow warnings in W=1"), these warnings are in W=1 and the goal is to enable them in the normal build once they are all eliminated. Disable the warnings for W=1 to avoid false positives. This block should move with -Wformat-overflow and -Wformat-truncation when they are enabled for a normal build. Link: ClangBuiltLinux#1923 Link: llvm/llvm-project#64871 Link: llvm/llvm-project#65969 Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111219 Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78512 Signed-off-by: Nathan Chancellor <[email protected]>

hazohelet added clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer labels Sep 11, 2023

hazohelet requested review from nickdesaulniers, AaronBallman and nathanchance September 11, 2023 15:54

hazohelet requested a review from a team as a code owner September 11, 2023 15:54

llvmbot added the clang Clang issues not falling into any other category label Sep 11, 2023

hazohelet commented Sep 11, 2023

View reviewed changes

clang/test/Sema/warn-fortify-source.c Outdated Show resolved Hide resolved

nickdesaulniers reviewed Sep 11, 2023

View reviewed changes

clang/lib/Sema/SemaChecking.cpp Outdated Show resolved Hide resolved

clang/lib/AST/PrintfFormatString.cpp Outdated Show resolved Hide resolved

hazohelet commented Sep 12, 2023

View reviewed changes

clang/lib/Sema/SemaChecking.cpp Outdated Show resolved Hide resolved

nickdesaulniers mentioned this pull request Sep 12, 2023

Several new instances of -Wfortify-source after LLVM commit 0c9c9dd9a24f9 ClangBuiltLinux/linux#1923

Closed

nickdesaulniers reviewed Sep 12, 2023

View reviewed changes

zygoloid previously requested changes Sep 13, 2023

View reviewed changes

clang/lib/Sema/SemaChecking.cpp Show resolved Hide resolved

hazohelet added 2 commits September 13, 2023 18:17

Introduce -Wformat-overflow-non-kprintf and `-Wformat-truncation-no…

59ff7cd

…n-kprintf` for format string that contains `%p`

hazohelet force-pushed the kernel-format-p branch from 84481f5 to 59ff7cd Compare September 13, 2023 09:21

nickdesaulniers mentioned this pull request Sep 13, 2023

get -Wformat-truncation and -Wformat-overflow re-enabled KSPP/linux#343

Open

zygoloid reviewed Sep 13, 2023

View reviewed changes

zygoloid requested review from zygoloid and removed request for zygoloid September 13, 2023 18:59

hazohelet added 2 commits September 14, 2023 23:09

Make -Wformat-overflow and -Wformat-truncation grouped in

e951303

`-Wfortify-source` as well as `-Wformat`

[NFC] fix typo in release note

34777e6

nickdesaulniers approved these changes Sep 19, 2023

View reviewed changes

zygoloid approved these changes Sep 20, 2023

View reviewed changes

hazohelet changed the title ~~[clang][Sema] Stop format size estimator upon %p to adapt to linux kernel's extension~~ [clang][Sema] Make format size estimator aware of %p's existence in format string Sep 25, 2023

hazohelet merged commit 56c3b8e into llvm:main Sep 25, 2023

phoebewang mentioned this pull request Sep 25, 2023

[X86][RFC] Support AVX10 options #67278

Merged

hazohelet deleted the kernel-format-p branch September 25, 2023 06:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][Sema] Make format size estimator aware of %p's existence in format string #65969

[clang][Sema] Make format size estimator aware of %p's existence in format string #65969

hazohelet commented Sep 11, 2023

llvmbot commented Sep 11, 2023

nickdesaulniers left a comment

zygoloid commented Sep 11, 2023

hazohelet commented Sep 12, 2023

nickdesaulniers commented Sep 12, 2023

nickdesaulniers Sep 12, 2023

zygoloid left a comment

hazohelet commented Sep 13, 2023

zygoloid left a comment

zygoloid Sep 13, 2023

hazohelet Sep 14, 2023

nickdesaulniers commented Sep 13, 2023

nickdesaulniers left a comment

zygoloid left a comment

nickdesaulniers commented Sep 22, 2023

hazohelet commented Sep 25, 2023

[clang][Sema] Make format size estimator aware of %p's existence in format string #65969

[clang][Sema] Make format size estimator aware of %p's existence in format string #65969

Conversation

hazohelet commented Sep 11, 2023

llvmbot commented Sep 11, 2023

nickdesaulniers left a comment

Choose a reason for hiding this comment

zygoloid commented Sep 11, 2023

hazohelet commented Sep 12, 2023

nickdesaulniers commented Sep 12, 2023

nickdesaulniers Sep 12, 2023

Choose a reason for hiding this comment

zygoloid left a comment

Choose a reason for hiding this comment

hazohelet commented Sep 13, 2023

zygoloid left a comment

Choose a reason for hiding this comment

zygoloid Sep 13, 2023

Choose a reason for hiding this comment

hazohelet Sep 14, 2023

Choose a reason for hiding this comment

nickdesaulniers commented Sep 13, 2023

nickdesaulniers left a comment

Choose a reason for hiding this comment

zygoloid left a comment

Choose a reason for hiding this comment

nickdesaulniers commented Sep 22, 2023

hazohelet commented Sep 25, 2023