[PATCH] D159107: [analyzer] ArrayBoundCheckerV2 should disallow forming lvalues to out-of-bounds locations

Thu Aug 31 01:09:34 PDT 2023

steakhal abandoned this revision.
steakhal added a comment.

In D159107#4627903 <https://reviews.llvm.org/D159107#4627903>, @donat.nagy wrote:

> Good direction of development, this will be useful for providing better bug reports (in addition to ensuring correct behavior some situations). 
> Note that it's also possible to dereference pointers with the operator `->`, which is represented by `MemberExpr`s in the AST; we should probably handle that as if it was a `UO_Deref`.

+1, +1

> There is also a small corner case that for an array `some_type arr[N]` it's well-defined to form the past-the-end pointer as `&arr[N]` (instead of `arr + N`) -- while any other use of `arr[N]` is undefined behavior. If this occurs in practice, then we'll probably need some additional logic to handle it. (Note that the `check::Location` implementation dodged this question, because it didn't report anything when the program formed `&arr[N]`, but later created a bug report when this pointer value was dereferenced.)

Ah, this disappoints me. You are right. This delayed mechanism definitely needs a more careful approach.

Given these concerns and the quality of the diagnostics, I don't think it's something easy to fix. I'll abandon this for now, to focus on the low-hanging patches to upstream.

Thanks for the thoughtful review! @donat.nagy

================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:34
+class ArrayBoundCheckerV2
+    : public Checker<check::Bind, check::PostStmt<ArraySubscriptExpr>,
+                     check::PostStmt<UnaryOperator>> {
----------------
donat.nagy wrote:
> Which testcase would break without the `check::Bind` callback? (Not action needed, I'm just curious.)
Good question. TO my surprise, no tests would be broken if we removed this callback.
Nice catch.
I thought it would break the new test added in D159106, but it would still pass.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159107