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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

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.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp:83-84
 
-    // 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.
+    auto ConcreteElementCount = ElementCount.getAs<nonloc::ConcreteInt>();
+    auto ConcreteIdx = Idx.getAs<nonloc::ConcreteInt>();
+
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp:102
     // Generate a report for this bug.
-    auto report =
-        std::make_unique<PathSensitiveBugReport>(*BT, BT->getDescription(), N);
-
+    auto report = std::make_unique<PathSensitiveBugReport>(*BT, SBuf, N);
     report->addRange(RetE->getSourceRange());
----------------



================
Comment at: clang/test/Analysis/return-ptr-range.cpp:1
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange -verify %s
+// RUN1: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange -analyzer-output text -verify=notes %s
----------------
Is `RUN1` intentional? If so, what does it do?


================
Comment at: clang/test/Analysis/return-ptr-range.cpp:2
+// RUN1: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.ReturnPtrRange -analyzer-output text -verify=notes %s
 
----------------
You can specify multiple match prefixes.
If you were passing `-verify=expected,notes`, you would not have to duplicate each warning.


================
Comment at: clang/test/Analysis/return-ptr-range.cpp:8-10
+int arr1[10]; // notes-note{{Memory object declared here}}
+int arr2[10]; // notes-note{{Memory object declared here}}
+int arr3[10]; // notes-note{{Memory object declared here}}
----------------
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.


================
Comment at: clang/test/Analysis/return-ptr-range.cpp:11
+int arr3[10]; // notes-note{{Memory object declared here}}
 int *ptr;
 
----------------
This is not used in every test case. I would recommend moving this to the function parameter where it's actually being used.


================
Comment at: clang/test/Analysis/return-ptr-range.cpp:19-20
+    ptr = arr1 + x; // notes-note{{Value assigned to 'ptr'}}
+    if (x != 20) // notes-note{{Assuming 'x' is equal to 20}}
+                 // notes-note at -1{{Taking false branch}}
+      return arr1; // no-warning
----------------
This is probably more of a taste.
I would prefer fewer indentations.
The same applies everywhere.


================
Comment at: clang/test/Analysis/return-ptr-range.cpp:23
   } 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 with size of 10 'int' objects (potential buffer overflow) [alpha.security.ReturnPtrRange]}}
+              // notes-warning at -1{{Returned pointer value points outside the original object with size of 10 'int' objects (potential buffer overflow) [alpha.security.ReturnPtrRange]}}
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107051/new/

https://reviews.llvm.org/D107051