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

[balazske]

Summary

Add some notes and track of bad return value.

[steakhal]

Aside from that Returned pointer value points outside the original object with size of 10 'int' objects reads somewhat unnatural I have no objections.
A couple of nits here and there but overall I'm pleased to see more verbose bug reports.

clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp

82–83

85–86

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

0–1

Is RUN1 intentional? If so, what does it do?

1

You can specify multiple match prefixes.
If you were passing -verify=expected,notes, you would not have to duplicate each warning.

1

This is not used in every test case. I would recommend moving this to the function parameter where it's actually being used.

4–6

Only a single array should be enough.
You can specify the match count for the diagnostic verifier.

// notes-note 3 {{Memory object...}} Will succeed only if exactly 3 times matched.

18–19

This is probably more of a taste.
I would prefer fewer indentations.
The same applies everywhere.

18–21

I think it's fine to leave the common suffix (potential ... out of the matched string. They give only a little value.
The same applies everywhere, except keeping a single one maybe.

[Szelethus]

One of the test files needs fixing: https://reviews.llvm.org/harbormaster/unit/view/931615/

Love the patch, though I agree that the note message needs some improvement. How about we change from this:

warning: Returned pointer value with index 11 points outside the original object with size of 10 'Data' objects (potential buffer overflow)

To this:

warning: Returned pointer points outside the original object (potential buffer overflow)
note: The original object 'Data' is an int array of size 10, the returned pointer points at the 11th element

or this:

warning: Returned pointer points to the 11th element of 'Data', but that is an int array of size 10 (potential buffer overflow)

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

0–1

We could just delete it. I guess that was the intent, to make this RUN line non-functional.

1

Don't forget core!

1

If this is a cpp file, just use namespaces, and redeclare everything inside them, like this: https://github.com/llvm/llvm-project/blob/main/clang/test/Analysis/track-control-dependency-conditions.cpp

18–19

I disagree, and prefer it as it is written now.

[balazske]

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

0–1

I wanted to make two runs, one for warnings only and one for notes only. But could not find out how to disable the warning messages and show only notes. Because the same warnings appear anyway it should be enough to use only one run with text output and remove the custom prefix.

18–19

The current style will be used, the comments are better grouped.

[balazske]

Changed way of generating notes.
Re-structured the tests.
Other small changes.

[steakhal]

First and foremost, I think this is a great change. I think the diagnostic is on the point.
BTW it seems like you missed two notes in misc-ps-region-store.m causing a test fail:

error: 'note' diagnostics seen but not expected:
  File /home/steakhal/git/llvm-project/clang/test/Analysis/misc-ps-region-store.m Line 464: Original object declared here
  File /home/steakhal/git/llvm-project/clang/test/Analysis/misc-ps-region-store.m Line 467: Original object 'test_cwe466_return_outofbounds_pointer_a' is an array of 10 'int' objects, returned pointer points at index 11
2 errors generated.

---

The following comments are not about your actual change, I'm rather pointing out further possibilities for improving the checker.

The checker seems to cover cases when we see the array, fine.
What about pointers/references to arrays? IMO we should trust those declarations about the size of the pointee.

using size_t = decltype(sizeof 1);
using int10 = int[10];

template <class T> void clang_analyzer_dump(T);
size_t clang_analyzer_getExtent(void *);
int conjure_index();


int *test_ptr_to_array(int10 *arr) {
  int x = conjure_index();
  if (x != 20)
    return *arr; // no-warning

  clang_analyzer_dump(clang_analyzer_getExtent(*arr));
  // expected-warning-re@-1 {{extent_${{[0-9]+}}{SymRegion{reg_${{[0-9]+}}<int10 * arr>}}}}
  // FIXME: Above should be '40 S64b' on Linux x86_64.

  int *result = *arr + x;
  clang_analyzer_dump(result);
  // expected-warning-re@-1 {{&Element{SymRegion{reg_${{[0-9]+}}<int10 * arr>},20 S64b,int}}}
  return result; // We should expect a warning here.
}

Please add this test to your patch.

It seems like the checker would catch it if MemRegionManager::getStaticSize() would consider the type of the SymbolicRegion for ConstantArray pointee types. I'll have a patch about that.

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

11

I would rather use a simple block {...} for opening a scope, but I don't know why you don't declare ptr in the original scope in the first place.
People usually use do {} while(0) constructs if they want to use break somewhere ~~ like a goto OR they implement a macro. You are doing none of these.

[steakhal]

In D107051#2928089, @steakhal wrote:

First and foremost, I think this is a great change. I think the diagnostic is on the point.
BTW it seems like you missed two notes in misc-ps-region-store.m causing a test fail:

error: 'note' diagnostics seen but not expected:
  File /home/steakhal/git/llvm-project/clang/test/Analysis/misc-ps-region-store.m Line 464: Original object declared here
  File /home/steakhal/git/llvm-project/clang/test/Analysis/misc-ps-region-store.m Line 467: Original object 'test_cwe466_return_outofbounds_pointer_a' is an array of 10 'int' objects, returned pointer points at index 11
2 errors generated.

---

The following comments are not about your actual change, I'm rather pointing out further possibilities for improving the checker.

The checker seems to cover cases when we see the array, fine.
What about pointers/references to arrays? IMO we should trust those declarations about the size of the pointee.

using size_t = decltype(sizeof 1);
using int10 = int[10];

template <class T> void clang_analyzer_dump(T);
size_t clang_analyzer_getExtent(void *);
int conjure_index();


int *test_ptr_to_array(int10 *arr) {
  int x = conjure_index();
  if (x != 20)
    return *arr; // no-warning

  clang_analyzer_dump(clang_analyzer_getExtent(*arr));
  // expected-warning-re@-1 {{extent_${{[0-9]+}}{SymRegion{reg_${{[0-9]+}}<int10 * arr>}}}}
  // FIXME: Above should be '40 S64b' on Linux x86_64.

  int *result = *arr + x;
  clang_analyzer_dump(result);
  // expected-warning-re@-1 {{&Element{SymRegion{reg_${{[0-9]+}}<int10 * arr>},20 S64b,int}}}
  return result; // We should expect a warning here.
}

Please add this test to your patch.

It seems like the checker would catch it if MemRegionManager::getStaticSize() would consider the type of the SymbolicRegion for ConstantArray pointee types. I'll have a patch about that.

It seems like we cannot implement this in a way fitting into the current memory model. To be able to do that we should be able to differentiate these two cases:

void test(int10 *arr) {
  clang_analyzer_dump((int*)(arr + 2)); // allow: `arr` might be a pointer to an array of `int10[100]`, in which case the resulting pointer is safe to use
  clang_analyzer_dump(*arr + 20);       // disallow: `*arr` :: `int[10]`, creating a pointer to the 20th element
  // Both dumps are the same:  &Element{SymRegion{reg_$0<int10 * arr>},20 S64b,int}
}

So, I'm actually puzzled about this. I think you can leave out the extra test case I proposed.

[balazske]

If the original memory object is not known the static size is not known too. Every pointer with unknown source can point into a bigger data structure.

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

11

I do not know why these loops are here but did not change the original code. Should we change it to simple block?

[steakhal]

In D107051#2928536, @balazske wrote:

If the original memory object is not known the static size is not known too. Every pointer with unknown source can point into a bigger data structure.

You are right, but IMO pointers to arrays are so rare that we could probably trust them. At least, that was my idea.

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

11

Yes, please. The note for the loop is only noise in its current form.

[balazske]

Fixes in tests.

[steakhal]

Aside from the inline nit, I think it's good to go.
Let some time for the others to catch up, they might have objections.

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

18–21

I don't think we need this extra scope. Same for the others.

[balazske]

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

18–21

The original test was used to make the x "dead" at return after the loop (at least without the fix), see D12726. In the case of loop the x is garbage-collected at end of the loop, if a block is used only at the end of the function (?) but not at end of the block. So I want to put back the loop to preserve the original code.

[balazske]

Re-add empty while loops in tests, to have the original code.
These loops were intentionally created and probably still needed.

[steakhal]

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

18–21

Thank you for letting me know.

clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp

 //== ReturnPointerRangeChecker.cpp ------------------------------*- C++ -*--==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines ReturnPointerRangeChecker, which is a path-sensitive check
 // which looks for an out-of-bound pointer being returned to callers.
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 
 using namespace clang;
if (StOutBound && !StInBound) {
 
     // FIXME: This bug correspond to CWE-466.  Eventually we should have bug
     // types explicitly reference such exploit categories (when applicable).
     if (!BT)
       BT.reset(new BuiltinBug(
           this, "Buffer overflow",
           "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());
+
+    const auto ConcreteElementCount = ElementCount.getAs<nonloc::ConcreteInt>();
+    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);
 
-    report->addRange(RetE->getSourceRange());
-    C.emitReport(std::move(report));
+    C.emitReport(std::move(Report));
   }
 }
 
 void ento::registerReturnPointerRangeChecker(CheckerManager &mgr) {
   mgr.registerChecker<ReturnPointerRangeChecker>();
 }
 
 bool ento::shouldRegisterReturnPointerRangeChecker(const CheckerManager &mgr) {
   return true;
 }

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

void element_region_with_symbolic_superregion(int* p) {
   if (p[0] == 1)
     (void)*x; // no-warning
 }
 
 //===----------------------------------------------------------------------===//
 // 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}}
 }
 
 //===----------------------------------------------------------------------===//
 // PR 3135 - Test case that shows that a variable may get invalidated when its
 // address is included in a structure that is passed-by-value to an unknown function.
 //===----------------------------------------------------------------------===//
 
 typedef struct { int *a; } pr3135_structure;

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

-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange -verify %s
-
-int arr[10];
-int *ptr;
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.security.ReturnPtrRange -analyzer-output text -verify %s
 
 int conjure_index();
 
-int *test_element_index_lifetime() {
-  do {
+namespace test_element_index_lifetime {
+
+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;
-    if (x != 20)
+    clang_analyzer_warnOnDeadSymbol(x);
+    ptr = arr + x; // expected-note{{Value assigned to 'ptr'}}
+    if (x != 20) // expected-note{{Assuming 'x' is equal to 20}}
+                 // expected-note@-1{{Taking false branch}}
       return arr; // no-warning
-  } while (0);
-  return ptr; // expected-warning{{Returned pointer value points outside the original object (potential buffer overflow)}}
+  } while(0);
+  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;
-    if (x != 20)
+    local_ptr = arr + x; // expected-note{{Value assigned to 'local_ptr'}}
+    if (x != 20) // expected-note{{Assuming 'x' is equal to 20}}
+                 // expected-note@-1{{Taking false branch}}
       return arr; // no-warning
-  } while (0);
-  return local_ptr; // expected-warning{{Returned pointer value points outside the original object (potential buffer overflow)}}
+  } while(0);
+  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>
 T* end(T (&arr)[N]) {
   return arr + N; // no-warning, because we want to avoid false positives on returning the end() iterator of a container.
 }
 
 void get_end_of_array() {

 
 void use_iterable_object() {
   Iterable<20> iter;
   iter.end();
 }
 
 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;
-  iter.end();
+  BadIterable<20> iter; // expected-note{{Calling default constructor for 'BadIterable<20>'}}
+                        // 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}}
+}
+
+}
+