Merge pull request #15460 from MathiasVP/fix-scanf-fp-2

C++: Fix another FP in `cpp/incorrectly-checked-scanf`

Author: MathiasVP

cpp/ql/src/Critical/ScanfChecks.qll

@@ -3,12 +3,15 @@ private import semmle.code.cpp.commons.Scanf
 private import semmle.code.cpp.controlflow.IRGuards
 private import semmle.code.cpp.ir.ValueNumbering
 
+private ConstantInstruction getZeroInstruction() { result.getValue() = "0" }
+
+private Operand zero() { result.getDef() = getZeroInstruction() }
+
 private predicate exprInBooleanContext(Expr e) {
   exists(IRGuardCondition gc |
-    exists(Instruction i, ConstantInstruction zero |
-      zero.getValue() = "0" and
+    exists(Instruction i |
       i.getUnconvertedResultExpression() = e and
-      gc.comparesEq(valueNumber(i).getAUse(), zero.getAUse(), 0, _, _)
+      gc.comparesEq(valueNumber(i).getAUse(), zero(), 0, _, _)
     )
     or
     gc.getUnconvertedResultExpression() = e
@@ -33,15 +36,21 @@ private string getEofValue() {
   )
 }
 
+private ConstantInstruction getEofInstruction() { result.getValue() = getEofValue() }
+
+private Operand eof() { result.getDef() = getEofInstruction() }
+
 /**
  * Holds if the value of `call` has been checked to not equal `EOF`.
  */
 private predicate checkedForEof(ScanfFunctionCall call) {
   exists(IRGuardCondition gc |
-    exists(Instruction i, ConstantInstruction eof |
-      eof.getValue() = getEofValue() and
-      i.getUnconvertedResultExpression() = call and
-      gc.comparesEq(valueNumber(i).getAUse(), eof.getAUse(), 0, _, _)
+    exists(Instruction i | i.getUnconvertedResultExpression() = call |
+      // call == EOF
+      gc.comparesEq(valueNumber(i).getAUse(), eof(), 0, _, _)
+      or
+      // call < 0 (EOF is guaranteed to be negative)
+      gc.comparesLt(valueNumber(i).getAUse(), zero(), 0, true, _)
     )
   )
 }

cpp/ql/src/change-notes/2024-01-29-incorrectly-checked-scanf-2.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The "Incorrect return-value check for a 'scanf'-like function" query (`cpp/incorrectly-checked-scanf`) now recognizes more EOF checks.

cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected

@@ -15,3 +15,4 @@
 | test.cpp:416:7:416:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:413:7:413:11 | call to scanf | call to scanf |
 | test.cpp:423:7:423:7 | i | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:420:7:420:11 | call to scanf | call to scanf |
 | test.cpp:460:6:460:10 | value | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:455:12:455:17 | call to sscanf | call to sscanf |
+| test.cpp:474:6:474:10 | value | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 1. | test.cpp:467:8:467:12 | call to scanf | call to scanf |

cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp

@@ -458,4 +458,18 @@ void disjunct_boolean_condition(const char* modifier_data) {
 		return;
 	}
 	use(value); // GOOD
+}
+
+void check_for_negative_test() {
+	int res;
+	int value;
+
+	res = scanf("%d", &value); // GOOD
+	if(res == 0) {
+		return;
+	}
+	if (res < 0) {
+		return;
+	}
+	use(value);
 }