[analyzer] Fix core.VLASize checker false positive taint reports

[dkrupp]

The checker reported a false positive on this code

void testTaintedSanitizedVLASize(void) {
int x;
scanf("%d", &x);
if (x<1)
return;
int vla[x]; // no-warning
}

After the fix, the checker only emits tainted warning if the vla size is coming from a tainted source and it cannot prove that it is positive.

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Changes

The checker reported a false positive on this code

void testTaintedSanitizedVLASize(void) {
int x;
scanf("%d", &x);
if (x<1)
return;
int vla[x]; // no-warning
}

After the fix, the checker only emits tainted warning if the vla size is coming from a tainted source and it cannot prove that it is positive.

---

Full diff: https://github.com/llvm/llvm-project/pull/68140.diff

3 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp (+8-8)
• (modified) clang/test/Analysis/taint-diagnostic-visitor.c (+2-2)
• (modified) clang/test/Analysis/taint-generic.c (+10-1)

diff --git a/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
index b195d912cadfe9b..46b5f5e10f0e65c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
@@ -162,12 +162,6 @@ ProgramStateRef VLASizeChecker::checkVLAIndexSize(CheckerContext &C,
   if (SizeV.isUnknown())
     return nullptr;
 
-  // Check if the size is tainted.
-  if (isTainted(State, SizeV)) {
-    reportTaintBug(SizeE, State, C, SizeV);
-    return nullptr;
-  }
-
   // Check if the size is zero.
   DefinedSVal SizeD = SizeV.castAs<DefinedSVal>();
 
@@ -189,10 +183,10 @@ ProgramStateRef VLASizeChecker::checkVLAIndexSize(CheckerContext &C,
   DefinedOrUnknownSVal Zero = SVB.makeZeroVal(SizeTy);
 
   SVal LessThanZeroVal = SVB.evalBinOp(State, BO_LT, SizeD, Zero, SizeTy);
+  ProgramStateRef StatePos, StateNeg;
   if (std::optional<DefinedSVal> LessThanZeroDVal =
           LessThanZeroVal.getAs<DefinedSVal>()) {
     ConstraintManager &CM = C.getConstraintManager();
-    ProgramStateRef StatePos, StateNeg;
 
     std::tie(StateNeg, StatePos) = CM.assumeDual(State, *LessThanZeroDVal);
     if (StateNeg && !StatePos) {
@@ -202,6 +196,12 @@ ProgramStateRef VLASizeChecker::checkVLAIndexSize(CheckerContext &C,
     State = StatePos;
   }
 
+  // Check if the size is tainted.
+  if ((StateNeg || StateZero) && isTainted(State, SizeV)) {
+    reportTaintBug(SizeE, State, C, SizeV);
+    return nullptr;
+  }
+
   return State;
 }
 
@@ -220,7 +220,7 @@ void VLASizeChecker::reportTaintBug(const Expr *SizeE, ProgramStateRef State,
   SmallString<256> buf;
   llvm::raw_svector_ostream os(buf);
   os << "Declared variable-length array (VLA) ";
-  os << "has tainted size";
+  os << "has a tainted (attacker controlled) size, that can be 0 or negative";
 
   auto report = std::make_unique<PathSensitiveBugReport>(*TaintBT, os.str(), N);
   report->addRange(SizeE->getSourceRange());
diff --git a/clang/test/Analysis/taint-diagnostic-visitor.c b/clang/test/Analysis/taint-diagnostic-visitor.c
index 8a7510177f3e444..45369785ed6924e 100644
--- a/clang/test/Analysis/taint-diagnostic-visitor.c
+++ b/clang/test/Analysis/taint-diagnostic-visitor.c
@@ -46,8 +46,8 @@ void taintDiagnosticVLA(void) {
   scanf("%d", &x); // expected-note {{Value assigned to 'x'}}
                    // expected-note@-1 {{Taint originated here}}
                    // expected-note@-2 {{Taint propagated to the 2nd argument}}
-  int vla[x]; // expected-warning {{Declared variable-length array (VLA) has tainted size}}
-              // expected-note@-1 {{Declared variable-length array (VLA) has tainted size}}
+  int vla[x]; // expected-warning {{Declared variable-length array (VLA) has a tainted}}
+              // expected-note@-1 {{Declared variable-length array (VLA) has a tainted}}
 }
 
 
diff --git a/clang/test/Analysis/taint-generic.c b/clang/test/Analysis/taint-generic.c
index c6a01594f15abb7..ae2ae5b23aab3c6 100644
--- a/clang/test/Analysis/taint-generic.c
+++ b/clang/test/Analysis/taint-generic.c
@@ -405,7 +405,16 @@ int testDivByZero(void) {
 void testTaintedVLASize(void) {
   int x;
   scanf("%d", &x);
-  int vla[x]; // expected-warning{{Declared variable-length array (VLA) has tainted size}}
+  int vla[x]; // expected-warning{{Declared variable-length array (VLA) has a tainted (attacker controlled) size, that can be 0 or negative}}
+}
+
+// Tainted-sanitized VLAs.
+void testTaintedSanitizedVLASize(void) {
+  int x;
+  scanf("%d", &x);
+  if (x<1)
+    return;
+  int vla[x]; // no-warning
 }
 
 int testTaintedAllocaMem() {

[NagyDonat]

This change is a good step forward and cleanly implemented. Could you show some results from open source projects?

As a side remark, I'd like to mention that the separate handling of the "size is zero" and the "size is negative" cases is logically incorrect in the old code, e.g. when the size may be either 0 or negative, the checker will claim that it's negative (because in the first step it sees that may be nonzero, and so assumes that it's nonzero). This issue should be cleaned up as a separate improvement

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[dkrupp]

I executed the analysis with this patch on the following open source projects: memcached,tmux,curl,twin,vim,openssl,sqlite,ffmpeg,postgres, xerces

And it did not bring any visible change in the reports. So there were no new or resolved findings compared to the baseline.
In both the baseline and the new analysis execution the alpha.security.taint.TaintPropagation and the core.VLASize checkers were enabled.

Link to the diff:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&run=%2avla_taint_baseline&is-unique=off&newcheck=%2avla_taint_new&diff-type=New&checker-msg=%2ataint%2a

[NagyDonat]

LGTM in general, with some minor nitpicks.

[NagyDonat]

Suggested change

	os << "has a tainted (attacker controlled) size that can be 0 or negative";
	os << "has tainted (attacker controlled) size that can be 0 or negative";

I feel that the indefinite article "sounds strange" in this message, because the size of an array is a specific unique value. Another alternative would be "The size of the variable-length array (VLA) is a tainted value that can be 0 or negative" or something similar.

Of course the tests need to be updated if you change this message.

[NagyDonat]

At this point StateNeg may be default-initialized; but I assume that it doesn't cause any problems.

[NagyDonat]

Suggested change

	int vla[x]; // no-warning. The analyzer can prove that the x can only be positive.
	int vla[x]; // no-warning. The analyzer can prove that x must be positive.

the x was very strange.