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

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

I was thinking about adding an improvement like this for the sake of consistency, but I fear that this might cause a surprising amount of false positives. (Did you test it on larger codebases?)

As far as I understand (correct me if I'm wrong), there are situations when TaintChecker marks the memory region of e.g. a returned string as tainted to mark that the //contents// of the region are unreliable. (There may be other more exotic situations when even the //pointer value// or the //extent// becomes unreliable e.g. your testcases where you `scanf` into a pointer.)

I fear that the overzealous handling of tainted data would produce too many false positives in situations when code indexes strings that contain user input and the SA engine cannot understand the logic that calculates the indices. For example if I understand it correctly a function like

  char f(int n) {
    char *var = getenv("FOO");
    return var[n];
  }

would be reported as an out of bounds memory access, because the region is tainted and the index value is not known. This is especially problematic on the underflow side (which is introduced by your commit), because I'd assume that it's common to have numeric values (e.g. function arguments) where the programmer knows that they're nonnegative, but the analyzer cannot deduce this.

Also note that the error message "Out of bound memory access (index is tainted)" becomes misleading after this patch -- but don't spend too much time on resolving this, because right now I'm working on a complete rewrite the message generation to replace the current spartan messages with useful error reporting.



================
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)}}
+  }
----------------
Also add the "opposite" of this where you test `if (n < BUFSIZE)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159105