[PATCH] D125524: [BoundV2] ArrayBoundV2 checks if the extent is tainted

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

steakhal added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:208
     if (state_exceedsUpperBound && state_withinUpperBound) {
-      SVal ByteOffset = rawOffset.getByteOffset();
-      if (isTainted(state, ByteOffset)) {
+      if (isTainted(state, *upperboundToCheck)) {
         reportOOB(checkerContext, state_exceedsUpperBound, OOB_Tainted,
----------------
martong wrote:
> Could you please explain why we change `rawOffset` to `*upperBoundToCheck`? And perhaps the same explanation could infiltrate into the checker's code itself as a comment to `upperbound`.
In the test attached you can see that the extent is tainted, not the offset.
Thus checking the offset for taint won't suffice.
The bug condition should depend on the calculation itself, which is basically what is done here.


================
Comment at: clang/test/Analysis/taint-diagnostic-visitor.c:46-48
+  int *p = (int *)malloc(x + conj); // Generic taint checker forbids tainted allocation.
+  // expected-warning at -1 {{Untrusted data is used to specify the buffer size}}
+  // expected-note at -2    {{Untrusted data is used to specify the buffer size}}
----------------
martong wrote:
> Could we get rid of the seemingly unrelated malloc taint report by using an array on the stack?
No, we need the extent to be tainted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125524