Merge pull request #15456 from MathiasVP/fix-scanf-fp

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

Author: MathiasVP

Diff for: cpp/ql/src/Critical/ScanfChecks.qll

@@ -20,10 +20,37 @@ private predicate isLinuxKernel() {
   exists(Macro macro | macro.getName() in ["_LINUX_KERNEL_SPRINTF_H_", "_LINUX_KERNEL_H"])
 }
 
+/**
+ * Gets the value of the EOF macro.
+ *
+ * This is typically `"-1"`, but this is not guaranteed to be the case on all
+ * systems.
+ */
+private string getEofValue() {
+  exists(MacroInvocation mi |
+    mi.getMacroName() = "EOF" and
+    result = unique( | | mi.getExpr().getValue())
+  )
+}
+
+/**
+ * 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, _, _)
+    )
+  )
+}
+
 /**
  * Holds if `call` is a `scanf`-like call were the result is only checked against 0, but it can also return EOF.
  */
 predicate incorrectlyCheckedScanf(ScanfFunctionCall call) {
   exprInBooleanContext(call) and
+  not checkedForEof(call) and
   not isLinuxKernel() // scanf in the linux kernel can't return EOF
 }

Diff for: cpp/ql/src/change-notes/2024-01-29-incorrectly-checked-scanf.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The "Incorrect return-value check for a 'scanf'-like function" query (`cpp/incorrectly-checked-scanf`) no longer reports an alert when an explicit check for EOF is added.

Diff for: cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected

@@ -14,3 +14,4 @@
 | test.cpp:404:25:404:25 | u | 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:403:6:403:11 | call to sscanf | call to sscanf |
 | 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 |

Diff for: cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp

@@ -446,4 +446,16 @@ void bad_check() {
 		}
 		use(i);  // GOOD [FALSE POSITIVE]: Technically no security issue, but code is incorrect.
 	}
+}
+
+#define EOF (-1)
+
+void disjunct_boolean_condition(const char* modifier_data) {
+	long value;
+	auto rc = sscanf(modifier_data, "%lx", &value);
+
+	if((rc == EOF) || (rc == 0)) {
+		return;
+	}
+	use(value); // GOOD
 }