[-Wunsafe-buffer-usage] Fix a bug and suppress libc warnings for C files #109496

ziqingluo-90 · 2024-09-20T23:36:36Z

Fix a bug in UnsafeBufferUsage.cpp related to casting to PointerType (report by @hnrklssn here)
Suppress -Wunsafe-buffer-usage-in-libc-call for C files

llvmbot · 2024-09-20T23:37:11Z

@llvm/pr-subscribers-clang-analysis

Author: Ziqing Luo (ziqingluo-90)

Changes

Fix a bug in UnsafeBufferUsage.cpp related to casting to PointerType (report by @hnrklssn here)
Suppress -Wunsafe-buffer-usage-in-libc-call for C files

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

3 Files Affected:

(modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+5-3)
(modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+4-2)
(added) clang/test/SemaCXX/warn-unsafe-buffer-usage-no-libc-functions-in-c.c (+9)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index a16762244b1766..110a121e71a7d2 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -250,7 +250,9 @@ AST_MATCHER_P(Stmt, ignoreUnsafeBufferInContainer,
 
 AST_MATCHER_P(Stmt, ignoreUnsafeLibcCall, const UnsafeBufferUsageHandler *,
               Handler) {
-  return Handler->ignoreUnsafeBufferInLibcCall(Node.getBeginLoc());
+  if (Finder->getASTContext().getLangOpts().CPlusPlus)
+    return Handler->ignoreUnsafeBufferInLibcCall(Node.getBeginLoc());
+  return true; /* Only warn about libc calls for C++ */
 }
 
 AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher<Expr>, innerMatcher) {
@@ -789,7 +791,7 @@ AST_MATCHER_P(CallExpr, hasUnsafePrintfStringArg,
   if (!FristParmTy->isPointerType())
     return false; // possibly some user-defined printf function
 
-  QualType FirstPteTy = (cast<PointerType>(FristParmTy))->getPointeeType();
+  QualType FirstPteTy = FristParmTy->getAs<PointerType>()->getPointeeType();
 
   if (!Ctx.getFILEType()
            .isNull() && //`FILE *` must be in the context if it is fprintf
@@ -865,7 +867,7 @@ AST_MATCHER(CallExpr, hasUnsafeSnprintfBuffer) {
   if (!FirstParmTy->isPointerType())
     return false; // Not an snprint
 
-  QualType FirstPteTy = cast<PointerType>(FirstParmTy)->getPointeeType();
+  QualType FirstPteTy = FirstParmTy->getAs<PointerType>()->getPointeeType();
   const Expr *Buf = Node.getArg(0), *Size = Node.getArg(1);
 
   if (FirstPteTy.isConstQualified() || !Buf->getType()->isPointerType() ||
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 117b2c8bc57935..724b543cdf882c 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2549,6 +2549,7 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
   DiagnosticOptions &DiagOpts = Diags.getDiagnosticOptions();
 
   // UnsafeBufferUsage analysis settings.
+  bool IsCXXLang = S.getLangOpts().CPlusPlus;
   bool UnsafeBufferUsageCanEmitSuggestions = S.getLangOpts().CPlusPlus20;
   bool UnsafeBufferUsageShouldEmitSuggestions =  // Should != Can.
       UnsafeBufferUsageCanEmitSuggestions &&
@@ -2567,8 +2568,9 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
                          Node->getBeginLoc()) ||
         !Diags.isIgnored(diag::warn_unsafe_buffer_usage_in_container,
                          Node->getBeginLoc()) ||
-        !Diags.isIgnored(diag::warn_unsafe_buffer_libc_call,
-                         Node->getBeginLoc())) {
+        (!Diags.isIgnored(diag::warn_unsafe_buffer_libc_call,
+                          Node->getBeginLoc()) &&
+         IsCXXLang /* we only warn about libc calls in C++ files */)) {
       clang::checkUnsafeBufferUsage(Node, R,
                                     UnsafeBufferUsageShouldEmitSuggestions);
     }
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-no-libc-functions-in-c.c b/clang/test/SemaCXX/warn-unsafe-buffer-usage-no-libc-functions-in-c.c
new file mode 100644
index 00000000000000..e305c3e140dff9
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-no-libc-functions-in-c.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -Wunsafe-buffer-usage %s -verify %s
+
+void* memcpy(void *dst,const void *src, unsigned long size);
+
+void f(int *p, int *q) {
+
+  memcpy(p, q, 10); // no libc warn in C
+  ++p[5];           // expected-warning{{unsafe buffer access}}
+}

llvmbot · 2024-09-20T23:37:11Z

@llvm/pr-subscribers-clang

Author: Ziqing Luo (ziqingluo-90)

Changes

Fix a bug in UnsafeBufferUsage.cpp related to casting to PointerType (report by @hnrklssn here)
Suppress -Wunsafe-buffer-usage-in-libc-call for C files

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

3 Files Affected:

(modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+5-3)
(modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+4-2)
(added) clang/test/SemaCXX/warn-unsafe-buffer-usage-no-libc-functions-in-c.c (+9)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index a16762244b1766..110a121e71a7d2 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -250,7 +250,9 @@ AST_MATCHER_P(Stmt, ignoreUnsafeBufferInContainer,
 
 AST_MATCHER_P(Stmt, ignoreUnsafeLibcCall, const UnsafeBufferUsageHandler *,
               Handler) {
-  return Handler->ignoreUnsafeBufferInLibcCall(Node.getBeginLoc());
+  if (Finder->getASTContext().getLangOpts().CPlusPlus)
+    return Handler->ignoreUnsafeBufferInLibcCall(Node.getBeginLoc());
+  return true; /* Only warn about libc calls for C++ */
 }
 
 AST_MATCHER_P(CastExpr, castSubExpr, internal::Matcher<Expr>, innerMatcher) {
@@ -789,7 +791,7 @@ AST_MATCHER_P(CallExpr, hasUnsafePrintfStringArg,
   if (!FristParmTy->isPointerType())
     return false; // possibly some user-defined printf function
 
-  QualType FirstPteTy = (cast<PointerType>(FristParmTy))->getPointeeType();
+  QualType FirstPteTy = FristParmTy->getAs<PointerType>()->getPointeeType();
 
   if (!Ctx.getFILEType()
            .isNull() && //`FILE *` must be in the context if it is fprintf
@@ -865,7 +867,7 @@ AST_MATCHER(CallExpr, hasUnsafeSnprintfBuffer) {
   if (!FirstParmTy->isPointerType())
     return false; // Not an snprint
 
-  QualType FirstPteTy = cast<PointerType>(FirstParmTy)->getPointeeType();
+  QualType FirstPteTy = FirstParmTy->getAs<PointerType>()->getPointeeType();
   const Expr *Buf = Node.getArg(0), *Size = Node.getArg(1);
 
   if (FirstPteTy.isConstQualified() || !Buf->getType()->isPointerType() ||
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 117b2c8bc57935..724b543cdf882c 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2549,6 +2549,7 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
   DiagnosticOptions &DiagOpts = Diags.getDiagnosticOptions();
 
   // UnsafeBufferUsage analysis settings.
+  bool IsCXXLang = S.getLangOpts().CPlusPlus;
   bool UnsafeBufferUsageCanEmitSuggestions = S.getLangOpts().CPlusPlus20;
   bool UnsafeBufferUsageShouldEmitSuggestions =  // Should != Can.
       UnsafeBufferUsageCanEmitSuggestions &&
@@ -2567,8 +2568,9 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
                          Node->getBeginLoc()) ||
         !Diags.isIgnored(diag::warn_unsafe_buffer_usage_in_container,
                          Node->getBeginLoc()) ||
-        !Diags.isIgnored(diag::warn_unsafe_buffer_libc_call,
-                         Node->getBeginLoc())) {
+        (!Diags.isIgnored(diag::warn_unsafe_buffer_libc_call,
+                          Node->getBeginLoc()) &&
+         IsCXXLang /* we only warn about libc calls in C++ files */)) {
       clang::checkUnsafeBufferUsage(Node, R,
                                     UnsafeBufferUsageShouldEmitSuggestions);
     }
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-no-libc-functions-in-c.c b/clang/test/SemaCXX/warn-unsafe-buffer-usage-no-libc-functions-in-c.c
new file mode 100644
index 00000000000000..e305c3e140dff9
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-no-libc-functions-in-c.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -Wunsafe-buffer-usage %s -verify %s
+
+void* memcpy(void *dst,const void *src, unsigned long size);
+
+void f(int *p, int *q) {
+
+  memcpy(p, q, 10); // no libc warn in C
+  ++p[5];           // expected-warning{{unsafe buffer access}}
+}

- Fix a bug in UnsafeBufferUsage.cpp related to casting to PointerType - Suppress -Wunsafe-buffer-usage-in-libc-call for C files (rdar://117182250)

hnrklssn

You may want to add some tests with annotated pointers to verify that you treat them correctly, e.g. _Nullable

hnrklssn · 2024-09-21T00:02:37Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -789,7 +791,7 @@ AST_MATCHER_P(CallExpr, hasUnsafePrintfStringArg,
  if (!FristParmTy->isPointerType())
    return false; // possibly some user-defined printf function

-  QualType FirstPteTy = (cast<PointerType>(FristParmTy))->getPointeeType();
+  QualType FirstPteTy = FristParmTy->getAs<PointerType>()->getPointeeType();


Nit: FristParmTy -> FirstParmTy

jkorous-apple · 2024-09-21T02:10:29Z

clang/test/SemaCXX/warn-unsafe-buffer-usage-no-libc-functions-in-c.c

@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -Wunsafe-buffer-usage %s -verify %s


FWIW the language mode is set via -x command line option. It is only inferred from the file extension if -x is missing.

jkorous-apple · 2024-09-21T02:11:29Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

-  return Handler->ignoreUnsafeBufferInLibcCall(Node.getBeginLoc());
+  if (Finder->getASTContext().getLangOpts().CPlusPlus)
+    return Handler->ignoreUnsafeBufferInLibcCall(Node.getBeginLoc());
+  return true; /* Only warn about libc calls for C++ */


This implementation should warn in Obj-C++ too and it should not in Obj-C but you might want to test that.

jkorous-apple · 2024-09-21T02:13:29Z

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -2549,6 +2549,7 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
  DiagnosticOptions &DiagOpts = Diags.getDiagnosticOptions();

  // UnsafeBufferUsage analysis settings.
+  bool IsCXXLang = S.getLangOpts().CPlusPlus;


Nit: it doesn't feel like we need a variable for that.

jkorous-apple

Left some comments but LGTM.

haoNoQ

LGTM!

haoNoQ · 2024-09-21T02:29:14Z

clang/lib/Analysis/UnsafeBufferUsage.cpp

    return false; // possibly some user-defined printf function

-  QualType FirstPteTy = (cast<PointerType>(FristParmTy))->getPointeeType();
+  QualType FirstPteTy = FirstParmTy->getAs<PointerType>()->getPointeeType();


The difference between castAs and getAs is that castAs asserts the canonical type is PointerType while getAs returns null if not. I think castAs is more suitable for our case.

…les (llvm#109496) - Fix a bug in UnsafeBufferUsage.cpp related to casting to PointerType - Suppress -Wunsafe-buffer-usage-in-libc-call for C files (rdar://117182250) (cherry picked from commit 090dc77)

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:analysis labels Sep 20, 2024

ziqingluo-90 requested a review from haoNoQ September 20, 2024 23:37

ziqingluo-90 requested review from malavikasamak, t-rasmud and jkorous-apple September 20, 2024 23:37

[-Wunsafe-buffer-usage] Fix a bug and suppress libc warnings for C files

e7f7f82

- Fix a bug in UnsafeBufferUsage.cpp related to casting to PointerType - Suppress -Wunsafe-buffer-usage-in-libc-call for C files (rdar://117182250)

ziqingluo-90 force-pushed the dev/ziqing/warn-libc-funcs-bug-fix branch from e261352 to e7f7f82 Compare September 20, 2024 23:59

hnrklssn reviewed Sep 21, 2024

View reviewed changes

Add tests and fix a typo

42664f0

jkorous-apple reviewed Sep 21, 2024

View reviewed changes

jkorous-apple approved these changes Sep 21, 2024

View reviewed changes

haoNoQ approved these changes Sep 21, 2024

View reviewed changes

ziqingluo-90 added 2 commits September 20, 2024 19:33

address comments

9e0e31b

Change memcpy to __asan_memcpy to avoid libc function redeclaration

1797521

ziqingluo-90 merged commit 090dc77 into llvm:main Sep 22, 2024
8 checks passed

ziqingluo-90 deleted the dev/ziqing/warn-libc-funcs-bug-fix branch December 19, 2024 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[-Wunsafe-buffer-usage] Fix a bug and suppress libc warnings for C files #109496

[-Wunsafe-buffer-usage] Fix a bug and suppress libc warnings for C files #109496

Uh oh!

ziqingluo-90 commented Sep 20, 2024

Uh oh!

llvmbot commented Sep 20, 2024

Uh oh!

llvmbot commented Sep 20, 2024

Uh oh!

hnrklssn left a comment

Uh oh!

hnrklssn Sep 21, 2024

Uh oh!

jkorous-apple Sep 21, 2024 •

edited

Loading

Uh oh!

jkorous-apple Sep 21, 2024

Uh oh!

jkorous-apple Sep 21, 2024

Uh oh!

jkorous-apple left a comment

Uh oh!

haoNoQ left a comment

Uh oh!

haoNoQ Sep 21, 2024

Uh oh!

ziqingluo-90 Sep 21, 2024

Uh oh!

Uh oh!

Uh oh!

		@@ -0,0 +1,9 @@
		// RUN: %clang_cc1 -Wunsafe-buffer-usage %s -verify %s

[-Wunsafe-buffer-usage] Fix a bug and suppress libc warnings for C files #109496

[-Wunsafe-buffer-usage] Fix a bug and suppress libc warnings for C files #109496

Uh oh!

Conversation

ziqingluo-90 commented Sep 20, 2024

Uh oh!

llvmbot commented Sep 20, 2024

Uh oh!

llvmbot commented Sep 20, 2024

Uh oh!

hnrklssn left a comment

Choose a reason for hiding this comment

Uh oh!

hnrklssn Sep 21, 2024

Choose a reason for hiding this comment

Uh oh!

jkorous-apple Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkorous-apple Sep 21, 2024

Choose a reason for hiding this comment

Uh oh!

jkorous-apple Sep 21, 2024

Choose a reason for hiding this comment

Uh oh!

jkorous-apple left a comment

Choose a reason for hiding this comment

Uh oh!

haoNoQ left a comment

Choose a reason for hiding this comment

Uh oh!

haoNoQ Sep 21, 2024

Choose a reason for hiding this comment

Uh oh!

ziqingluo-90 Sep 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkorous-apple Sep 21, 2024 •

edited

Loading