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

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

steakhal added a comment.

In D107051#2928089 <https://reviews.llvm.org/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 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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107051