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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
