From c9bd9e93033478bc99a8602b248136c0b6741b1c Mon Sep 17 00:00:00 2001
From: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Date: Mon, 7 Oct 2024 16:47:43 +0100
Subject: [PATCH 01/13] C++: Modernize the 'cpp/unclear-array-index-validation'
 query by getting rid of the DefaultTaintTracking barriers and replacing them
 with a 'BarrierGuard' instantiation.

---
 .../CWE-129/ImproperArrayIndexValidation.ql   | 100 ++++--------------
 1 file changed, 21 insertions(+), 79 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql b/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql
index 107be7bddfde..0c5802a4c564 100644
--- a/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql
+++ b/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql
@@ -14,102 +14,44 @@
 
 import cpp
 import semmle.code.cpp.controlflow.IRGuards
-import semmle.code.cpp.security.FlowSources
-import semmle.code.cpp.ir.dataflow.TaintTracking
+import semmle.code.cpp.security.FlowSources as FS
+import semmle.code.cpp.dataflow.new.TaintTracking
 import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
 import ImproperArrayIndexValidation::PathGraph
-import semmle.code.cpp.security.Security
 
-predicate hasUpperBound(VariableAccess offsetExpr) {
-  exists(BasicBlock controlled, StackVariable offsetVar, SsaDefinition def |
-    controlled.contains(offsetExpr) and
-    linearBoundControls(controlled, def, offsetVar) and
-    offsetExpr = def.getAUse(offsetVar)
-  )
-}
-
-pragma[noinline]
-predicate linearBoundControls(BasicBlock controlled, SsaDefinition def, StackVariable offsetVar) {
-  exists(GuardCondition guard, boolean branch |
-    guard.controls(controlled, branch) and
-    cmpWithLinearBound(guard, def.getAUse(offsetVar), Lesser(), branch)
-  )
-}
-
-predicate readsVariable(LoadInstruction load, Variable var) {
-  load.getSourceAddress().(VariableAddressInstruction).getAstVariable() = var
+predicate isFlowSource(FS::FlowSource source, string sourceType) {
+  sourceType = source.getSourceType()
 }
 
-predicate hasUpperBoundsCheck(Variable var) {
-  exists(RelationalOperation oper, VariableAccess access |
-    oper.getAnOperand() = access and
-    access.getTarget() = var and
-    // Comparing to 0 is not an upper bound check
-    not oper.getAnOperand().getValue() = "0"
+predicate guardChecks(IRGuardCondition g, Expr e, boolean branch) {
+  exists(Operand op | op.getDef().getConvertedResultExpression() = e |
+    // op < k
+    g.comparesLt(op, _, true, any(BooleanValue bv | bv.getValue() = branch))
+    or
+    // op < _ + k
+    g.comparesLt(op, _, _, true, branch)
+    or
+    // op == k
+    g.comparesEq(op, _, true, any(BooleanValue bv | bv.getValue() = branch))
+    or
+    // op == _ + k
+    g.comparesEq(op, _, _, true, branch)
   )
 }
 
-predicate nodeIsBarrierEqualityCandidate(DataFlow::Node node, Operand access, Variable checkedVar) {
-  readsVariable(node.asInstruction(), checkedVar) and
-  any(IRGuardCondition guard).ensuresEq(access, _, _, node.asInstruction().getBlock(), true)
-}
-
-predicate isFlowSource(FlowSource source, string sourceType) { sourceType = source.getSourceType() }
-
-predicate predictableInstruction(Instruction instr) {
-  instr instanceof ConstantInstruction
-  or
-  instr instanceof StringConstantInstruction
-  or
-  // This could be a conversion on a string literal
-  predictableInstruction(instr.(UnaryInstruction).getUnary())
-}
-
 module ImproperArrayIndexValidationConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { isFlowSource(source, _) }
 
   predicate isBarrier(DataFlow::Node node) {
-    hasUpperBound(node.asExpr())
-    or
-    // These barriers are ported from `DefaultTaintTracking` because this query is quite noisy
-    // otherwise.
-    exists(Variable checkedVar |
-      readsVariable(node.asInstruction(), checkedVar) and
-      hasUpperBoundsCheck(checkedVar)
-    )
-    or
-    exists(Variable checkedVar, Operand access |
-      readsVariable(access.getDef(), checkedVar) and
-      nodeIsBarrierEqualityCandidate(node, access, checkedVar)
-    )
-    or
-    // Don't use dataflow into binary instructions if both operands are unpredictable
-    exists(BinaryInstruction iTo |
-      iTo = node.asInstruction() and
-      not predictableInstruction(iTo.getLeft()) and
-      not predictableInstruction(iTo.getRight()) and
-      // propagate taint from either the pointer or the offset, regardless of predictability
-      not iTo instanceof PointerArithmeticInstruction
-    )
-    or
-    // don't use dataflow through calls to pure functions if two or more operands
-    // are unpredictable
-    exists(Instruction iFrom1, Instruction iFrom2, CallInstruction iTo |
-      iTo = node.asInstruction() and
-      isPureFunction(iTo.getStaticCallTarget().getName()) and
-      iFrom1 = iTo.getAnArgument() and
-      iFrom2 = iTo.getAnArgument() and
-      not predictableInstruction(iFrom1) and
-      not predictableInstruction(iFrom2) and
-      iFrom1 != iFrom2
-    )
+    node = DataFlow::BarrierGuard<guardChecks/3>::getABarrierNode()
   }
 
+  predicate isBarrierOut(DataFlow::Node node) { isSink(node) }
+
   predicate isSink(DataFlow::Node sink) {
     exists(ArrayExpr arrayExpr, VariableAccess offsetExpr |
       offsetExpr = arrayExpr.getArrayOffset() and
-      sink.asExpr() = offsetExpr and
-      not hasUpperBound(offsetExpr)
+      sink.asExpr() = offsetExpr
     )
   }
 }

From f6f5f5d4b4212ca38e81c2fc04c6fb675a5bc7a7 Mon Sep 17 00:00:00 2001
From: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Date: Mon, 7 Oct 2024 16:49:34 +0100
Subject: [PATCH 02/13] C++: Accept test changes.

---
 .../ImproperArrayIndexValidation.expected                  | 7 +++++++
 .../CWE-129/semmle/ImproperArrayIndexValidation/test1.c    | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
index d6ffc7aed578..3692ec4509df 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
@@ -2,12 +2,15 @@ edges
 | test1.c:7:26:7:29 | **argv | test1.c:8:11:8:14 | call to atoi | provenance | TaintFunction |
 | test1.c:8:11:8:14 | call to atoi | test1.c:9:9:9:9 | i | provenance |  |
 | test1.c:8:11:8:14 | call to atoi | test1.c:11:9:11:9 | i | provenance |  |
+| test1.c:8:11:8:14 | call to atoi | test1.c:12:9:12:9 | i | provenance |  |
 | test1.c:8:11:8:14 | call to atoi | test1.c:13:9:13:9 | i | provenance |  |
 | test1.c:9:9:9:9 | i | test1.c:16:16:16:16 | i | provenance |  |
 | test1.c:11:9:11:9 | i | test1.c:32:16:32:16 | i | provenance |  |
+| test1.c:12:9:12:9 | i | test1.c:40:16:40:16 | i | provenance |  |
 | test1.c:13:9:13:9 | i | test1.c:48:16:48:16 | i | provenance |  |
 | test1.c:16:16:16:16 | i | test1.c:18:16:18:16 | i | provenance |  |
 | test1.c:32:16:32:16 | i | test1.c:33:11:33:11 | i | provenance |  |
+| test1.c:40:16:40:16 | i | test1.c:41:11:41:11 | i | provenance |  |
 | test1.c:48:16:48:16 | i | test1.c:51:3:51:7 | ... = ... | provenance |  |
 | test1.c:51:3:51:7 | ... = ... | test1.c:53:15:53:15 | j | provenance |  |
 nodes
@@ -15,11 +18,14 @@ nodes
 | test1.c:8:11:8:14 | call to atoi | semmle.label | call to atoi |
 | test1.c:9:9:9:9 | i | semmle.label | i |
 | test1.c:11:9:11:9 | i | semmle.label | i |
+| test1.c:12:9:12:9 | i | semmle.label | i |
 | test1.c:13:9:13:9 | i | semmle.label | i |
 | test1.c:16:16:16:16 | i | semmle.label | i |
 | test1.c:18:16:18:16 | i | semmle.label | i |
 | test1.c:32:16:32:16 | i | semmle.label | i |
 | test1.c:33:11:33:11 | i | semmle.label | i |
+| test1.c:40:16:40:16 | i | semmle.label | i |
+| test1.c:41:11:41:11 | i | semmle.label | i |
 | test1.c:48:16:48:16 | i | semmle.label | i |
 | test1.c:51:3:51:7 | ... = ... | semmle.label | ... = ... |
 | test1.c:53:15:53:15 | j | semmle.label | j |
@@ -27,4 +33,5 @@ subpaths
 #select
 | test1.c:18:16:18:16 | i | test1.c:7:26:7:29 | **argv | test1.c:18:16:18:16 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:33:11:33:11 | i | test1.c:7:26:7:29 | **argv | test1.c:33:11:33:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
+| test1.c:41:11:41:11 | i | test1.c:7:26:7:29 | **argv | test1.c:41:11:41:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:53:15:53:15 | j | test1.c:7:26:7:29 | **argv | test1.c:53:15:53:15 | j | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
index 08484aef51fc..926b81dad7c3 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
@@ -38,7 +38,7 @@ void test3(int i) {
 }
 
 void test4(int i) {
-  myArray[i] = 0; // BAD: i has not been validated [NOT REPORTED]
+  myArray[i] = 0; // BAD: i has not been validated
 
   if ((i < 0) || (i >= 10)) return;
 

From afb5e4f8417c01760f0ac309624b912c9f8ab892 Mon Sep 17 00:00:00 2001
From: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Date: Mon, 7 Oct 2024 16:57:48 +0100
Subject: [PATCH 03/13] C++: Add test spacing.

---
 .../ImproperArrayIndexValidation.expected     | 44 +++++++++----------
 .../ImproperArrayIndexValidation/test1.c      |  5 +++
 2 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
index 3692ec4509df..6f9ecd1d85d7 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
@@ -4,15 +4,15 @@ edges
 | test1.c:8:11:8:14 | call to atoi | test1.c:11:9:11:9 | i | provenance |  |
 | test1.c:8:11:8:14 | call to atoi | test1.c:12:9:12:9 | i | provenance |  |
 | test1.c:8:11:8:14 | call to atoi | test1.c:13:9:13:9 | i | provenance |  |
-| test1.c:9:9:9:9 | i | test1.c:16:16:16:16 | i | provenance |  |
-| test1.c:11:9:11:9 | i | test1.c:32:16:32:16 | i | provenance |  |
-| test1.c:12:9:12:9 | i | test1.c:40:16:40:16 | i | provenance |  |
-| test1.c:13:9:13:9 | i | test1.c:48:16:48:16 | i | provenance |  |
-| test1.c:16:16:16:16 | i | test1.c:18:16:18:16 | i | provenance |  |
-| test1.c:32:16:32:16 | i | test1.c:33:11:33:11 | i | provenance |  |
-| test1.c:40:16:40:16 | i | test1.c:41:11:41:11 | i | provenance |  |
-| test1.c:48:16:48:16 | i | test1.c:51:3:51:7 | ... = ... | provenance |  |
-| test1.c:51:3:51:7 | ... = ... | test1.c:53:15:53:15 | j | provenance |  |
+| test1.c:9:9:9:9 | i | test1.c:17:16:17:16 | i | provenance |  |
+| test1.c:11:9:11:9 | i | test1.c:33:16:33:16 | i | provenance |  |
+| test1.c:12:9:12:9 | i | test1.c:41:16:41:16 | i | provenance |  |
+| test1.c:13:9:13:9 | i | test1.c:49:16:49:16 | i | provenance |  |
+| test1.c:17:16:17:16 | i | test1.c:19:16:19:16 | i | provenance |  |
+| test1.c:33:16:33:16 | i | test1.c:34:11:34:11 | i | provenance |  |
+| test1.c:41:16:41:16 | i | test1.c:42:11:42:11 | i | provenance |  |
+| test1.c:49:16:49:16 | i | test1.c:52:3:52:7 | ... = ... | provenance |  |
+| test1.c:52:3:52:7 | ... = ... | test1.c:54:15:54:15 | j | provenance |  |
 nodes
 | test1.c:7:26:7:29 | **argv | semmle.label | **argv |
 | test1.c:8:11:8:14 | call to atoi | semmle.label | call to atoi |
@@ -20,18 +20,18 @@ nodes
 | test1.c:11:9:11:9 | i | semmle.label | i |
 | test1.c:12:9:12:9 | i | semmle.label | i |
 | test1.c:13:9:13:9 | i | semmle.label | i |
-| test1.c:16:16:16:16 | i | semmle.label | i |
-| test1.c:18:16:18:16 | i | semmle.label | i |
-| test1.c:32:16:32:16 | i | semmle.label | i |
-| test1.c:33:11:33:11 | i | semmle.label | i |
-| test1.c:40:16:40:16 | i | semmle.label | i |
-| test1.c:41:11:41:11 | i | semmle.label | i |
-| test1.c:48:16:48:16 | i | semmle.label | i |
-| test1.c:51:3:51:7 | ... = ... | semmle.label | ... = ... |
-| test1.c:53:15:53:15 | j | semmle.label | j |
+| test1.c:17:16:17:16 | i | semmle.label | i |
+| test1.c:19:16:19:16 | i | semmle.label | i |
+| test1.c:33:16:33:16 | i | semmle.label | i |
+| test1.c:34:11:34:11 | i | semmle.label | i |
+| test1.c:41:16:41:16 | i | semmle.label | i |
+| test1.c:42:11:42:11 | i | semmle.label | i |
+| test1.c:49:16:49:16 | i | semmle.label | i |
+| test1.c:52:3:52:7 | ... = ... | semmle.label | ... = ... |
+| test1.c:54:15:54:15 | j | semmle.label | j |
 subpaths
 #select
-| test1.c:18:16:18:16 | i | test1.c:7:26:7:29 | **argv | test1.c:18:16:18:16 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
-| test1.c:33:11:33:11 | i | test1.c:7:26:7:29 | **argv | test1.c:33:11:33:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
-| test1.c:41:11:41:11 | i | test1.c:7:26:7:29 | **argv | test1.c:41:11:41:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
-| test1.c:53:15:53:15 | j | test1.c:7:26:7:29 | **argv | test1.c:53:15:53:15 | j | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
+| test1.c:19:16:19:16 | i | test1.c:7:26:7:29 | **argv | test1.c:19:16:19:16 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
+| test1.c:34:11:34:11 | i | test1.c:7:26:7:29 | **argv | test1.c:34:11:34:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
+| test1.c:42:11:42:11 | i | test1.c:7:26:7:29 | **argv | test1.c:42:11:42:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
+| test1.c:54:15:54:15 | j | test1.c:7:26:7:29 | **argv | test1.c:54:15:54:15 | j | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
index 926b81dad7c3..2c6a15d9ee1d 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
@@ -11,6 +11,7 @@ int main(int argc, char *argv[]) {
   test3(i);
   test4(i);
   test5(i);
+  test6(i);
 }
 
 void test1(int i) {
@@ -52,3 +53,7 @@ void test5(int i) {
 
   j = myArray[j]; // BAD: j has not been validated
 }
+
+void test6(int i) {
+
+}
\ No newline at end of file

From c3d9ea1820f9274bf9e1342b1f767e028ceeb106 Mon Sep 17 00:00:00 2001
From: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Date: Mon, 7 Oct 2024 17:01:06 +0100
Subject: [PATCH 04/13] C++: Add FP.

---
 .../ImproperArrayIndexValidation.expected                | 9 +++++++++
 .../CWE-129/semmle/ImproperArrayIndexValidation/test1.c  | 4 ++++
 2 files changed, 13 insertions(+)

diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
index 6f9ecd1d85d7..0631ac646f5c 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
@@ -4,15 +4,19 @@ edges
 | test1.c:8:11:8:14 | call to atoi | test1.c:11:9:11:9 | i | provenance |  |
 | test1.c:8:11:8:14 | call to atoi | test1.c:12:9:12:9 | i | provenance |  |
 | test1.c:8:11:8:14 | call to atoi | test1.c:13:9:13:9 | i | provenance |  |
+| test1.c:8:11:8:14 | call to atoi | test1.c:14:9:14:9 | i | provenance |  |
 | test1.c:9:9:9:9 | i | test1.c:17:16:17:16 | i | provenance |  |
 | test1.c:11:9:11:9 | i | test1.c:33:16:33:16 | i | provenance |  |
 | test1.c:12:9:12:9 | i | test1.c:41:16:41:16 | i | provenance |  |
 | test1.c:13:9:13:9 | i | test1.c:49:16:49:16 | i | provenance |  |
+| test1.c:14:9:14:9 | i | test1.c:59:16:59:16 | i | provenance |  |
 | test1.c:17:16:17:16 | i | test1.c:19:16:19:16 | i | provenance |  |
 | test1.c:33:16:33:16 | i | test1.c:34:11:34:11 | i | provenance |  |
 | test1.c:41:16:41:16 | i | test1.c:42:11:42:11 | i | provenance |  |
 | test1.c:49:16:49:16 | i | test1.c:52:3:52:7 | ... = ... | provenance |  |
 | test1.c:52:3:52:7 | ... = ... | test1.c:54:15:54:15 | j | provenance |  |
+| test1.c:59:16:59:16 | i | test1.c:60:21:60:21 | i | provenance |  |
+| test1.c:60:21:60:21 | i | test1.c:62:11:62:11 | s | provenance |  |
 nodes
 | test1.c:7:26:7:29 | **argv | semmle.label | **argv |
 | test1.c:8:11:8:14 | call to atoi | semmle.label | call to atoi |
@@ -20,6 +24,7 @@ nodes
 | test1.c:11:9:11:9 | i | semmle.label | i |
 | test1.c:12:9:12:9 | i | semmle.label | i |
 | test1.c:13:9:13:9 | i | semmle.label | i |
+| test1.c:14:9:14:9 | i | semmle.label | i |
 | test1.c:17:16:17:16 | i | semmle.label | i |
 | test1.c:19:16:19:16 | i | semmle.label | i |
 | test1.c:33:16:33:16 | i | semmle.label | i |
@@ -29,9 +34,13 @@ nodes
 | test1.c:49:16:49:16 | i | semmle.label | i |
 | test1.c:52:3:52:7 | ... = ... | semmle.label | ... = ... |
 | test1.c:54:15:54:15 | j | semmle.label | j |
+| test1.c:59:16:59:16 | i | semmle.label | i |
+| test1.c:60:21:60:21 | i | semmle.label | i |
+| test1.c:62:11:62:11 | s | semmle.label | s |
 subpaths
 #select
 | test1.c:19:16:19:16 | i | test1.c:7:26:7:29 | **argv | test1.c:19:16:19:16 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:34:11:34:11 | i | test1.c:7:26:7:29 | **argv | test1.c:34:11:34:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:42:11:42:11 | i | test1.c:7:26:7:29 | **argv | test1.c:42:11:42:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:54:15:54:15 | j | test1.c:7:26:7:29 | **argv | test1.c:54:15:54:15 | j | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
+| test1.c:62:11:62:11 | s | test1.c:7:26:7:29 | **argv | test1.c:62:11:62:11 | s | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
index 2c6a15d9ee1d..2aa77fc9ee13 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
@@ -54,6 +54,10 @@ void test5(int i) {
   j = myArray[j]; // BAD: j has not been validated
 }
 
+extern int myTable[256];
+
 void test6(int i) {
+  unsigned char s = i;
 
+  myTable[s] = 0; // GOOD: Input is small [FALSE POSITIVE]
 }
\ No newline at end of file

From a2cdb9c1731057d5ad6cdd46ff40aed8fec02c7e Mon Sep 17 00:00:00 2001
From: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Date: Mon, 7 Oct 2024 17:15:22 +0100
Subject: [PATCH 05/13] C++: Use range analysis at the sink to exclude trivial
 FPs.

---
 .../CWE/CWE-129/ImproperArrayIndexValidation.ql  | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql b/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql
index 0c5802a4c564..1978fc984d69 100644
--- a/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql
+++ b/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql
@@ -16,7 +16,7 @@ import cpp
 import semmle.code.cpp.controlflow.IRGuards
 import semmle.code.cpp.security.FlowSources as FS
 import semmle.code.cpp.dataflow.new.TaintTracking
-import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
+import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 import ImproperArrayIndexValidation::PathGraph
 
 predicate isFlowSource(FS::FlowSource source, string sourceType) {
@@ -39,6 +39,17 @@ predicate guardChecks(IRGuardCondition g, Expr e, boolean branch) {
   )
 }
 
+/**
+ * Holds if `arrayExpr` accesses an `ArrayType` with a constant size `N`, and
+ * the value of `offsetExpr` is known to be smaller than `N`.
+ */
+predicate offsetIsAlwaysInBounds(ArrayExpr arrayExpr, VariableAccess offsetExpr) {
+  exists(ArrayType arrayType |
+    arrayType = arrayExpr.getArrayBase().getUnspecifiedType() and
+    arrayType.getArraySize() > upperBound(offsetExpr.getFullyConverted())
+  )
+}
+
 module ImproperArrayIndexValidationConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { isFlowSource(source, _) }
 
@@ -51,7 +62,8 @@ module ImproperArrayIndexValidationConfig implements DataFlow::ConfigSig {
   predicate isSink(DataFlow::Node sink) {
     exists(ArrayExpr arrayExpr, VariableAccess offsetExpr |
       offsetExpr = arrayExpr.getArrayOffset() and
-      sink.asExpr() = offsetExpr
+      sink.asExpr() = offsetExpr and
+      not offsetIsAlwaysInBounds(arrayExpr, offsetExpr)
     )
   }
 }

From 8bb21e1b499f740d1fa9e37103af9be0b9dc84cf Mon Sep 17 00:00:00 2001
From: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Date: Mon, 7 Oct 2024 17:15:32 +0100
Subject: [PATCH 06/13] C++: Accept test changes.

---
 .../ImproperArrayIndexValidation.expected                | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
index 0631ac646f5c..6f9ecd1d85d7 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
@@ -4,19 +4,15 @@ edges
 | test1.c:8:11:8:14 | call to atoi | test1.c:11:9:11:9 | i | provenance |  |
 | test1.c:8:11:8:14 | call to atoi | test1.c:12:9:12:9 | i | provenance |  |
 | test1.c:8:11:8:14 | call to atoi | test1.c:13:9:13:9 | i | provenance |  |
-| test1.c:8:11:8:14 | call to atoi | test1.c:14:9:14:9 | i | provenance |  |
 | test1.c:9:9:9:9 | i | test1.c:17:16:17:16 | i | provenance |  |
 | test1.c:11:9:11:9 | i | test1.c:33:16:33:16 | i | provenance |  |
 | test1.c:12:9:12:9 | i | test1.c:41:16:41:16 | i | provenance |  |
 | test1.c:13:9:13:9 | i | test1.c:49:16:49:16 | i | provenance |  |
-| test1.c:14:9:14:9 | i | test1.c:59:16:59:16 | i | provenance |  |
 | test1.c:17:16:17:16 | i | test1.c:19:16:19:16 | i | provenance |  |
 | test1.c:33:16:33:16 | i | test1.c:34:11:34:11 | i | provenance |  |
 | test1.c:41:16:41:16 | i | test1.c:42:11:42:11 | i | provenance |  |
 | test1.c:49:16:49:16 | i | test1.c:52:3:52:7 | ... = ... | provenance |  |
 | test1.c:52:3:52:7 | ... = ... | test1.c:54:15:54:15 | j | provenance |  |
-| test1.c:59:16:59:16 | i | test1.c:60:21:60:21 | i | provenance |  |
-| test1.c:60:21:60:21 | i | test1.c:62:11:62:11 | s | provenance |  |
 nodes
 | test1.c:7:26:7:29 | **argv | semmle.label | **argv |
 | test1.c:8:11:8:14 | call to atoi | semmle.label | call to atoi |
@@ -24,7 +20,6 @@ nodes
 | test1.c:11:9:11:9 | i | semmle.label | i |
 | test1.c:12:9:12:9 | i | semmle.label | i |
 | test1.c:13:9:13:9 | i | semmle.label | i |
-| test1.c:14:9:14:9 | i | semmle.label | i |
 | test1.c:17:16:17:16 | i | semmle.label | i |
 | test1.c:19:16:19:16 | i | semmle.label | i |
 | test1.c:33:16:33:16 | i | semmle.label | i |
@@ -34,13 +29,9 @@ nodes
 | test1.c:49:16:49:16 | i | semmle.label | i |
 | test1.c:52:3:52:7 | ... = ... | semmle.label | ... = ... |
 | test1.c:54:15:54:15 | j | semmle.label | j |
-| test1.c:59:16:59:16 | i | semmle.label | i |
-| test1.c:60:21:60:21 | i | semmle.label | i |
-| test1.c:62:11:62:11 | s | semmle.label | s |
 subpaths
 #select
 | test1.c:19:16:19:16 | i | test1.c:7:26:7:29 | **argv | test1.c:19:16:19:16 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:34:11:34:11 | i | test1.c:7:26:7:29 | **argv | test1.c:34:11:34:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:42:11:42:11 | i | test1.c:7:26:7:29 | **argv | test1.c:42:11:42:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:54:15:54:15 | j | test1.c:7:26:7:29 | **argv | test1.c:54:15:54:15 | j | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
-| test1.c:62:11:62:11 | s | test1.c:7:26:7:29 | **argv | test1.c:62:11:62:11 | s | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |

From 823d5acd69eb713cb99e89336b92444ccf88a53d Mon Sep 17 00:00:00 2001
From: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Date: Mon, 7 Oct 2024 19:04:26 +0100
Subject: [PATCH 07/13] C++: Spacing.

---
 .../ImproperArrayIndexValidation.expected     | 44 +++++++++----------
 .../ImproperArrayIndexValidation/test1.c      |  5 ++-
 2 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
index 6f9ecd1d85d7..184af69e72c4 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
@@ -4,15 +4,15 @@ edges
 | test1.c:8:11:8:14 | call to atoi | test1.c:11:9:11:9 | i | provenance |  |
 | test1.c:8:11:8:14 | call to atoi | test1.c:12:9:12:9 | i | provenance |  |
 | test1.c:8:11:8:14 | call to atoi | test1.c:13:9:13:9 | i | provenance |  |
-| test1.c:9:9:9:9 | i | test1.c:17:16:17:16 | i | provenance |  |
-| test1.c:11:9:11:9 | i | test1.c:33:16:33:16 | i | provenance |  |
-| test1.c:12:9:12:9 | i | test1.c:41:16:41:16 | i | provenance |  |
-| test1.c:13:9:13:9 | i | test1.c:49:16:49:16 | i | provenance |  |
-| test1.c:17:16:17:16 | i | test1.c:19:16:19:16 | i | provenance |  |
-| test1.c:33:16:33:16 | i | test1.c:34:11:34:11 | i | provenance |  |
-| test1.c:41:16:41:16 | i | test1.c:42:11:42:11 | i | provenance |  |
-| test1.c:49:16:49:16 | i | test1.c:52:3:52:7 | ... = ... | provenance |  |
-| test1.c:52:3:52:7 | ... = ... | test1.c:54:15:54:15 | j | provenance |  |
+| test1.c:9:9:9:9 | i | test1.c:18:16:18:16 | i | provenance |  |
+| test1.c:11:9:11:9 | i | test1.c:34:16:34:16 | i | provenance |  |
+| test1.c:12:9:12:9 | i | test1.c:42:16:42:16 | i | provenance |  |
+| test1.c:13:9:13:9 | i | test1.c:50:16:50:16 | i | provenance |  |
+| test1.c:18:16:18:16 | i | test1.c:20:16:20:16 | i | provenance |  |
+| test1.c:34:16:34:16 | i | test1.c:35:11:35:11 | i | provenance |  |
+| test1.c:42:16:42:16 | i | test1.c:43:11:43:11 | i | provenance |  |
+| test1.c:50:16:50:16 | i | test1.c:53:3:53:7 | ... = ... | provenance |  |
+| test1.c:53:3:53:7 | ... = ... | test1.c:55:15:55:15 | j | provenance |  |
 nodes
 | test1.c:7:26:7:29 | **argv | semmle.label | **argv |
 | test1.c:8:11:8:14 | call to atoi | semmle.label | call to atoi |
@@ -20,18 +20,18 @@ nodes
 | test1.c:11:9:11:9 | i | semmle.label | i |
 | test1.c:12:9:12:9 | i | semmle.label | i |
 | test1.c:13:9:13:9 | i | semmle.label | i |
-| test1.c:17:16:17:16 | i | semmle.label | i |
-| test1.c:19:16:19:16 | i | semmle.label | i |
-| test1.c:33:16:33:16 | i | semmle.label | i |
-| test1.c:34:11:34:11 | i | semmle.label | i |
-| test1.c:41:16:41:16 | i | semmle.label | i |
-| test1.c:42:11:42:11 | i | semmle.label | i |
-| test1.c:49:16:49:16 | i | semmle.label | i |
-| test1.c:52:3:52:7 | ... = ... | semmle.label | ... = ... |
-| test1.c:54:15:54:15 | j | semmle.label | j |
+| test1.c:18:16:18:16 | i | semmle.label | i |
+| test1.c:20:16:20:16 | i | semmle.label | i |
+| test1.c:34:16:34:16 | i | semmle.label | i |
+| test1.c:35:11:35:11 | i | semmle.label | i |
+| test1.c:42:16:42:16 | i | semmle.label | i |
+| test1.c:43:11:43:11 | i | semmle.label | i |
+| test1.c:50:16:50:16 | i | semmle.label | i |
+| test1.c:53:3:53:7 | ... = ... | semmle.label | ... = ... |
+| test1.c:55:15:55:15 | j | semmle.label | j |
 subpaths
 #select
-| test1.c:19:16:19:16 | i | test1.c:7:26:7:29 | **argv | test1.c:19:16:19:16 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
-| test1.c:34:11:34:11 | i | test1.c:7:26:7:29 | **argv | test1.c:34:11:34:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
-| test1.c:42:11:42:11 | i | test1.c:7:26:7:29 | **argv | test1.c:42:11:42:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
-| test1.c:54:15:54:15 | j | test1.c:7:26:7:29 | **argv | test1.c:54:15:54:15 | j | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
+| test1.c:20:16:20:16 | i | test1.c:7:26:7:29 | **argv | test1.c:20:16:20:16 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
+| test1.c:35:11:35:11 | i | test1.c:7:26:7:29 | **argv | test1.c:35:11:35:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
+| test1.c:43:11:43:11 | i | test1.c:7:26:7:29 | **argv | test1.c:43:11:43:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
+| test1.c:55:15:55:15 | j | test1.c:7:26:7:29 | **argv | test1.c:55:15:55:15 | j | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
index 2aa77fc9ee13..0d3b45752c21 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
@@ -12,6 +12,7 @@ int main(int argc, char *argv[]) {
   test4(i);
   test5(i);
   test6(i);
+  test7(argv[1]);
 }
 
 void test1(int i) {
@@ -60,4 +61,6 @@ void test6(int i) {
   unsigned char s = i;
 
   myTable[s] = 0; // GOOD: Input is small [FALSE POSITIVE]
-}
\ No newline at end of file
+}
+
+void test7(char *s) { }
\ No newline at end of file

From 6ca978e1ccaa55e7155d21330fe44b0386be975d Mon Sep 17 00:00:00 2001
From: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Date: Mon, 7 Oct 2024 18:52:44 +0100
Subject: [PATCH 08/13] C++: Add FP test.

---
 .../ImproperArrayIndexValidation.expected         |  6 ++++++
 .../semmle/ImproperArrayIndexValidation/test1.c   | 15 ++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
index 184af69e72c4..7be82edfc096 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
@@ -13,6 +13,8 @@ edges
 | test1.c:42:16:42:16 | i | test1.c:43:11:43:11 | i | provenance |  |
 | test1.c:50:16:50:16 | i | test1.c:53:3:53:7 | ... = ... | provenance |  |
 | test1.c:53:3:53:7 | ... = ... | test1.c:55:15:55:15 | j | provenance |  |
+| test1.c:76:11:76:23 | ... = ... | test1.c:77:20:77:21 | ch | provenance |  |
+| test1.c:76:16:76:19 | call to getc | test1.c:76:11:76:23 | ... = ... | provenance |  |
 nodes
 | test1.c:7:26:7:29 | **argv | semmle.label | **argv |
 | test1.c:8:11:8:14 | call to atoi | semmle.label | call to atoi |
@@ -29,9 +31,13 @@ nodes
 | test1.c:50:16:50:16 | i | semmle.label | i |
 | test1.c:53:3:53:7 | ... = ... | semmle.label | ... = ... |
 | test1.c:55:15:55:15 | j | semmle.label | j |
+| test1.c:76:11:76:23 | ... = ... | semmle.label | ... = ... |
+| test1.c:76:16:76:19 | call to getc | semmle.label | call to getc |
+| test1.c:77:20:77:21 | ch | semmle.label | ch |
 subpaths
 #select
 | test1.c:20:16:20:16 | i | test1.c:7:26:7:29 | **argv | test1.c:20:16:20:16 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:35:11:35:11 | i | test1.c:7:26:7:29 | **argv | test1.c:35:11:35:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:43:11:43:11 | i | test1.c:7:26:7:29 | **argv | test1.c:43:11:43:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:55:15:55:15 | j | test1.c:7:26:7:29 | **argv | test1.c:55:15:55:15 | j | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
+| test1.c:77:20:77:21 | ch | test1.c:76:16:76:19 | call to getc | test1.c:77:20:77:21 | ch | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:76:16:76:19 | call to getc | external |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
index 0d3b45752c21..f688c19cf5bc 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
@@ -63,4 +63,17 @@ void test6(int i) {
   myTable[s] = 0; // GOOD: Input is small [FALSE POSITIVE]
 }
 
-void test7(char *s) { }
\ No newline at end of file
+typedef void FILE;
+#define EOF (-1)
+
+int getc(FILE*);
+
+extern int myMaxCharTable[256];
+
+void test7(FILE* fp) {
+  int ch;
+  while ((ch = getc(fp)) != EOF) {
+    myMaxCharTable[ch] = 0; // GOOD [FALSE POSITIVE]
+  }
+}
+

From f7392d6498aa1780700db6b9987fae42fd39384f Mon Sep 17 00:00:00 2001
From: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Date: Mon, 7 Oct 2024 19:00:03 +0100
Subject: [PATCH 09/13] C++: Range analysis of 'getc'.

---
 .../cpp/rangeanalysis/SimpleRangeAnalysis.qll | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll b/cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll
index 1ce7a6a4f5a8..990def8b2f1c 100644
--- a/cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll
+++ b/cpp/ql/lib/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll
@@ -192,6 +192,37 @@ private class UnsignedMulExpr extends MulExpr {
   }
 }
 
+/**
+ * Gets the value of the `EOF` macro.
+ *
+ * This is typically `"-1"`, but this is not guaranteed to be the case on all
+ * systems.
+ */
+private int getEofValue() {
+  exists(MacroInvocation mi |
+    mi.getMacroName() = "EOF" and
+    result = unique( | | mi.getExpr().getValue().toInt())
+  )
+}
+
+/** Get standard `getc` function or related variants. */
+private class Getc extends Function {
+  Getc() { this.hasGlobalOrStdOrBslName(["fgetc", "getc"]) }
+}
+
+/** A call to `getc` */
+private class CallToGetc extends FunctionCall {
+  CallToGetc() { this.getTarget() instanceof Getc }
+}
+
+/**
+ * A call to `getc` that we can analyze because we know
+ * the value of the `EOF` macro.
+ */
+private class AnalyzableCallToGetc extends CallToGetc {
+  AnalyzableCallToGetc() { exists(getEofValue()) }
+}
+
 /**
  * Holds if `expr` is effectively a multiplication of `operand` with the
  * positive constant `positive`.
@@ -287,6 +318,8 @@ private predicate analyzableExpr(Expr e) {
     or
     e instanceof RemExpr
     or
+    e instanceof AnalyzableCallToGetc
+    or
     // A conversion is analyzable, provided that its child has an arithmetic
     // type. (Sometimes the child is a reference type, and so does not get
     // any bounds.) Rather than checking whether the type of the child is
@@ -861,6 +894,14 @@ private float getLowerBoundsImpl(Expr expr) {
       )
     )
     or
+    exists(AnalyzableCallToGetc getc |
+      expr = getc and
+      // from https://en.cppreference.com/w/c/io/fgetc:
+      // On success, returns the obtained character as an unsigned char
+      // converted to an int. On failure, returns EOF.
+      result = min([typeLowerBound(any(UnsignedCharType pct)), getEofValue()])
+    )
+    or
     // If the conversion is to an arithmetic type then we just return the
     // lower bound of the child. We do not need to handle truncation and
     // overflow here, because that is done in `getTruncatedLowerBounds`.
@@ -1055,6 +1096,14 @@ private float getUpperBoundsImpl(Expr expr) {
       )
     )
     or
+    exists(AnalyzableCallToGetc getc |
+      expr = getc and
+      // from https://en.cppreference.com/w/c/io/fgetc:
+      // On success, returns the obtained character as an unsigned char
+      // converted to an int. On failure, returns EOF.
+      result = max([typeUpperBound(any(UnsignedCharType pct)), getEofValue()])
+    )
+    or
     // If the conversion is to an arithmetic type then we just return the
     // upper bound of the child. We do not need to handle truncation and
     // overflow here, because that is done in `getTruncatedUpperBounds`.

From d6618edf01e9eb0da94227f3a2d381810f682ab5 Mon Sep 17 00:00:00 2001
From: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Date: Mon, 7 Oct 2024 19:00:13 +0100
Subject: [PATCH 10/13] C++: Accept test changes.

---
 .../ImproperArrayIndexValidation.expected                   | 6 ------
 .../CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c | 2 +-
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
index 7be82edfc096..184af69e72c4 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected
@@ -13,8 +13,6 @@ edges
 | test1.c:42:16:42:16 | i | test1.c:43:11:43:11 | i | provenance |  |
 | test1.c:50:16:50:16 | i | test1.c:53:3:53:7 | ... = ... | provenance |  |
 | test1.c:53:3:53:7 | ... = ... | test1.c:55:15:55:15 | j | provenance |  |
-| test1.c:76:11:76:23 | ... = ... | test1.c:77:20:77:21 | ch | provenance |  |
-| test1.c:76:16:76:19 | call to getc | test1.c:76:11:76:23 | ... = ... | provenance |  |
 nodes
 | test1.c:7:26:7:29 | **argv | semmle.label | **argv |
 | test1.c:8:11:8:14 | call to atoi | semmle.label | call to atoi |
@@ -31,13 +29,9 @@ nodes
 | test1.c:50:16:50:16 | i | semmle.label | i |
 | test1.c:53:3:53:7 | ... = ... | semmle.label | ... = ... |
 | test1.c:55:15:55:15 | j | semmle.label | j |
-| test1.c:76:11:76:23 | ... = ... | semmle.label | ... = ... |
-| test1.c:76:16:76:19 | call to getc | semmle.label | call to getc |
-| test1.c:77:20:77:21 | ch | semmle.label | ch |
 subpaths
 #select
 | test1.c:20:16:20:16 | i | test1.c:7:26:7:29 | **argv | test1.c:20:16:20:16 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:35:11:35:11 | i | test1.c:7:26:7:29 | **argv | test1.c:35:11:35:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:43:11:43:11 | i | test1.c:7:26:7:29 | **argv | test1.c:43:11:43:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
 | test1.c:55:15:55:15 | j | test1.c:7:26:7:29 | **argv | test1.c:55:15:55:15 | j | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | **argv | a command-line argument |
-| test1.c:77:20:77:21 | ch | test1.c:76:16:76:19 | call to getc | test1.c:77:20:77:21 | ch | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:76:16:76:19 | call to getc | external |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
index f688c19cf5bc..89619626de91 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-129/semmle/ImproperArrayIndexValidation/test1.c
@@ -73,7 +73,7 @@ extern int myMaxCharTable[256];
 void test7(FILE* fp) {
   int ch;
   while ((ch = getc(fp)) != EOF) {
-    myMaxCharTable[ch] = 0; // GOOD [FALSE POSITIVE]
+    myMaxCharTable[ch] = 0; // GOOD
   }
 }
 

From b00c5457992c0bb88e1554269dade2f1eb5d3477 Mon Sep 17 00:00:00 2001
From: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Date: Mon, 7 Oct 2024 19:18:41 +0100
Subject: [PATCH 11/13] C++: Add change notes.

---
 cpp/ql/lib/change-notes/2024-10-07-range-analysis-of-getc.md  | 4 ++++
 .../change-notes/2024-10-07-unclear-array-index-validation.md | 4 ++++
 2 files changed, 8 insertions(+)
 create mode 100644 cpp/ql/lib/change-notes/2024-10-07-range-analysis-of-getc.md
 create mode 100644 cpp/ql/src/change-notes/2024-10-07-unclear-array-index-validation.md

diff --git a/cpp/ql/lib/change-notes/2024-10-07-range-analysis-of-getc.md b/cpp/ql/lib/change-notes/2024-10-07-range-analysis-of-getc.md
new file mode 100644
index 000000000000..f796fba2ece2
--- /dev/null
+++ b/cpp/ql/lib/change-notes/2024-10-07-range-analysis-of-getc.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `SimpleRangeAnalysis` library (`semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis`) now generates more precise ranges for calls to `fgetc` and `getc`.
\ No newline at end of file
diff --git a/cpp/ql/src/change-notes/2024-10-07-unclear-array-index-validation.md b/cpp/ql/src/change-notes/2024-10-07-unclear-array-index-validation.md
new file mode 100644
index 000000000000..97944e9da7ae
--- /dev/null
+++ b/cpp/ql/src/change-notes/2024-10-07-unclear-array-index-validation.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `cpp/unclear-array-index-validation` ("Unclear validation of array index") query has been imoroved to reduce false positives increase true positives.
\ No newline at end of file

From c883aa09f8321f9e6a9112aacbb02e3f2b975dc4 Mon Sep 17 00:00:00 2001
From: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Date: Wed, 9 Oct 2024 13:45:18 +0100
Subject: [PATCH 12/13] Update
 cpp/ql/src/change-notes/2024-10-07-unclear-array-index-validation.md

Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com>
---
 .../change-notes/2024-10-07-unclear-array-index-validation.md   | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpp/ql/src/change-notes/2024-10-07-unclear-array-index-validation.md b/cpp/ql/src/change-notes/2024-10-07-unclear-array-index-validation.md
index 97944e9da7ae..b237afdd6be8 100644
--- a/cpp/ql/src/change-notes/2024-10-07-unclear-array-index-validation.md
+++ b/cpp/ql/src/change-notes/2024-10-07-unclear-array-index-validation.md
@@ -1,4 +1,4 @@
 ---
 category: minorAnalysis
 ---
-* The `cpp/unclear-array-index-validation` ("Unclear validation of array index") query has been imoroved to reduce false positives increase true positives.
\ No newline at end of file
+* The `cpp/unclear-array-index-validation` ("Unclear validation of array index") query has been improved to reduce false positives increase true positives.
\ No newline at end of file

From 61a012fc6c1a96587f5420b63ca0719d3d5d9e36 Mon Sep 17 00:00:00 2001
From: Mathias Vorreiter Pedersen <mathiasvp@github.com>
Date: Wed, 9 Oct 2024 14:17:56 +0100
Subject: [PATCH 13/13] C++: Don't allow 'x < 0' as a barrier guard.

---
 .../Security/CWE/CWE-129/ImproperArrayIndexValidation.ql  | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql b/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql
index 1978fc984d69..b5dc4d893b21 100644
--- a/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql
+++ b/cpp/ql/src/Security/CWE/CWE-129/ImproperArrayIndexValidation.ql
@@ -25,11 +25,11 @@ predicate isFlowSource(FS::FlowSource source, string sourceType) {
 
 predicate guardChecks(IRGuardCondition g, Expr e, boolean branch) {
   exists(Operand op | op.getDef().getConvertedResultExpression() = e |
-    // op < k
-    g.comparesLt(op, _, true, any(BooleanValue bv | bv.getValue() = branch))
+    // `op < k` is true and `k > 0`
+    g.comparesLt(op, any(int k | k > 0), true, any(BooleanValue bv | bv.getValue() = branch))
     or
-    // op < _ + k
-    g.comparesLt(op, _, _, true, branch)
+    // `op < _ + k` is true and `k > 0`.
+    g.comparesLt(op, _, any(int k | k > 0), true, branch)
     or
     // op == k
     g.comparesEq(op, _, true, any(BooleanValue bv | bv.getValue() = branch))