[clang] a6302b6 - [-Wunsafe-buffer-usage] Check source location validity before using `TypeLoc`s

[via cfe-commits]

Author: ziqingluo-90
Date: 2023-07-19T15:04:42-07:00
New Revision: a6302b6934b349fff122eeb6c4b39eff580c4b1b

URL: https://github.com/llvm/llvm-project/commit/a6302b6934b349fff122eeb6c4b39eff580c4b1b
DIFF: https://github.com/llvm/llvm-project/commit/a6302b6934b349fff122eeb6c4b39eff580c4b1b.diff

LOG: [-Wunsafe-buffer-usage] Check source location validity before using `TypeLoc`s

The safe-buffer analysis analyzes TypeLocs of types of variable
declarations in order to get source locations of them.

However, in some cases, the source locations of a TypeLoc are not
valid. Using invalid source locations results in assertion violation
or incorrect analysis or fix-its.

It is still not clear to me in what circumstances a TypeLoc does not
have valid source locations (it looks like a bug in Clang to me, but
it is not our responsibility to fix it). So we will conservatively
give up the analysis when required source locations are not valid.

Reviewed By: NoQ (Artem Dergachev)

Differential Revision: https://reviews.llvm.org/D155667

Added: 
    

Modified: 
    clang/lib/Analysis/UnsafeBufferUsage.cpp
    clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 495d171c618c99..5295d9d92b2977 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1385,8 +1385,18 @@ getPointeeTypeText(const ParmVarDecl *VD, const SourceManager &SM,
   TypeLoc PteTyLoc = TyLoc.getUnqualifiedLoc().getNextTypeLoc();
   SourceLocation VDNameStartLoc = VD->getLocation();
 
-  if (!SM.isBeforeInTranslationUnit(PteTyLoc.getSourceRange().getEnd(),
-                                    VDNameStartLoc)) {
+  if (!(VDNameStartLoc.isValid() && PteTyLoc.getSourceRange().isValid())) {
+    // We are expecting these locations to be valid. But in some cases, they are
+    // not all valid. It is a Clang bug to me and we are not responsible for
+    // fixing it.  So we will just give up for now when it happens.
+    return std::nullopt;
+  }
+
+  // Note that TypeLoc.getEndLoc() returns the begin location of the last token:
+  SourceLocation PteEndOfTokenLoc =
+      Lexer::getLocForEndOfToken(PteTyLoc.getEndLoc(), 0, SM, LangOpts);
+
+  if (!SM.isBeforeInTranslationUnit(PteEndOfTokenLoc, VDNameStartLoc)) {
     // We only deal with the cases where the source text of the pointee type
     // appears on the left-hand side of the variable identifier completely,
     // including the following forms:
@@ -1401,13 +1411,8 @@ getPointeeTypeText(const ParmVarDecl *VD, const SourceManager &SM,
     // `PteTy` via source ranges.
     *QualifiersToAppend = PteTy.getQualifiers();
   }
-
-  // Note that TypeLoc.getEndLoc() returns the begin location of the last token:
-  SourceRange CSR{
-      PteTyLoc.getBeginLoc(),
-      Lexer::getLocForEndOfToken(PteTyLoc.getEndLoc(), 0, SM, LangOpts)};
-
-  return getRangeText(CSR, SM, LangOpts)->str();
+  return getRangeText({PteTyLoc.getBeginLoc(), PteEndOfTokenLoc}, SM, LangOpts)
+      ->str();
 }
 
 // Returns the text of the name (with qualifiers) of a `FunctionDecl`.

diff  --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp
index f5be80b518ad32..44f44d0e077e11 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-parm-unsupported.cpp
@@ -33,6 +33,26 @@ void unnamedPointeeType(PTR_TO_ANON p) {  // expected-warning{{'p' is an unsafe
   }
 }
 
+// The analysis requires accurate source location informations from
+// `TypeLoc`s of types of variable (parameter) declarations in order
+// to generate fix-its for them. But those information is not always
+// available (probably due to some bugs in clang but it is irrelevant
+// to the safe-buffer project).  The following is an example.  When
+// `_Atomic` is used, we cannot get valid source locations of the
+// pointee type of `unsigned *`.  The analysis gives up in such a
+// case.
+// CHECK-NOT: fix-it:
+void typeLocSourceLocationInvalid(_Atomic unsigned *map) { // expected-warning{{'map' is an unsafe pointer used for buffer access}}
+  map[5] = 5; // expected-note{{used in buffer access here}}
+}
+
+// CHECK: fix-it:"{{.*}}":{[[@LINE+1]]:33-[[@LINE+1]]:46}:"std::span<unsigned> map"
+void typeLocSourceLocationValid(unsigned *map) { // expected-warning{{'map' is an unsafe pointer used for buffer access}} \
+						    expected-note{{change type of 'map' to 'std::span' to preserve bounds information}}
+  map[5] = 5; // expected-note{{used in buffer access here}}
+}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:2-[[@LINE-1]]:2}:"\n{{\[}}{{\[}}clang::unsafe_buffer_usage{{\]}}{{\]}} void typeLocSourceLocationValid(unsigned *map) {return typeLocSourceLocationValid(std::span<unsigned>(map, <# size #>));}\n"
+
 // We do not fix parameters participating unsafe operations for the
 // following functions/methods or function-like expressions:
 
@@ -128,4 +148,3 @@ void parmWithDefaultValueDecl(int * x) {
   int tmp;
   tmp = x[5]; // expected-note{{used in buffer access here}}
 }
-