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

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

steakhal added a comment.

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 at -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 at -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.



================
Comment at: clang/test/Analysis/return-ptr-range.cpp:11
+int *test_global_ptr() {
+  do { // expected-note{{Loop condition is false.  Exiting loop}}
     int x = conjure_index();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107051