C++: Fix more FPs in cpp/invalid-pointer-deref

cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/RangeAnalysisUtil.qll

@@ -7,51 +7,14 @@ private import cpp
 private import semmle.code.cpp.rangeanalysis.new.internal.semantic.analysis.RangeAnalysis
 private import semmle.code.cpp.rangeanalysis.new.internal.semantic.SemanticExprSpecific
 private import semmle.code.cpp.ir.IR
+private import semmle.code.cpp.rangeanalysis.new.internal.semantic.analysis.Bound
+private import semmle.code.cpp.ir.ValueNumbering
 
-pragma[nomagic]
-private Instruction getABoundIn(SemBound b, IRFunction func) {
-  getSemanticExpr(result) = b.getExpr(0) and
-  result.getEnclosingIRFunction() = func
-}
-
-/**
- * Holds if `i <= b + delta`.
- */
-pragma[inline]
-private predicate boundedImplCand(Instruction i, Instruction b, int delta) {
-  exists(SemBound bound, IRFunction func |
-    semBounded(getSemanticExpr(i), bound, delta, true, _) and
-    b = getABoundIn(bound, func) and
-    i.getEnclosingIRFunction() = func
+/** Holds if `i < vn + delta` */
+predicate boundedByValueNumber(Instruction i, ValueNumber vn, int delta) {
+  exists(ValueNumberBound b |
+    b.getValueNumber() = vn and
+    delta = min(int cand | semBounded(getSemanticExpr(i), b, cand, true, _)) and
+    semBounded(getSemanticExpr(i), b, delta, true, _)
   )
 }
-
-/**
- * Holds if `i <= b + delta` and `delta` is the smallest integer that satisfies
- * this condition.
- */
-pragma[inline]
-private predicate boundedImpl(Instruction i, Instruction b, int delta) {
-  delta = min(int cand | boundedImplCand(i, b, cand))
-}
-
-/**
- * Holds if `i <= b + delta`.
- *
- * This predicate enforces a join-order that ensures that `i` has already been bound.
- */
-bindingset[i]
-pragma[inline_late]
-predicate bounded1(Instruction i, Instruction b, int delta) { boundedImpl(i, b, delta) }
-
-/**
- * Holds if `i <= b + delta`.
- *
- * This predicate enforces a join-order that ensures that `b` has already been bound.
- */
-bindingset[b]
-pragma[inline_late]
-predicate bounded2(Instruction i, Instruction b, int delta) { boundedImpl(i, b, delta) }
-
-/** Holds if `i <= b + delta`. */
-predicate bounded = boundedImpl/3;

cpp/ql/lib/semmle/code/cpp/security/InvalidPointerDereference/AllocationToInvalidPointer.qll

@@ -70,8 +70,8 @@ predicate hasSize(HeuristicAllocationExpr alloc, DataFlow::Node n, int state) {
     // Get the unique variable in a size expression like `x` in `malloc(x + 1)`.
     va = unique( | | getAVariableAccess(size)) and
     // Compute `delta` as the constant difference between `x` and `x + 1`.
-    bounded1(any(Instruction instr | instr.getUnconvertedResultExpression() = size),
-      any(LoadInstruction load | load.getUnconvertedResultExpression() = va), delta) and
+    boundedByValueNumber(any(Instruction instr | instr.getUnconvertedResultExpression() = size),
+      valueNumber(any(LoadInstruction load | load.getUnconvertedResultExpression() = va)), delta) and
     n.asExpr() = va and
     state = delta
   )
@@ -167,7 +167,7 @@ private module SizeBarrier {
       small = value.getAUse() and
       operandGuardChecks(pragma[only_bind_into](g), pragma[only_bind_into](small), large,
         pragma[only_bind_into](k), pragma[only_bind_into](edge)) and
-      bounded(result, value.getAnInstruction(), delta) and
+      boundedByValueNumber(result, value, delta) and
       g.controls(result.getBlock(), edge) and
       k < getASizeAddend(large)
     )
@@ -321,7 +321,7 @@ private predicate pointerAddInstructionHasBounds0(
     pai.getLeft() = allocSink.asInstruction() and
     sizeInstr = sizeSink.asInstruction() and
     // pai.getRight() <= sizeSink + delta
-    bounded1(right, sizeInstr, delta) and
+    boundedByValueNumber(right, valueNumber(sizeInstr), delta) and
     not right = SizeBarrier::getABarrierInstruction(delta) and
     not sizeInstr = SizeBarrier::getABarrierInstruction(delta)
   )

cpp/ql/lib/semmle/code/cpp/security/InvalidPointerDereference/InvalidPointerToDereference.qll

@@ -98,7 +98,7 @@ private module InvalidPointerToDerefBarrier {
     additional predicate isSource(DataFlow::Node source, PointerArithmeticInstruction pai) {
       invalidPointerToDerefSource(_, pai, _, _) and
       // source <= pai
-      bounded2(source.asInstruction(), pai, any(int d | d <= 0))
+      boundedByValueNumber(source.asInstruction(), valueNumber(pai), any(int d | d <= 0))
     }
 
     predicate isSource(DataFlow::Node source) { isSource(source, _) }
@@ -144,7 +144,7 @@ private module InvalidPointerToDerefBarrier {
       operandGuardChecks(pai, pragma[only_bind_into](g), pragma[only_bind_into](use),
         pragma[only_bind_into](k), pragma[only_bind_into](edge)) and
       // result <= value + delta
-      bounded(result, value.getAnInstruction(), delta) and
+      boundedByValueNumber(result, value, delta) and
       g.controls(result.getBlock(), edge) and
       delta + k <= 0
       // combining the above we have: result < pai + k + delta <= pai
@@ -214,7 +214,7 @@ private predicate invalidPointerToDerefSource(
   // and the instruction computing the address for which we will search for a dereference.
   AllocToInvalidPointer::pointerAddInstructionHasBounds(allocSource, pai, _, _) and
   // derefSource <= pai + deltaDerefSourceAndPai
-  bounded2(derefSource.asInstruction(), pai, deltaDerefSourceAndPai) and
+  boundedByValueNumber(derefSource.asInstruction(), valueNumber(pai), deltaDerefSourceAndPai) and
   deltaDerefSourceAndPai >= 0
 }
 
@@ -230,7 +230,7 @@ private predicate isInvalidPointerDerefSink(
 ) {
   exists(Instruction s |
     s = sink.asInstruction() and
-    bounded(addr.getDef(), s, deltaDerefSinkAndDerefAddress) and
+    boundedByValueNumber(addr.getDef(), valueNumber(s), deltaDerefSinkAndDerefAddress) and
     deltaDerefSinkAndDerefAddress >= 0 and
     i.getAnOperand() = addr
   |

cpp/ql/src/Security/CWE/CWE-119/OverrunWriteProductFlow.ql

@@ -21,6 +21,7 @@ import semmle.code.cpp.models.interfaces.ArrayFunction
 import semmle.code.cpp.rangeanalysis.new.internal.semantic.analysis.RangeAnalysis
 import semmle.code.cpp.rangeanalysis.new.internal.semantic.SemanticExprSpecific
 import semmle.code.cpp.rangeanalysis.new.RangeAnalysisUtil
+import semmle.code.cpp.ir.ValueNumbering
 import StringSizeFlow::PathGraph1
 import codeql.util.Unit
 
@@ -36,8 +37,8 @@ predicate hasSize(HeuristicAllocationExpr alloc, DataFlow::Node n, int state) {
     // Get the unique variable in a size expression like `x` in `malloc(x + 1)`.
     va = unique( | | getAVariableAccess(size)) and
     // Compute `delta` as the constant difference between `x` and `x + 1`.
-    bounded(any(Instruction instr | instr.getUnconvertedResultExpression() = size),
-      any(LoadInstruction load | load.getUnconvertedResultExpression() = va), delta) and
+    boundedByValueNumber(any(Instruction instr | instr.getUnconvertedResultExpression() = size),
+      valueNumber(any(LoadInstruction load | load.getUnconvertedResultExpression() = va)), delta) and
     n.asExpr() = va and
     state = delta
   )
@@ -56,7 +57,7 @@ predicate isSinkPairImpl(
     pragma[only_bind_into](func)
         .hasArrayWithVariableSize(pragma[only_bind_into](bufIndex),
           pragma[only_bind_into](sizeIndex)) and
-    bounded(c.getArgument(sizeIndex), sizeInstr, delta) and
+    boundedByValueNumber(c.getArgument(sizeIndex), valueNumber(sizeInstr), delta) and
     eBuf = bufInstr.getUnconvertedResultExpression()
   )
 }

cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql

@@ -12,36 +12,12 @@
 
 import semmle.code.cpp.rangeanalysis.new.internal.semantic.analysis.RangeAnalysis
 import semmle.code.cpp.rangeanalysis.new.internal.semantic.SemanticExprSpecific
+import semmle.code.cpp.rangeanalysis.new.RangeAnalysisUtil
+import semmle.code.cpp.ir.ValueNumbering
 import semmle.code.cpp.ir.IR
 import semmle.code.cpp.ir.dataflow.DataFlow
 import ArrayAddressToDerefFlow::PathGraph
 
-pragma[nomagic]
-Instruction getABoundIn(SemBound b, IRFunction func) {
-  getSemanticExpr(result) = b.getExpr(0) and
-  result.getEnclosingIRFunction() = func
-}
-
-/**
- * Holds if `i <= b + delta`.
- */
-pragma[inline]
-predicate boundedImpl(Instruction i, Instruction b, int delta) {
-  exists(SemBound bound, IRFunction func |
-    semBounded(getSemanticExpr(i), bound, delta, true, _) and
-    b = getABoundIn(bound, func) and
-    pragma[only_bind_out](i.getEnclosingIRFunction()) = func
-  )
-}
-
-bindingset[i]
-pragma[inline_late]
-predicate bounded1(Instruction i, Instruction b, int delta) { boundedImpl(i, b, delta) }
-
-bindingset[b]
-pragma[inline_late]
-predicate bounded2(Instruction i, Instruction b, int delta) { boundedImpl(i, b, delta) }
-
 bindingset[delta]
 predicate isInvalidPointerDerefSinkImpl(
   int delta, Instruction i, AddressOperand addr, string operation
@@ -65,15 +41,15 @@ predicate isInvalidPointerDerefSinkImpl(
 pragma[inline]
 predicate isInvalidPointerDerefSink1(DataFlow::Node sink, Instruction i, string operation) {
   exists(AddressOperand addr, int delta |
-    bounded1(addr.getDef(), sink.asInstruction(), delta) and
+    boundedByValueNumber(addr.getDef(), valueNumber(sink.asInstruction()), delta) and
     isInvalidPointerDerefSinkImpl(delta, i, addr, operation)
   )
 }
 
 pragma[inline]
 predicate isInvalidPointerDerefSink2(DataFlow::Node sink, Instruction i, string operation) {
   exists(AddressOperand addr, int delta |
-    bounded2(addr.getDef(), sink.asInstruction(), delta) and
+    boundedByValueNumber(addr.getDef(), valueNumber(sink.asInstruction()), delta) and
     isInvalidPointerDerefSinkImpl(delta, i, addr, operation)
   )
 }

cpp/ql/test/query-tests/Security/CWE/CWE-193/InvalidPointerDeref.expected

@@ -57,8 +57,6 @@ edges
 | test.cpp:206:17:206:23 | ... + ... | test.cpp:206:17:206:23 | ... + ... |
 | test.cpp:206:17:206:23 | ... + ... | test.cpp:213:5:213:13 | ... = ... |
 | test.cpp:206:17:206:23 | ... + ... | test.cpp:213:5:213:13 | ... = ... |
-| test.cpp:231:18:231:30 | new[] | test.cpp:232:3:232:20 | ... = ... |
-| test.cpp:238:20:238:32 | new[] | test.cpp:239:5:239:22 | ... = ... |
 | test.cpp:248:13:248:36 | call to realloc | test.cpp:254:9:254:16 | ... = ... |
 | test.cpp:260:13:260:24 | new[] | test.cpp:261:14:261:21 | ... + ... |
 | test.cpp:260:13:260:24 | new[] | test.cpp:261:14:261:21 | ... + ... |
@@ -228,10 +226,6 @@ nodes
 | test.cpp:206:17:206:23 | ... + ... | semmle.label | ... + ... |
 | test.cpp:206:17:206:23 | ... + ... | semmle.label | ... + ... |
 | test.cpp:213:5:213:13 | ... = ... | semmle.label | ... = ... |
-| test.cpp:231:18:231:30 | new[] | semmle.label | new[] |
-| test.cpp:232:3:232:20 | ... = ... | semmle.label | ... = ... |
-| test.cpp:238:20:238:32 | new[] | semmle.label | new[] |
-| test.cpp:239:5:239:22 | ... = ... | semmle.label | ... = ... |
 | test.cpp:248:13:248:36 | call to realloc | semmle.label | call to realloc |
 | test.cpp:254:9:254:16 | ... = ... | semmle.label | ... = ... |
 | test.cpp:260:13:260:24 | new[] | semmle.label | new[] |
@@ -335,8 +329,6 @@ subpaths
 | test.cpp:67:9:67:14 | ... = ... | test.cpp:52:19:52:24 | call to malloc | test.cpp:67:9:67:14 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:52:19:52:24 | call to malloc | call to malloc | test.cpp:53:20:53:23 | size | size |
 | test.cpp:201:5:201:19 | ... = ... | test.cpp:194:15:194:33 | call to malloc | test.cpp:201:5:201:19 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:194:15:194:33 | call to malloc | call to malloc | test.cpp:195:21:195:23 | len | len |
 | test.cpp:213:5:213:13 | ... = ... | test.cpp:205:15:205:33 | call to malloc | test.cpp:213:5:213:13 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:205:15:205:33 | call to malloc | call to malloc | test.cpp:206:21:206:23 | len | len |
-| test.cpp:232:3:232:20 | ... = ... | test.cpp:231:18:231:30 | new[] | test.cpp:232:3:232:20 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:231:18:231:30 | new[] | new[] | test.cpp:232:11:232:15 | index | index |
-| test.cpp:239:5:239:22 | ... = ... | test.cpp:238:20:238:32 | new[] | test.cpp:239:5:239:22 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:238:20:238:32 | new[] | new[] | test.cpp:239:13:239:17 | index | index |
 | test.cpp:254:9:254:16 | ... = ... | test.cpp:248:13:248:36 | call to realloc | test.cpp:254:9:254:16 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:248:13:248:36 | call to realloc | call to realloc | test.cpp:254:11:254:11 | i | i |
 | test.cpp:264:13:264:14 | * ... | test.cpp:260:13:260:24 | new[] | test.cpp:264:13:264:14 | * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:260:13:260:24 | new[] | new[] | test.cpp:261:19:261:21 | len | len |
 | test.cpp:274:5:274:10 | ... = ... | test.cpp:270:13:270:24 | new[] | test.cpp:274:5:274:10 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:270:13:270:24 | new[] | new[] | test.cpp:271:19:271:21 | len | len |

cpp/ql/test/query-tests/Security/CWE/CWE-193/test.cpp

@@ -229,14 +229,14 @@ void test15(unsigned index) {
     return;
   }
   int* newname = new int[size];
-  newname[index] = 0; // $ alloc=L231 deref=L232 // GOOD [FALSE POSITIVE]
+  newname[index] = 0; // GOOD
 }
 
 void test16(unsigned index) {
   unsigned size = index + 13;
   if(size >= index) {
     int* newname = new int[size];
-    newname[index] = 0; // $ alloc=L238 deref=L239 // GOOD [FALSE POSITIVE]
+    newname[index] = 0; // GOOD
   }
 }