[PATCH] D159105: [analyzer] ArrayBoundCheckerV2 should check the region for taint as well

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

steakhal added a comment.

I'm thinking of a heuristic like this:

  bool IsAtOffsetZero = [ByteOffset] {
    const auto *Int = ByteOffset.getAsInteger();
    return Int && Int->isZero();
  }();
  // ...
  if (state_precedesLowerBound && state_withinLowerBound && !IsAtOffsetZero &&
      isTainted(state, location)) {
    reportTaintOOB(checkerContext, state_precedesLowerBound, location);
    return;
  }

It will definitely not work for all cases but should filter out the most annoying ones.

  int testTaintedPtr() {
    // Read a pointer from a tainted source and dereference it.
    char *app = getenv("APP_NAME");
    if (!app)
      return -1;
    if (app[0] == '\0') return 0; // no-warning: Allow manual checking for null terminator at offset zero.
    if (app[1] == '\0') return 1; // expected-warning {{Out of bound memory access (index is tainted)}} Any other index than 0 is disallowed.
    if (app[2] == '\0') return 2; // no-warning: The path already sunk.
    return -1;
  }



================
Comment at: clang/test/Analysis/taint-generic.c:1133-1141
+void testTaintedLowerConstrainedIndex() {
+  int n;
+  scanf("%d", &n);
+  if (n >= 0) {
+    // We only constained the lower end, and it's tainted => report.
+    Buffer[n] = 1; // expected-warning {{Out of bound memory access (index is tainted)}}
+  }
----------------
donat.nagy wrote:
> Also add the "opposite" of this where you test `if (n < BUFSIZE)`.
Makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159105