Merge pull request #17054 from geoffw0/scanf

C++: Fix false positives in cpp/incorrectly-checked-scanf

cpp/ql/src/Critical/ScanfChecks.qll

@@ -38,11 +38,18 @@ private string getEofValue() {
 private predicate checkedForEof(ScanfFunctionCall call) {
   exists(IRGuardCondition gc |
     exists(Instruction i | i.getUnconvertedResultExpression() = call |
-      // call == EOF
-      gc.comparesEq(valueNumber(i).getAUse(), getEofValue().toInt(), _, _)
+      exists(int val | gc.comparesEq(valueNumber(i).getAUse(), val, _, _) |
+        // call == EOF
+        val = getEofValue().toInt()
+        or
+        // call == [any positive number]
+        val > 0
+      )
       or
-      // call < 0 (EOF is guaranteed to be negative)
-      gc.comparesLt(valueNumber(i).getAUse(), 0, true, _)
+      exists(int val | gc.comparesLt(valueNumber(i).getAUse(), val, true, _) |
+        // call < [any non-negative number] (EOF is guaranteed to be negative)
+        val >= 0
+      )
     )
   )
 }

cpp/ql/src/change-notes/2024-07-23-incorrectly-checked-scanf.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `cpp/incorrectly-checked-scanf` ("Incorrect return-value check for a 'scanf'-like function") query now produces fewer false positive results.

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

@@ -37,6 +37,21 @@ edges
 | test.cpp:420:19:420:20 | scanf output argument | test.cpp:423:7:423:7 | i | provenance |  |
 | test.cpp:455:41:455:46 | sscanf output argument | test.cpp:460:6:460:10 | value | provenance |  |
 | test.cpp:467:20:467:25 | scanf output argument | test.cpp:474:6:474:10 | value | provenance |  |
+| test.cpp:480:25:480:26 | scanf output argument | test.cpp:484:9:484:9 | i | provenance |  |
+| test.cpp:491:25:491:26 | scanf output argument | test.cpp:495:8:495:8 | i | provenance |  |
+| test.cpp:501:25:501:26 | scanf output argument | test.cpp:505:9:505:9 | i | provenance |  |
+| test.cpp:512:25:512:26 | scanf output argument | test.cpp:516:9:516:9 | i | provenance |  |
+| test.cpp:525:35:525:36 | sscanf output argument | test.cpp:527:8:527:8 | a | provenance |  |
+| test.cpp:525:35:525:36 | sscanf output argument | test.cpp:531:8:531:8 | a | provenance |  |
+| test.cpp:525:39:525:40 | sscanf output argument | test.cpp:528:8:528:8 | b | provenance |  |
+| test.cpp:525:39:525:40 | sscanf output argument | test.cpp:532:8:532:8 | b | provenance |  |
+| test.cpp:525:43:525:44 | sscanf output argument | test.cpp:533:8:533:8 | c | provenance |  |
+| test.cpp:541:35:541:36 | sscanf output argument | test.cpp:543:8:543:8 | d | provenance |  |
+| test.cpp:541:35:541:36 | sscanf output argument | test.cpp:548:8:548:8 | d | provenance |  |
+| test.cpp:541:39:541:40 | sscanf output argument | test.cpp:544:8:544:8 | e | provenance |  |
+| test.cpp:541:39:541:40 | sscanf output argument | test.cpp:549:8:549:8 | e | provenance |  |
+| test.cpp:541:43:541:44 | sscanf output argument | test.cpp:545:8:545:8 | f | provenance |  |
+| test.cpp:541:43:541:44 | sscanf output argument | test.cpp:550:8:550:8 | f | provenance |  |
 nodes
 | test.cpp:34:15:34:16 | scanf output argument | semmle.label | scanf output argument |
 | test.cpp:35:7:35:7 | i | semmle.label | i |
@@ -114,6 +129,31 @@ nodes
 | test.cpp:460:6:460:10 | value | semmle.label | value |
 | test.cpp:467:20:467:25 | scanf output argument | semmle.label | scanf output argument |
 | test.cpp:474:6:474:10 | value | semmle.label | value |
+| test.cpp:480:25:480:26 | scanf output argument | semmle.label | scanf output argument |
+| test.cpp:484:9:484:9 | i | semmle.label | i |
+| test.cpp:491:25:491:26 | scanf output argument | semmle.label | scanf output argument |
+| test.cpp:495:8:495:8 | i | semmle.label | i |
+| test.cpp:501:25:501:26 | scanf output argument | semmle.label | scanf output argument |
+| test.cpp:505:9:505:9 | i | semmle.label | i |
+| test.cpp:512:25:512:26 | scanf output argument | semmle.label | scanf output argument |
+| test.cpp:516:9:516:9 | i | semmle.label | i |
+| test.cpp:525:35:525:36 | sscanf output argument | semmle.label | sscanf output argument |
+| test.cpp:525:39:525:40 | sscanf output argument | semmle.label | sscanf output argument |
+| test.cpp:525:43:525:44 | sscanf output argument | semmle.label | sscanf output argument |
+| test.cpp:527:8:527:8 | a | semmle.label | a |
+| test.cpp:528:8:528:8 | b | semmle.label | b |
+| test.cpp:531:8:531:8 | a | semmle.label | a |
+| test.cpp:532:8:532:8 | b | semmle.label | b |
+| test.cpp:533:8:533:8 | c | semmle.label | c |
+| test.cpp:541:35:541:36 | sscanf output argument | semmle.label | sscanf output argument |
+| test.cpp:541:39:541:40 | sscanf output argument | semmle.label | sscanf output argument |
+| test.cpp:541:43:541:44 | sscanf output argument | semmle.label | sscanf output argument |
+| test.cpp:543:8:543:8 | d | semmle.label | d |
+| test.cpp:544:8:544:8 | e | semmle.label | e |
+| test.cpp:545:8:545:8 | f | semmle.label | f |
+| test.cpp:548:8:548:8 | d | semmle.label | d |
+| test.cpp:549:8:549:8 | e | semmle.label | e |
+| test.cpp:550:8:550:8 | f | semmle.label | f |
 subpaths
 #select
 | test.cpp:35:7:35:7 | i | test.cpp:34:15:34:16 | scanf output argument | test.cpp:35:7:35: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:34:3:34:7 | call to scanf | call to scanf |
@@ -134,3 +174,6 @@ subpaths
 | test.cpp:423:7:423:7 | i | test.cpp:420:19:420:20 | scanf output argument | 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 | test.cpp:455:41:455:46 | sscanf output argument | 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 | test.cpp:467:20:467:25 | scanf output argument | 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 |
+| test.cpp:484:9:484:9 | i | test.cpp:480:25:480:26 | scanf output argument | test.cpp:484:9:484:9 | 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:480:13:480:17 | call to scanf | call to scanf |
+| test.cpp:495:8:495:8 | i | test.cpp:491:25:491:26 | scanf output argument | test.cpp:495:8:495:8 | 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:491:13:491:17 | call to scanf | call to scanf |
+| test.cpp:545:8:545:8 | f | test.cpp:541:43:541:44 | sscanf output argument | test.cpp:545:8:545:8 | f | This variable is read, but may not have been written. It should be guarded by a check that the $@ returns at least 3. | test.cpp:541:10:541:15 | call to sscanf | call to sscanf |

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

@@ -472,4 +472,84 @@ void check_for_negative_test() {
 		return;
 	}
 	use(value);
-}
+}
+
+void multiple_checks() {
+	{
+		int i;
+		int res = scanf("%d", &i);
+
+		if (res >= 0) {
+			if (res != 0) {
+				use(i); // GOOD: checks return value [FALSE POSITIVE]
+			}
+		}
+	}
+
+	{
+		int i;
+		int res = scanf("%d", &i);
+
+		if (res < 0) return;
+		if (res != 0) {
+			use(i); // GOOD: checks return value [FALSE POSITIVE]
+		}
+	}
+
+	{
+		int i;
+		int res = scanf("%d", &i);
+
+		if (res >= 1) {
+			if (res != 0) {
+				use(i); // GOOD: checks return value
+			}
+		}
+	}
+
+	{
+		int i;
+		int res = scanf("%d", &i);
+
+		if (res == 1) {
+			if (res != 0) {
+				use(i); // GOOD: checks return value
+			}
+		}
+	}
+}
+
+void switch_cases(const char *data) {
+	float a, b, c;
+
+	switch (sscanf(data, "%f %f %f", &a, &b, &c)) {
+		case 2:
+			use(a); // GOOD
+			use(b); // GOOD
+			break;
+		case 3:
+			use(a); // GOOD
+			use(b); // GOOD
+			use(c); // GOOD
+			break;
+		default:
+			break;
+	}
+
+	float d, e, f;
+
+	switch (sscanf(data, "%f %f %f", &d, &e, &f)) {
+		case 2:
+			use(d); // GOOD
+			use(e); // GOOD
+			use(f); // BAD
+			break;
+		case 3:
+			use(d); // GOOD
+			use(e); // GOOD
+			use(f); // GOOD
+			break;
+		default:
+			break;
+	}
+}