[clang][analyzer] Improve bug report in alpha.security.ReturnPtrRange

Add some notes and track of bad return value.

Reviewed By: steakhal

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

clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp

@@ -12,6 +12,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
@@ -79,16 +80,44 @@ void ReturnPointerRangeChecker::checkPreStmt(const ReturnStmt *RS,
           "Returned pointer value points outside the original object "
           "(potential buffer overflow)"));
 
-    // FIXME: It would be nice to eventually make this diagnostic more clear,
-    // e.g., by referencing the original declaration or by saying *why* this
-    // reference is outside the range.
-
     // Generate a report for this bug.
-    auto report =
+    auto Report =
         std::make_unique<PathSensitiveBugReport>(*BT, BT->getDescription(), N);
+    Report->addRange(RetE->getSourceRange());
 
-    report->addRange(RetE->getSourceRange());
+    const auto ConcreteElementCount = ElementCount.getAs<nonloc::ConcreteInt>();
-    C.emitReport(std::move(report));
+    const auto ConcreteIdx = Idx.getAs<nonloc::ConcreteInt>();
+
+    const auto *DeclR = ER->getSuperRegion()->getAs<DeclRegion>();
+
+    if (DeclR)
+      Report->addNote("Original object declared here",
+                      {DeclR->getDecl(), C.getSourceManager()});
+
+    if (ConcreteElementCount) {
+      SmallString<128> SBuf;
+      llvm::raw_svector_ostream OS(SBuf);
+      OS << "Original object ";
+      if (DeclR) {
+        OS << "'";
+        DeclR->getDecl()->printName(OS);
+        OS << "' ";
+      }
+      OS << "is an array of " << ConcreteElementCount->getValue() << " '";
+      ER->getValueType().print(OS,
+                               PrintingPolicy(C.getASTContext().getLangOpts()));
+      OS << "' objects";
+      if (ConcreteIdx) {
+        OS << ", returned pointer points at index " << ConcreteIdx->getValue();
+      }
+
+      Report->addNote(SBuf,
+                      {RetE, C.getSourceManager(), C.getLocationContext()});
+    }
+
+    bugreporter::trackExpressionValue(N, RetE, *Report);
+
+    C.emitReport(std::move(Report));
   }
 }
 

clang/test/Analysis/misc-ps-region-store.m

@@ -461,10 +461,11 @@ void element_region_with_symbolic_superregion(int* p) {
 // Test returning an out-of-bounds pointer (CWE-466)
 //===----------------------------------------------------------------------===//
 
-static int test_cwe466_return_outofbounds_pointer_a[10];
+static int test_cwe466_return_outofbounds_pointer_a[10]; // expected-note{{Original object declared here}}
 int *test_cwe466_return_outofbounds_pointer() {
   int *p = test_cwe466_return_outofbounds_pointer_a+11;
   return p; // expected-warning{{Returned pointer value points outside the original object}}
+            // expected-note@-1{{Original object 'test_cwe466_return_outofbounds_pointer_a' is an array of 10 'int' objects, returned pointer points at index 11}}
 }
 
 //===----------------------------------------------------------------------===//

clang/test/Analysis/return-ptr-range.cpp

@@ -1,29 +1,39 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.security.ReturnPtrRange -analyzer-output text -verify %s
-
-int arr[10];
-int *ptr;
 
 int conjure_index();
 
-int *test_element_index_lifetime() {
+namespace test_element_index_lifetime {
-  do {
+
+int arr[10]; // expected-note{{Original object declared here}} expected-note{{Original object declared here}}
+int *ptr;
+
+int *test_global_ptr() {
+  do { // expected-note{{Loop condition is false.  Exiting loop}}
     int x = conjure_index();
-    ptr = arr + x;
+    ptr = arr + x; // expected-note{{Value assigned to 'ptr'}}
-    if (x != 20)
+    if (x != 20) // expected-note{{Assuming 'x' is equal to 20}}
+                 // expected-note@-1{{Taking false branch}}
       return arr; // no-warning
-  } while (0);
+  } while(0);
-  return ptr; // expected-warning{{Returned pointer value points outside the original object (potential buffer overflow)}}
+  return ptr; // expected-warning{{Returned pointer value points outside the original object (potential buffer overflow) [alpha.security.ReturnPtrRange]}}
+              // expected-note@-1{{Returned pointer value points outside the original object (potential buffer overflow)}}
+              // expected-note@-2{{Original object 'arr' is an array of 10 'int' objects}}
 }
 
-int *test_element_index_lifetime_with_local_ptr() {
+int *test_local_ptr() {
   int *local_ptr;
-  do {
+  do { // expected-note{{Loop condition is false.  Exiting loop}}
     int x = conjure_index();
-    local_ptr = arr + x;
+    local_ptr = arr + x; // expected-note{{Value assigned to 'local_ptr'}}
-    if (x != 20)
+    if (x != 20) // expected-note{{Assuming 'x' is equal to 20}}
+                 // expected-note@-1{{Taking false branch}}
       return arr; // no-warning
-  } while (0);
+  } while(0);
-  return local_ptr; // expected-warning{{Returned pointer value points outside the original object (potential buffer overflow)}}
+  return local_ptr; // expected-warning{{Returned pointer value points outside the original object (potential buffer overflow) [alpha.security.ReturnPtrRange]}}
+                    // expected-note@-1{{Returned pointer value points outside the original object (potential buffer overflow)}}
+                    // expected-note@-2{{Original object 'arr' is an array of 10 'int' objects}}
+}
+
 }
 
 template <typename T, int N>
@@ -55,17 +65,53 @@ void use_iterable_object() {
 
 template <int N>
 class BadIterable {
-  int buffer[N];
+  int buffer[N]; // expected-note{{Original object declared here}}
   int *start, *finish;
 
 public:
-  BadIterable() : start(buffer), finish(buffer + N) {}
+  BadIterable() : start(buffer), finish(buffer + N) {} // expected-note{{Value assigned to 'iter.finish'}}
 
   int* begin() { return start; }
-  int* end() { return finish + 1; } // expected-warning{{Returned pointer value points outside the original object (potential buffer overflow)}}
+  int* end() { return finish + 1; } // expected-warning{{Returned pointer value points outside the original object}}
+                                    // expected-note@-1{{Returned pointer value points outside the original object}}
+                                    // expected-note@-2{{Original object 'buffer' is an array of 20 'int' objects, returned pointer points at index 21}}
 };
 
 void use_bad_iterable_object() {
-  BadIterable<20> iter;
+  BadIterable<20> iter; // expected-note{{Calling default constructor for 'BadIterable<20>'}}
-  iter.end();
+                        // expected-note@-1{{Returning from default constructor for 'BadIterable<20>'}}
+  iter.end(); // expected-note{{Calling 'BadIterable::end'}}
 }
+
+int *test_idx_sym(int I) {
+  static int arr[10]; // expected-note{{Original object declared here}} expected-note{{'arr' initialized here}}
+
+  if (I != 11) // expected-note{{Assuming 'I' is equal to 11}}
+               // expected-note@-1{{Taking false branch}}
+    return arr;
+  return arr + I; // expected-warning{{Returned pointer value points outside the original object}}
+                  // expected-note@-1{{Returned pointer value points outside the original object}}
+                  // expected-note@-2{{Original object 'arr' is an array of 10 'int' objects, returned pointer points at index 11}}
+}
+
+namespace test_array_of_struct {
+
+struct Data {
+  int A;
+  char *B;
+};
+
+Data DataArr[10]; // expected-note{{Original object declared here}}
+
+Data *test_struct_array() {
+  int I = conjure_index();
+  if (I != 11) // expected-note{{Assuming 'I' is equal to 11}}
+               // expected-note@-1{{Taking false branch}}
+    return DataArr;
+  return DataArr + I; // expected-warning{{Returned pointer value points outside the original object}}
+                      // expected-note@-1{{Returned pointer value points outside the original object}}
+                      // expected-note@-2{{Original object 'DataArr' is an array of 10 'test_array_of_struct::Data' objects, returned pointer points at index 11}}
+}
+
+}
+