From 94c43c07c72d65c8d8c8084c214061e6d9b164b2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Thu, 11 Aug 2022 12:18:37 +0200
Subject: [PATCH 01/17] Update .gitignore for .vscode/*.log temporaries

These keep getting added, by the Makefile extension I believe.
---
 .gitignore | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/.gitignore b/.gitignore
index 7b8532b00d2c..cfea60b51a16 100644
--- a/.gitignore
+++ b/.gitignore
@@ -15,6 +15,9 @@
 .vs/*
 !.vs/VSWorkspaceSettings.json
 
+# VSCode temporaries
+.vscode/*.log
+
 # Byte-compiled python files
 *.pyc
 

From 76ef779f60b01bf0efe6289c31ea6a89c682cf7e Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 Jul 2022 13:23:02 +0100
Subject: [PATCH 02/17] C++: Add test and placeholder query.

---
 cpp/ql/src/Critical/MissingCheckScanf.ql      |  14 ++
 .../MissingCheckScanf.expected                |   0
 .../MissingCheckScanf/MissingCheckScanf.qlref |   1 +
 .../Critical/MissingCheckScanf/test.cpp       | 195 ++++++++++++++++++
 4 files changed, 210 insertions(+)
 create mode 100644 cpp/ql/src/Critical/MissingCheckScanf.ql
 create mode 100644 cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
 create mode 100644 cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.qlref
 create mode 100644 cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp

diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql
new file mode 100644
index 000000000000..3ad7f2b99c00
--- /dev/null
+++ b/cpp/ql/src/Critical/MissingCheckScanf.ql
@@ -0,0 +1,14 @@
+/**
+ * @name TODO
+ * @description TODO
+ * @kind problem
+ * @problem.severity TODO
+ * @security-severity TODO
+ * @precision TODO
+ * @id cpp/missing-check-scanf
+ * @tags TODO
+ */
+
+import cpp
+
+select "TODO"
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
new file mode 100644
index 000000000000..e69de29bb2d1
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.qlref b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.qlref
new file mode 100644
index 000000000000..97e85b5abbea
--- /dev/null
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.qlref
@@ -0,0 +1 @@
+Critical/MissingCheckScanf.ql
\ No newline at end of file
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
new file mode 100644
index 000000000000..2e4abdab5452
--- /dev/null
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
@@ -0,0 +1,195 @@
+typedef struct {} FILE;
+
+int scanf(const char *format, ...);
+int fscanf(FILE *stream, const char *format, ...);
+int sscanf(const char *s, const char *format, ...);
+
+void use(int i);
+
+void set_by_ref(int &i);
+void set_by_ptr(int *i);
+bool maybe();
+
+FILE *get_a_stream();
+const char *get_a_string();
+
+int main()
+{
+	// --- simple cases ---
+
+	{
+		int i;
+
+		scanf("%d", &i); // BAD: may not have written `i`
+		use(i);
+	}
+
+	{
+		int i;
+
+		if (scanf("%d", &i) == 1) // GOOD: checks return value
+		{
+			use(i);
+		}
+	}
+
+	{
+		int i = 0;
+
+		scanf("%d", &i); // GOOD: we assume the initialization of `i` is a reasonable default
+		use(i);
+	}
+
+	// --- different scanf functions ---
+
+	{
+		int i;
+
+		fscanf(get_a_stream(), "%d", &i); // BAD: may not have written `i`
+		use(i);
+	}
+
+	{
+		int i;
+
+		sscanf(get_a_string(), "%d", &i); // BAD: may not have written `i`
+		use(i);
+	}
+
+	// --- different ways of checking ---
+
+	{
+		int i;
+
+		if (scanf("%d", &i) >= 1) // GOOD
+		{
+			use(i);
+		}
+	}
+
+	{
+		int i;
+
+		if (scanf("%d", &i) == 1) // GOOD
+		{
+			use(i);
+		}
+	}
+
+	{
+		int i;
+
+		if (scanf("%d", &i) != 0) // GOOD (just barely)
+		{
+			use(i);
+		}
+	}
+
+	{
+		int i;
+
+		if (scanf("%d", &i) == 0) // BAD: checks return value incorrectly
+		{
+			use(i);
+		}
+	}
+
+	{
+		bool b;
+		int i;
+
+		b = scanf("%d", &i); // GOOD
+
+		if (b >= 1)
+		{
+			use(i);
+		}
+	}
+
+	{
+		int i, j;
+
+		if (scanf("%d %d", &i) >= 2) // GOOD
+		{
+			use(i);
+			use(j);
+		}
+	}
+
+	{
+		int i, j;
+
+		if (scanf("%d %d", &i) >= 1) // BAD: checks return value incorrectly
+		{
+			use(i);
+			use(j);
+		}
+	}
+
+	// --- different initialization ---
+
+	{
+		int i;
+		i = 0;
+
+		scanf("%d", &i); // GOOD
+		use(i);
+	}
+
+	{
+		int i;
+
+		set_by_ref(i);
+		scanf("%d", &i); // GOOD: we have to assume `i` was initialized
+		use(i);
+	}
+
+	{
+		int i;
+
+		set_by_ptr(&i);
+		scanf("%d", &i); // GOOD: we have to assume `i` was initialized
+		use(i);
+	}
+
+	{
+		int i;
+
+		if (maybe())
+		{
+			i = 0;
+		}
+
+		scanf("%d", &i); // BAD: `i` may not have been initialized
+		use(i);
+	}
+
+	// --- weird formatting strings ---
+
+	{
+		int i;
+
+		if (scanf("%n %d", &i) >= 1) // GOOD (`%n` does not consume input)
+		{
+			use(i);
+		}
+	}
+
+	{
+		int i;
+
+		if (scanf("%% %d", &i) >= 1) // GOOD (`%%` does not consume input)
+		{
+			use(i);
+		}
+	}
+
+	{
+		int i;
+
+		if (scanf("%*d %d", &i) >= 1) // GOOD (`%*d` does not consume input)
+		{
+			use(i);
+		}
+	}
+}

From c62ae3b350de68f319f986bc9c1debc689570a13 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 Jul 2022 15:37:19 +0100
Subject: [PATCH 03/17] C++: First working. We now prefer flagging the cases
 where the variable was initialized, as in real world cases we haven't seen it
 done safely.

---
 cpp/ql/src/Critical/MissingCheckScanf.ql      |  9 +++++--
 .../MissingCheckScanf.expected                |  9 +++++++
 .../Critical/MissingCheckScanf/test.cpp       | 24 +++++++++++++------
 3 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql
index 3ad7f2b99c00..80ef5aa596f9 100644
--- a/cpp/ql/src/Critical/MissingCheckScanf.ql
+++ b/cpp/ql/src/Critical/MissingCheckScanf.ql
@@ -2,7 +2,7 @@
  * @name TODO
  * @description TODO
  * @kind problem
- * @problem.severity TODO
+ * @problem.severity warning
  * @security-severity TODO
  * @precision TODO
  * @id cpp/missing-check-scanf
@@ -10,5 +10,10 @@
  */
 
 import cpp
+import semmle.code.cpp.commons.Scanf
 
-select "TODO"
+from ScanfFunction scanf, FunctionCall fc
+where
+    fc.getTarget() = scanf and
+    fc instanceof ExprInVoidContext
+select fc, "This is a call to scanf."
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
index e69de29bb2d1..edd1b21a0e03 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
@@ -0,0 +1,9 @@
+| test.cpp:23:3:23:7 | call to scanf | This is a call to scanf. |
+| test.cpp:39:3:39:7 | call to scanf | This is a call to scanf. |
+| test.cpp:48:3:48:8 | call to fscanf | This is a call to scanf. |
+| test.cpp:55:3:55:8 | call to sscanf | This is a call to scanf. |
+| test.cpp:135:3:135:7 | call to scanf | This is a call to scanf. |
+| test.cpp:143:3:143:7 | call to scanf | This is a call to scanf. |
+| test.cpp:151:3:151:7 | call to scanf | This is a call to scanf. |
+| test.cpp:163:3:163:7 | call to scanf | This is a call to scanf. |
+| test.cpp:173:3:173:7 | call to scanf | This is a call to scanf. |
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
index 2e4abdab5452..8d76495b6337 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
@@ -36,7 +36,7 @@ int main()
 	{
 		int i = 0;
 
-		scanf("%d", &i); // GOOD: we assume the initialization of `i` is a reasonable default
+		scanf("%d", &i); // BAD
 		use(i);
 	}
 
@@ -79,7 +79,7 @@ int main()
 	{
 		int i;
 
-		if (scanf("%d", &i) != 0) // GOOD (just barely)
+		if (scanf("%d", &i) != 0) // BAD: scanf can return -1 [NOT DETECTED]
 		{
 			use(i);
 		}
@@ -88,7 +88,7 @@ int main()
 	{
 		int i;
 
-		if (scanf("%d", &i) == 0) // BAD: checks return value incorrectly
+		if (scanf("%d", &i) == 0) // BAD: checks return value incorrectly [NOT DETECTED]
 		{
 			use(i);
 		}
@@ -119,7 +119,7 @@ int main()
 	{
 		int i, j;
 
-		if (scanf("%d %d", &i) >= 1) // BAD: checks return value incorrectly
+		if (scanf("%d %d", &i, &j) >= 1) // BAD: checks return value incorrectly [NOT DETECTED]
 		{
 			use(i);
 			use(j);
@@ -132,7 +132,7 @@ int main()
 		int i;
 		i = 0;
 
-		scanf("%d", &i); // GOOD
+		scanf("%d", &i); // BAD
 		use(i);
 	}
 
@@ -140,7 +140,7 @@ int main()
 		int i;
 
 		set_by_ref(i);
-		scanf("%d", &i); // GOOD: we have to assume `i` was initialized
+		scanf("%d", &i); // BAD
 		use(i);
 	}
 
@@ -148,7 +148,7 @@ int main()
 		int i;
 
 		set_by_ptr(&i);
-		scanf("%d", &i); // GOOD: we have to assume `i` was initialized
+		scanf("%d", &i); // BAD
 		use(i);
 	}
 
@@ -164,6 +164,16 @@ int main()
 		use(i);
 	}
 
+	// --- different use ---
+	
+	{
+		int i;
+		int *ptr_i = &i;
+
+		scanf("%d", &i); // BAD: may not have written `i`
+		use(*ptr_i);
+	}
+
 	// --- weird formatting strings ---
 
 	{

From 3e71ef2baa8c6b10550167609ce8e917610aa65b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Thu, 11 Aug 2022 16:48:11 +0200
Subject: [PATCH 04/17] Add more MissingCheckScanf test cases

---
 .../MissingCheckScanf.expected                | 15 ++--
 .../Critical/MissingCheckScanf/test.cpp       | 71 ++++++++++++++++++-
 2 files changed, 76 insertions(+), 10 deletions(-)

diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
index edd1b21a0e03..6db509fcd9de 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
@@ -1,9 +1,10 @@
 | test.cpp:23:3:23:7 | call to scanf | This is a call to scanf. |
 | test.cpp:39:3:39:7 | call to scanf | This is a call to scanf. |
-| test.cpp:48:3:48:8 | call to fscanf | This is a call to scanf. |
-| test.cpp:55:3:55:8 | call to sscanf | This is a call to scanf. |
-| test.cpp:135:3:135:7 | call to scanf | This is a call to scanf. |
-| test.cpp:143:3:143:7 | call to scanf | This is a call to scanf. |
-| test.cpp:151:3:151:7 | call to scanf | This is a call to scanf. |
-| test.cpp:163:3:163:7 | call to scanf | This is a call to scanf. |
-| test.cpp:173:3:173:7 | call to scanf | This is a call to scanf. |
+| test.cpp:56:3:56:7 | call to scanf | This is a call to scanf. |
+| test.cpp:70:3:70:8 | call to fscanf | This is a call to scanf. |
+| test.cpp:77:3:77:8 | call to sscanf | This is a call to scanf. |
+| test.cpp:178:3:178:7 | call to scanf | This is a call to scanf. |
+| test.cpp:186:3:186:7 | call to scanf | This is a call to scanf. |
+| test.cpp:194:3:194:7 | call to scanf | This is a call to scanf. |
+| test.cpp:206:3:206:7 | call to scanf | This is a call to scanf. |
+| test.cpp:216:3:216:7 | call to scanf | This is a call to scanf. |
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
index 8d76495b6337..abd98e21aacd 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
@@ -36,8 +36,30 @@ int main()
 	{
 		int i = 0;
 
+		scanf("%d", &i); // BAD. Design choice: already initialized variables shouldn't make a difference.
+		use(i);
+	}
+
+	{
+		int i;
+		use(i);
+
+		if (scanf("%d", &i) == 1) // GOOD: only care about uses after scanf call
+		{
+			use(i);
+		}
+	}
+
+	{
+		int i; // Reused variable
+
 		scanf("%d", &i); // BAD
 		use(i);
+
+		if (scanf("%d", &i) == 1) // GOOD
+		{
+			use(i);
+		}
 	}
 
 	// --- different scanf functions ---
@@ -94,11 +116,23 @@ int main()
 		}
 	}
 
+	{
+		int r;
+		int i;
+
+		r = scanf("%d", &i); // GOOD
+
+		if (r >= 1)
+		{
+			use(i);
+		}
+	}
+
 	{
 		bool b;
 		int i;
 
-		b = scanf("%d", &i); // GOOD
+		b = scanf("%d", &i); // BAD [NOT DETECTED]: scanf can return EOF (boolifies true)
 
 		if (b >= 1)
 		{
@@ -106,10 +140,19 @@ int main()
 		}
 	}
 
+	{
+		bool b;
+		int i;
+
+		b = scanf("%d", &i); // BAD [NOT DETECTED]
+
+		use(i);
+	}
+
 	{
 		int i, j;
 
-		if (scanf("%d %d", &i) >= 2) // GOOD
+		if (scanf("%d %d", &i) >= 2) // GOOD: `j` is not a scanf arg, so out of scope of MissingCheckScanf
 		{
 			use(i);
 			use(j);
@@ -165,7 +208,7 @@ int main()
 	}
 
 	// --- different use ---
-	
+
 	{
 		int i;
 		int *ptr_i = &i;
@@ -203,3 +246,25 @@ int main()
 		}
 	}
 }
+
+// Non-local cases:
+
+bool my_scan_int(int &i)
+{
+	return scanf("%d", &i) == 1; // GOOD
+}
+
+void my_scan_int_test()
+{
+	int i;
+
+	use(i); // GOOD: used before scanf
+
+	my_scan_int(i); // BAD [NOT DETECTED]
+	use(i);
+
+	if (my_scan_int(i)) // GOOD
+	{
+		use(i);
+	}
+}

From c7db7ecfebebb67ae47d61c37584900e585fa56f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Tue, 16 Aug 2022 11:05:34 +0200
Subject: [PATCH 05/17] WIP: ScanfOutput.getAnAccess() returns all immediate
 use()s.

Also, include a couple of other test cases for related corner cases,
namely, to catch stores-before-uses and another case of aliasing.
---
 cpp/ql/src/Critical/MissingCheckScanf.ql      | 74 ++++++++++++++++++-
 .../Critical/MissingCheckScanf/test.cpp       | 19 ++++-
 2 files changed, 88 insertions(+), 5 deletions(-)

diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql
index 80ef5aa596f9..9232de2b30ab 100644
--- a/cpp/ql/src/Critical/MissingCheckScanf.ql
+++ b/cpp/ql/src/Critical/MissingCheckScanf.ql
@@ -11,9 +11,75 @@
 
 import cpp
 import semmle.code.cpp.commons.Scanf
+import semmle.code.cpp.controlflow.Guards
+import semmle.code.cpp.dataflow.DataFlow
+import semmle.code.cpp.ir.IR
+import semmle.code.cpp.ir.ValueNumbering
 
-from ScanfFunction scanf, FunctionCall fc
-where
-    fc.getTarget() = scanf and
-    fc instanceof ExprInVoidContext
+// DataFlow::localFlow(DataFlow::parameterNode(src), DataFlow::exprNode(snk))
+/*
+ * Find:
+ * - all scanf calls
+ *   => ScanfFunctionCall
+ *
+ * - relate the value number of each out-arg to:
+ *   1. the return value of the scanf call, and
+ *   2. the arg number, post-format, of the out-arg.
+ *   => scanfOutput/3
+ *
+ * - Find all accesses to variables with the same
+ *   value number, and if applicable, the guard condition
+ *   of one of their enclosing blocks that ensures
+ *   that the scanf return value is at least the
+ *   associated arg-number
+ *
+ * - Combine into data flow tracking from out-arg to use,
+ *   with usage in _other_ scanf calls as barrier.
+ */
+
+class ScanfOutput extends Expr {
+  ScanfFunctionCall call;
+  int argNum;
+
+  ScanfOutput() {
+    this = call.getArgument(call.getFormatParameterIndex() + argNum) and
+    argNum >= 1
+  }
+
+  Access getAnAccess() {
+    exists(Instruction j |
+      j = getNextInstruction() and
+      forall(Instruction k | k = getAReset() implies j.getASuccessor+() = k) and
+      forall(Instruction k | k = getAReuse() implies j.getASuccessor+() = k)
+    |
+      result = j.getAst()
+    )
+  }
+
+  private Instruction getAReset() {
+    result = getNextInstruction() and
+    result = any(StoreInstruction s).getDestinationAddress()
+  }
+
+  private Instruction getAReuse() {
+    result = getNextInstruction() and
+    exists(Expr e | result.getAst() = e |
+      e instanceof ScanfOutput
+      or
+      e.getParent().(AddressOfExpr) instanceof ScanfOutput
+    )
+  }
+
+  private Instruction getNextInstruction() {
+    exists(Instruction i |
+      i.getUnconvertedResultExpression() = this and
+      result = valueNumber(i).getAnInstruction() and
+      i.getASuccessor+() = result
+    )
+  }
+}
+
+//select any(CallInstruction i | i.getStaticCallTarget() instanceof ScanfFunction).toString()
+from ScanfFunctionCall fc
+where fc instanceof ExprInVoidContext
 select fc, "This is a call to scanf."
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
index abd98e21aacd..937e7197896a 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
@@ -1,4 +1,6 @@
-typedef struct {} FILE;
+typedef struct
+{
+} FILE;
 
 int scanf(const char *format, ...);
 int fscanf(FILE *stream, const char *format, ...);
@@ -217,6 +219,21 @@ int main()
 		use(*ptr_i);
 	}
 
+	{
+		int i;
+		int *ptr_i = &i;
+
+		scanf("%d", ptr_i); // BAD: may not have written `*ptr_i`
+		use(i);
+	}
+
+	{
+		int i;
+		scanf("%d", &i);
+		i = 42;
+		use(i); // GOOD [FALSE POSITIVE]
+	}
+
 	// --- weird formatting strings ---
 
 	{

From a7f46e80b48f34df332e96959c0c75d6a7130271 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Tue, 16 Aug 2022 16:29:18 +0200
Subject: [PATCH 06/17] WIP: Full-featured query

Still needs to be tested further. But currently returns results on
the `unbit/uwsgi` database: see https://github.com/github/semmle-code/actions/runs/2868369365
---
 cpp/ql/src/Critical/MissingCheckScanf.ql      |  53 ++++++-
 .../Critical/MissingCheckScanf/test.cpp       | 143 ++++++++++--------
 2 files changed, 130 insertions(+), 66 deletions(-)

diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql
index 9232de2b30ab..62914572348d 100644
--- a/cpp/ql/src/Critical/MissingCheckScanf.ql
+++ b/cpp/ql/src/Critical/MissingCheckScanf.ql
@@ -46,11 +46,32 @@ class ScanfOutput extends Expr {
     argNum >= 1
   }
 
+  ScanfFunctionCall getCall() { result = call }
+
+  int getNumber() { result = argNum }
+
+  predicate hasGuardedAccess(Access e, boolean isGuarded) {
+    e = getAnAccess() and
+    if
+      exists(int value |
+        e.getBasicBlock() = blockGuardedBy(value, "==", call) and argNum <= value
+        or
+        e.getBasicBlock() = blockGuardedBy(value, "<", call) and argNum - 1 <= value
+        or
+        e.getBasicBlock() = blockGuardedBy(value, "<=", call) and argNum <= value
+      )
+    then isGuarded = true
+    else isGuarded = false
+  }
+
+  /**
+   * Get a subsequent access of the same underlying storage,
+   * but before it gets reset or reused in another scanf call.
+   */
   Access getAnAccess() {
     exists(Instruction j |
       j = getNextInstruction() and
-      forall(Instruction k | k = getAReset() implies j.getASuccessor+() = k) and
-      forall(Instruction k | k = getAReuse() implies j.getASuccessor+() = k)
+      forall(Instruction k | k = [getAReset(), getAReuse()] implies j.getASuccessor+() = k)
     |
       result = j.getAst()
     )
@@ -79,7 +100,27 @@ class ScanfOutput extends Expr {
   }
 }
 
-//select any(CallInstruction i | i.getStaticCallTarget() instanceof ScanfFunction).toString()
-from ScanfFunctionCall fc
-where fc instanceof ExprInVoidContext
-select fc, "This is a call to scanf."
+/** Returns a block guarded by the assertion of $value $op $call */
+BasicBlock blockGuardedBy(int value, string op, ScanfFunctionCall call) {
+  exists(GuardCondition g, Expr left, Expr right |
+    right = g.getAChild() and
+    value = left.getValue().toInt() and
+    DataFlow::localExprFlow(call, right)
+  |
+    g.ensuresEq(left, right, 0, result, true) and op = "=="
+    or
+    g.ensuresLt(left, right, 0, result, true) and op = "<"
+    or
+    g.ensuresLt(left, right, 1, result, true) and op = "<="
+  )
+}
+
+from ScanfOutput output, ScanfFunctionCall call, ScanfFunction fun, Access access
+where
+  call.getTarget() = fun and
+  output.getCall() = call and
+  output.hasGuardedAccess(access, false)
+select access,
+  "`$@` is read here, but may not have been written. " +
+    "It should be guarded by a check that `$@()` returns at least " + output.getNumber() + ".",
+  access, access.toString(), call, fun.getName()
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
index 937e7197896a..97f8f2ee1da6 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
@@ -22,45 +22,45 @@ int main()
 	{
 		int i;
 
-		scanf("%d", &i); // BAD: may not have written `i`
-		use(i);
+		scanf("%d", &i);
+		use(i); // BAD: may not have written `i`
 	}
 
 	{
 		int i;
 
-		if (scanf("%d", &i) == 1) // GOOD: checks return value
+		if (scanf("%d", &i) == 1)
 		{
-			use(i);
+			use(i); // GOOD: checks return value
 		}
 	}
 
 	{
 		int i = 0;
 
-		scanf("%d", &i); // BAD. Design choice: already initialized variables shouldn't make a difference.
-		use(i);
+		scanf("%d", &i);
+		use(i); // BAD. Design choice: already initialized variables shouldn't make a difference.
 	}
 
 	{
 		int i;
-		use(i);
+		use(i); // GOOD: only care about uses after scanf call
 
-		if (scanf("%d", &i) == 1) // GOOD: only care about uses after scanf call
+		if (scanf("%d", &i) == 1)
 		{
-			use(i);
+			use(i); // GOOD
 		}
 	}
 
 	{
 		int i; // Reused variable
 
-		scanf("%d", &i); // BAD
-		use(i);
+		scanf("%d", &i);
+		use(i); // BAD
 
-		if (scanf("%d", &i) == 1) // GOOD
+		if (scanf("%d", &i) == 1)
 		{
-			use(i);
+			use(i); // GOOD
 		}
 	}
 
@@ -69,15 +69,15 @@ int main()
 	{
 		int i;
 
-		fscanf(get_a_stream(), "%d", &i); // BAD: may not have written `i`
-		use(i);
+		fscanf(get_a_stream(), "%d", &i);
+		use(i); // BAD: may not have written `i`
 	}
 
 	{
 		int i;
 
-		sscanf(get_a_string(), "%d", &i); // BAD: may not have written `i`
-		use(i);
+		sscanf(get_a_string(), "%d", &i);
+		use(i); // BAD: may not have written `i`
 	}
 
 	// --- different ways of checking ---
@@ -85,36 +85,48 @@ int main()
 	{
 		int i;
 
-		if (scanf("%d", &i) >= 1) // GOOD
+		if (scanf("%d", &i) >= 1)
 		{
-			use(i);
+			use(i); // GOOD
 		}
 	}
 
 	{
 		int i;
 
-		if (scanf("%d", &i) == 1) // GOOD
+		if (scanf("%d", &i) == 1)
 		{
-			use(i);
+			use(i); // GOOD
 		}
 	}
 
 	{
 		int i;
 
-		if (scanf("%d", &i) != 0) // BAD: scanf can return -1 [NOT DETECTED]
+		if (0 < scanf("%d", &i))
 		{
-			use(i);
+			if (true)
+			{
+				use(i); // GOOD
+			}
 		}
 	}
 
 	{
 		int i;
 
-		if (scanf("%d", &i) == 0) // BAD: checks return value incorrectly [NOT DETECTED]
+		if (scanf("%d", &i) != 0)
 		{
-			use(i);
+			use(i); // BAD [NOT DETECTED]: scanf can return -1 (EOF)
+		}
+	}
+
+	{
+		int i;
+
+		if (scanf("%d", &i) == 0)
+		{
+			use(i); // BAD [NOT DETECTED]: checks return value incorrectly
 		}
 	}
 
@@ -122,11 +134,11 @@ int main()
 		int r;
 		int i;
 
-		r = scanf("%d", &i); // GOOD
+		r = scanf("%d", &i);
 
 		if (r >= 1)
 		{
-			use(i);
+			use(i); // GOOD
 		}
 	}
 
@@ -134,11 +146,11 @@ int main()
 		bool b;
 		int i;
 
-		b = scanf("%d", &i); // BAD [NOT DETECTED]: scanf can return EOF (boolifies true)
+		b = scanf("%d", &i);
 
 		if (b >= 1)
 		{
-			use(i);
+			use(i); // BAD [NOT DETECTED]: scanf can return EOF (boolifies true)
 		}
 	}
 
@@ -146,28 +158,39 @@ int main()
 		bool b;
 		int i;
 
-		b = scanf("%d", &i); // BAD [NOT DETECTED]
+		b = scanf("%d", &i);
+
+		if (b)
+			use(i); // BAD
+	}
+
+	{
+		int i, j;
 
-		use(i);
+		if (scanf("%d %d", &i) >= 2)
+		{
+			use(i); // GOOD
+			use(j); // GOOD: `j` is not a scanf arg, so out of scope of MissingCheckScanf
+		}
 	}
 
 	{
 		int i, j;
 
-		if (scanf("%d %d", &i) >= 2) // GOOD: `j` is not a scanf arg, so out of scope of MissingCheckScanf
+		if (scanf("%d %d", &i, &j) >= 1)
 		{
-			use(i);
-			use(j);
+			use(i); // GOOD
+			use(j); // BAD: checks return value incorrectly [NOT DETECTED]
 		}
 	}
 
 	{
 		int i, j;
 
-		if (scanf("%d %d", &i, &j) >= 1) // BAD: checks return value incorrectly [NOT DETECTED]
+		if (scanf("%d %d", &i, &j) >= 2)
 		{
-			use(i);
-			use(j);
+			use(i); // GOOD
+			use(j); // GOOD
 		}
 	}
 
@@ -177,24 +200,24 @@ int main()
 		int i;
 		i = 0;
 
-		scanf("%d", &i); // BAD
-		use(i);
+		scanf("%d", &i);
+		use(i); // BAD
 	}
 
 	{
 		int i;
 
 		set_by_ref(i);
-		scanf("%d", &i); // BAD
-		use(i);
+		scanf("%d", &i);
+		use(i); // BAD
 	}
 
 	{
 		int i;
 
 		set_by_ptr(&i);
-		scanf("%d", &i); // BAD
-		use(i);
+		scanf("%d", &i);
+		use(i); // BAD
 	}
 
 	{
@@ -205,8 +228,8 @@ int main()
 			i = 0;
 		}
 
-		scanf("%d", &i); // BAD: `i` may not have been initialized
-		use(i);
+		scanf("%d", &i);
+		use(i); // BAD: `i` may not have been initialized
 	}
 
 	// --- different use ---
@@ -215,23 +238,23 @@ int main()
 		int i;
 		int *ptr_i = &i;
 
-		scanf("%d", &i); // BAD: may not have written `i`
-		use(*ptr_i);
+		scanf("%d", &i);
+		use(*ptr_i); // BAD: may not have written `i`
 	}
 
 	{
 		int i;
 		int *ptr_i = &i;
 
-		scanf("%d", ptr_i); // BAD: may not have written `*ptr_i`
-		use(i);
+		scanf("%d", ptr_i);
+		use(i); // BAD: may not have written `*ptr_i`
 	}
 
 	{
 		int i;
 		scanf("%d", &i);
 		i = 42;
-		use(i); // GOOD [FALSE POSITIVE]
+		use(i); // GOOD
 	}
 
 	// --- weird formatting strings ---
@@ -239,27 +262,27 @@ int main()
 	{
 		int i;
 
-		if (scanf("%n %d", &i) >= 1) // GOOD (`%n` does not consume input)
+		if (scanf("%n %d", &i) >= 1)
 		{
-			use(i);
+			use(i); // GOOD (`%n` does not consume input)
 		}
 	}
 
 	{
 		int i;
 
-		if (scanf("%% %d", &i) >= 1) // GOOD (`%%` does not consume input)
+		if (scanf("%% %d", &i) >= 1)
 		{
-			use(i);
+			use(i); // GOOD (`%%` does not consume input)
 		}
 	}
 
 	{
 		int i;
 
-		if (scanf("%*d %d", &i) >= 1) // GOOD (`%*d` does not consume input)
+		if (scanf("%*d %d", &i) >= 1)
 		{
-			use(i);
+			use(i); // GOOD (`%*d` does not consume input)
 		}
 	}
 }
@@ -277,11 +300,11 @@ void my_scan_int_test()
 
 	use(i); // GOOD: used before scanf
 
-	my_scan_int(i); // BAD [NOT DETECTED]
-	use(i);
+	my_scan_int(i);
+	use(i); // BAD [NOT DETECTED]
 
-	if (my_scan_int(i)) // GOOD
+	if (my_scan_int(i))
 	{
-		use(i);
+		use(i); // GOOD
 	}
 }

From 28331a522351761090e3b05b45d0a0e726fde0c9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Wed, 17 Aug 2022 16:26:53 +0200
Subject: [PATCH 07/17] WIP: More test cases, including False Positives

---
 cpp/ql/src/Critical/MissingCheckScanf.ql      | 60 ++++++++-----------
 .../Critical/MissingCheckScanf/test.cpp       | 44 +++++++++++---
 2 files changed, 60 insertions(+), 44 deletions(-)

diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql
index 62914572348d..e806416f5f10 100644
--- a/cpp/ql/src/Critical/MissingCheckScanf.ql
+++ b/cpp/ql/src/Critical/MissingCheckScanf.ql
@@ -1,12 +1,12 @@
 /**
- * @name TODO
+ * @name Missing return-value check for a scanf-like function
  * @description TODO
  * @kind problem
  * @problem.severity warning
  * @security-severity TODO
  * @precision TODO
  * @id cpp/missing-check-scanf
- * @tags TODO
+ * @tags security
  */
 
 import cpp
@@ -16,27 +16,7 @@ import semmle.code.cpp.dataflow.DataFlow
 import semmle.code.cpp.ir.IR
 import semmle.code.cpp.ir.ValueNumbering
 
-// DataFlow::localFlow(DataFlow::parameterNode(src), DataFlow::exprNode(snk))
-/*
- * Find:
- * - all scanf calls
- *   => ScanfFunctionCall
- *
- * - relate the value number of each out-arg to:
- *   1. the return value of the scanf call, and
- *   2. the arg number, post-format, of the out-arg.
- *   => scanfOutput/3
- *
- * - Find all accesses to variables with the same
- *   value number, and if applicable, the guard condition
- *   of one of their enclosing blocks that ensures
- *   that the scanf return value is at least the
- *   associated arg-number
- *
- * - Combine into data flow tracking from out-arg to use,
- *   with usage in _other_ scanf calls as barrier.
- */
-
+/** An expression appearing as an output argument to a `scanf`-like call */
 class ScanfOutput extends Expr {
   ScanfFunctionCall call;
   int argNum;
@@ -66,24 +46,24 @@ class ScanfOutput extends Expr {
 
   /**
    * Get a subsequent access of the same underlying storage,
-   * but before it gets reset or reused in another scanf call.
+   * but before it gets reset or reused in another `scanf` call.
    */
   Access getAnAccess() {
-    exists(Instruction j |
-      j = getNextInstruction() and
-      forall(Instruction k | k = [getAReset(), getAReuse()] implies j.getASuccessor+() = k)
-    |
-      result = j.getAst()
+    exists(Instruction j | result = j.getAst() |
+      j = getASubsequentSameValuedInstruction() and
+      forall(Instruction k |
+        k = [getAResetInstruction(), getAReuseInstruction()] implies j.getASuccessor+() = k
+      )
     )
   }
 
-  private Instruction getAReset() {
-    result = getNextInstruction() and
+  private Instruction getAResetInstruction() {
+    result = getASubsequentSameValuedInstruction() and
     result = any(StoreInstruction s).getDestinationAddress()
   }
 
-  private Instruction getAReuse() {
-    result = getNextInstruction() and
+  private Instruction getAReuseInstruction() {
+    result = getASubsequentSameValuedInstruction() and
     exists(Expr e | result.getAst() = e |
       e instanceof ScanfOutput
       or
@@ -91,13 +71,21 @@ class ScanfOutput extends Expr {
     )
   }
 
-  private Instruction getNextInstruction() {
+  private Instruction getASubsequentSameValuedInstruction() {
     exists(Instruction i |
       i.getUnconvertedResultExpression() = this and
       result = valueNumber(i).getAnInstruction() and
       i.getASuccessor+() = result
     )
   }
+
+  private predicate foo(Instruction i, Instruction j, boolean sameValueNum, boolean sameAst) {
+    i.getUnconvertedResultExpression() = this and
+    i.getASuccessor+() = j and
+    (if valueNumber(i) = valueNumber(j) then sameValueNum = true else sameValueNum = false) and
+    (if i.getAst() = j.getAst() then sameAst = true else sameAst = false) and
+    (sameValueNum = true and sameAst = true)
+  }
 }
 
 /** Returns a block guarded by the assertion of $value $op $call */
@@ -121,6 +109,6 @@ where
   output.getCall() = call and
   output.hasGuardedAccess(access, false)
 select access,
-  "`$@` is read here, but may not have been written. " +
-    "It should be guarded by a check that `$@()` returns at least " + output.getNumber() + ".",
+  "$@ is read here, but may not have been written. " +
+    "It should be guarded by a check that $@() returns at least " + output.getNumber() + ".",
   access, access.toString(), call, fun.getName()
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
index 97f8f2ee1da6..fa8379705004 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
@@ -2,9 +2,12 @@ typedef struct
 {
 } FILE;
 
+typedef void *locale_t;
+
 int scanf(const char *format, ...);
 int fscanf(FILE *stream, const char *format, ...);
 int sscanf(const char *s, const char *format, ...);
+int _scanf_l(const char *format, locale_t locale, ...);
 
 void use(int i);
 
@@ -14,6 +17,7 @@ bool maybe();
 
 FILE *get_a_stream();
 const char *get_a_string();
+extern locale_t get_a_locale();
 
 int main()
 {
@@ -80,6 +84,15 @@ int main()
 		use(i); // BAD: may not have written `i`
 	}
 
+	{
+		int i;
+
+		if (_scanf_l("%d", get_a_locale(), &i) == 1)
+		{
+			use(i); // GOOD [FALSE POSITIVE]
+		}
+	}
+
 	// --- different ways of checking ---
 
 	{
@@ -117,7 +130,7 @@ int main()
 
 		if (scanf("%d", &i) != 0)
 		{
-			use(i); // BAD [NOT DETECTED]: scanf can return -1 (EOF)
+			use(i); // BAD: scanf can return EOF
 		}
 	}
 
@@ -126,7 +139,7 @@ int main()
 
 		if (scanf("%d", &i) == 0)
 		{
-			use(i); // BAD [NOT DETECTED]: checks return value incorrectly
+			use(i); // BAD: checks return value incorrectly
 		}
 	}
 
@@ -155,12 +168,9 @@ int main()
 	}
 
 	{
-		bool b;
 		int i;
 
-		b = scanf("%d", &i);
-
-		if (b)
+		if (scanf("%d", &i))
 			use(i); // BAD
 	}
 
@@ -180,7 +190,7 @@ int main()
 		if (scanf("%d %d", &i, &j) >= 1)
 		{
 			use(i); // GOOD
-			use(j); // BAD: checks return value incorrectly [NOT DETECTED]
+			use(j); // BAD: checks return value incorrectly
 		}
 	}
 
@@ -264,7 +274,7 @@ int main()
 
 		if (scanf("%n %d", &i) >= 1)
 		{
-			use(i); // GOOD (`%n` does not consume input)
+			use(i); // GOOD (`%n` does not consume input, but writes to i)
 		}
 	}
 
@@ -285,6 +295,24 @@ int main()
 			use(i); // GOOD (`%*d` does not consume input)
 		}
 	}
+
+	{
+		int d, n;
+
+		if (scanf("%*d %d %n", &d, &n) == 1) {
+			use(d); // GOOD
+			use(n); // GOOD [FALSE POSITIVE]
+		}
+	}
+
+	{
+		char substr[32];
+		int n;
+        if (sscanf(get_a_string(), "%31[^:]: %d", substr, &n) == 2) {
+			use(*(int *)substr); // GOOD
+            use(n); // GOOD
+		}
+	}
 }
 
 // Non-local cases:

From b380e9ae6c22b5d93c005d16dddce05848deb014 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Wed, 17 Aug 2022 16:27:33 +0200
Subject: [PATCH 08/17] .clang-format: do not autoformat test.cpp

---
 cpp/ql/test/query-tests/Critical/MissingCheckScanf/.clang-format | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 cpp/ql/test/query-tests/Critical/MissingCheckScanf/.clang-format

diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/.clang-format b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/.clang-format
new file mode 100644
index 000000000000..e3845288a2ae
--- /dev/null
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/.clang-format
@@ -0,0 +1 @@
+DisableFormat: true

From 9b4b7cb1b15f0693b17db4b3e8dced6881b6ae65 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Wed, 17 Aug 2022 17:21:10 +0200
Subject: [PATCH 09/17] WIP: Eliminated the _scanf_l false positive

---
 cpp/ql/src/Critical/MissingCheckScanf.ql      | 35 +++++++++++++------
 .../Critical/MissingCheckScanf/test.cpp       |  2 +-
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql
index e806416f5f10..a8bfa43b5ccd 100644
--- a/cpp/ql/src/Critical/MissingCheckScanf.ql
+++ b/cpp/ql/src/Critical/MissingCheckScanf.ql
@@ -19,26 +19,37 @@ import semmle.code.cpp.ir.ValueNumbering
 /** An expression appearing as an output argument to a `scanf`-like call */
 class ScanfOutput extends Expr {
   ScanfFunctionCall call;
-  int argNum;
+  int varargIndex;
 
   ScanfOutput() {
-    this = call.getArgument(call.getFormatParameterIndex() + argNum) and
-    argNum >= 1
+    this = call.getArgument(call.getTarget().getNumberOfParameters() + varargIndex) and
+    varargIndex >= 0
   }
 
   ScanfFunctionCall getCall() { result = call }
 
-  int getNumber() { result = argNum }
+  /**
+   * The 0-based index of this output argument in the vararg list of its
+   * enclosing `scanf`-like function call.
+   */
+  int getVarargIndex() { result = varargIndex }
+
+  /**
+   * Any subsequent use of this argument should be surrounded by a
+   * check ensuring that the `scanf`-like function has returned a value
+   * equal to at least `getMinimumGuardConstant()`.
+   */
+  int getMinimumGuardConstant() { result = varargIndex + 1 }
 
   predicate hasGuardedAccess(Access e, boolean isGuarded) {
     e = getAnAccess() and
     if
-      exists(int value |
-        e.getBasicBlock() = blockGuardedBy(value, "==", call) and argNum <= value
+      exists(int value, int minGuard | minGuard = getMinimumGuardConstant() |
+        e.getBasicBlock() = blockGuardedBy(value, "==", call) and minGuard <= value
         or
-        e.getBasicBlock() = blockGuardedBy(value, "<", call) and argNum - 1 <= value
+        e.getBasicBlock() = blockGuardedBy(value, "<", call) and minGuard - 1 <= value
         or
-        e.getBasicBlock() = blockGuardedBy(value, "<=", call) and argNum <= value
+        e.getBasicBlock() = blockGuardedBy(value, "<=", call) and minGuard <= value
       )
     then isGuarded = true
     else isGuarded = false
@@ -103,6 +114,10 @@ BasicBlock blockGuardedBy(int value, string op, ScanfFunctionCall call) {
   )
 }
 
+predicate foo(ScanfFunctionCall call, int nargs) {
+  nargs = call.getTarget().getNumberOfParameters()
+}
+
 from ScanfOutput output, ScanfFunctionCall call, ScanfFunction fun, Access access
 where
   call.getTarget() = fun and
@@ -110,5 +125,5 @@ where
   output.hasGuardedAccess(access, false)
 select access,
   "$@ is read here, but may not have been written. " +
-    "It should be guarded by a check that $@() returns at least " + output.getNumber() + ".",
-  access, access.toString(), call, fun.getName()
+    "It should be guarded by a check that $@() returns at least " + output.getMinimumGuardConstant()
+    + ".", access, access.toString(), call, fun.getName()
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
index fa8379705004..8acd4b022938 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
@@ -89,7 +89,7 @@ int main()
 
 		if (_scanf_l("%d", get_a_locale(), &i) == 1)
 		{
-			use(i); // GOOD [FALSE POSITIVE]
+			use(i); // GOOD
 		}
 	}
 

From ea033f53fab3ecd74124b40d57d64f63ea0aa676 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Wed, 17 Aug 2022 18:13:26 +0200
Subject: [PATCH 10/17] WIP: Fix the %n-related False Positive

---
 cpp/ql/src/Critical/MissingCheckScanf.ql      | 18 +++++--------
 .../MissingCheckScanf.expected                | 26 ++++++++++++-------
 .../Critical/MissingCheckScanf/test.cpp       |  9 ++++---
 3 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql
index a8bfa43b5ccd..761bae50463e 100644
--- a/cpp/ql/src/Critical/MissingCheckScanf.ql
+++ b/cpp/ql/src/Critical/MissingCheckScanf.ql
@@ -28,18 +28,18 @@ class ScanfOutput extends Expr {
 
   ScanfFunctionCall getCall() { result = call }
 
-  /**
-   * The 0-based index of this output argument in the vararg list of its
-   * enclosing `scanf`-like function call.
-   */
-  int getVarargIndex() { result = varargIndex }
-
   /**
    * Any subsequent use of this argument should be surrounded by a
    * check ensuring that the `scanf`-like function has returned a value
    * equal to at least `getMinimumGuardConstant()`.
    */
-  int getMinimumGuardConstant() { result = varargIndex + 1 }
+  int getMinimumGuardConstant() {
+    result =
+      varargIndex + 1 -
+        count(ScanfFormatLiteral f, int n |
+          n <= varargIndex and f.getUse() = call and f.parseConvSpec(n, _, _, _, "n")
+        )
+  }
 
   predicate hasGuardedAccess(Access e, boolean isGuarded) {
     e = getAnAccess() and
@@ -114,10 +114,6 @@ BasicBlock blockGuardedBy(int value, string op, ScanfFunctionCall call) {
   )
 }
 
-predicate foo(ScanfFunctionCall call, int nargs) {
-  nargs = call.getTarget().getNumberOfParameters()
-}
-
 from ScanfOutput output, ScanfFunctionCall call, ScanfFunction fun, Access access
 where
   call.getTarget() = fun and
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
index 6db509fcd9de..0eb4d1f53b1a 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
@@ -1,10 +1,16 @@
-| test.cpp:23:3:23:7 | call to scanf | This is a call to scanf. |
-| test.cpp:39:3:39:7 | call to scanf | This is a call to scanf. |
-| test.cpp:56:3:56:7 | call to scanf | This is a call to scanf. |
-| test.cpp:70:3:70:8 | call to fscanf | This is a call to scanf. |
-| test.cpp:77:3:77:8 | call to sscanf | This is a call to scanf. |
-| test.cpp:178:3:178:7 | call to scanf | This is a call to scanf. |
-| test.cpp:186:3:186:7 | call to scanf | This is a call to scanf. |
-| test.cpp:194:3:194:7 | call to scanf | This is a call to scanf. |
-| test.cpp:206:3:206:7 | call to scanf | This is a call to scanf. |
-| test.cpp:216:3:216:7 | call to scanf | This is a call to scanf. |
+WARNING: Unused method foo (/Users/d10c/src/d10c-codeql/cpp/ql/src/Critical/MissingCheckScanf.ql:93,21-24)
+| test.cpp:30:7:30:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:30:7:30:7 | i | i | test.cpp:29:3:29:7 | call to scanf | scanf |
+| test.cpp:46:7:46:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:46:7:46:7 | i | i | test.cpp:45:3:45:7 | call to scanf | scanf |
+| test.cpp:63:7:63:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:63:7:63:7 | i | i | test.cpp:62:3:62:7 | call to scanf | scanf |
+| test.cpp:77:7:77:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:77:7:77:7 | i | i | test.cpp:76:3:76:8 | call to fscanf | fscanf |
+| test.cpp:84:7:84:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:84:7:84:7 | i | i | test.cpp:83:3:83:8 | call to sscanf | sscanf |
+| test.cpp:133:8:133:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:133:8:133:8 | i | i | test.cpp:131:7:131:11 | call to scanf | scanf |
+| test.cpp:142:8:142:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:142:8:142:8 | i | i | test.cpp:140:7:140:11 | call to scanf | scanf |
+| test.cpp:174:8:174:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:174:8:174:8 | i | i | test.cpp:173:7:173:11 | call to scanf | scanf |
+| test.cpp:193:8:193:8 | j | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 2. | test.cpp:193:8:193:8 | j | j | test.cpp:190:7:190:11 | call to scanf | scanf |
+| test.cpp:214:7:214:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:214:7:214:7 | i | i | test.cpp:213:3:213:7 | call to scanf | scanf |
+| test.cpp:222:7:222:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:222:7:222:7 | i | i | test.cpp:221:3:221:7 | call to scanf | scanf |
+| test.cpp:230:7:230:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:230:7:230:7 | i | i | test.cpp:229:3:229:7 | call to scanf | scanf |
+| test.cpp:242:7:242:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:242:7:242:7 | i | i | test.cpp:241:3:241:7 | call to scanf | scanf |
+| test.cpp:252:8:252:12 | ptr_i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:252:8:252:12 | ptr_i | ptr_i | test.cpp:251:3:251:7 | call to scanf | scanf |
+| test.cpp:260:7:260:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:260:7:260:7 | i | i | test.cpp:259:3:259:7 | call to scanf | scanf |
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
index 8acd4b022938..16be8d413eb0 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
@@ -270,11 +270,12 @@ int main()
 	// --- weird formatting strings ---
 
 	{
-		int i;
+		int i, j;
 
-		if (scanf("%n %d", &i) >= 1)
+		if (sscanf("123", "%n %*d %n", &i, &j) >= 0)
 		{
-			use(i); // GOOD (`%n` does not consume input, but writes to i)
+			use(i); // GOOD (`%n` does not consume input, but writes 0 to i)
+			use(j); // GOOD (`%n` does not consume input, but writes 3 to j)
 		}
 	}
 
@@ -301,7 +302,7 @@ int main()
 
 		if (scanf("%*d %d %n", &d, &n) == 1) {
 			use(d); // GOOD
-			use(n); // GOOD [FALSE POSITIVE]
+			use(n); // GOOD
 		}
 	}
 

From db04f77dda88e8da70b24fe80048afa27aa3042d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Wed, 17 Aug 2022 18:25:59 +0200
Subject: [PATCH 11/17] WIP: Add test for loop-related False Positive

The problem with the current query is that instructions can be in a
cycle, so the scanf argument itself is viewed as an unguarded use.
---
 cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
index 16be8d413eb0..b65999375d05 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
@@ -309,7 +309,7 @@ int main()
 	{
 		char substr[32];
 		int n;
-        if (sscanf(get_a_string(), "%31[^:]: %d", substr, &n) == 2) {
+        while (sscanf(get_a_string(), "%31[^:]: %d", substr, &n) == 2) { // GOOD [FALSE POSITIVE]: there is a cycle from write to unguarded access!
 			use(*(int *)substr); // GOOD
             use(n); // GOOD
 		}

From f7800f2972bed374beef76375a247f6ac921c143 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Wed, 17 Aug 2022 20:30:37 +0200
Subject: [PATCH 12/17] WIP: Fix loop-related False Positive

---
 cpp/ql/src/Critical/MissingCheckScanf.ql              | 11 ++---------
 .../MissingCheckScanf/MissingCheckScanf.expected      |  1 -
 .../query-tests/Critical/MissingCheckScanf/test.cpp   |  2 +-
 3 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql
index 761bae50463e..ea4388bac447 100644
--- a/cpp/ql/src/Critical/MissingCheckScanf.ql
+++ b/cpp/ql/src/Critical/MissingCheckScanf.ql
@@ -86,17 +86,10 @@ class ScanfOutput extends Expr {
     exists(Instruction i |
       i.getUnconvertedResultExpression() = this and
       result = valueNumber(i).getAnInstruction() and
-      i.getASuccessor+() = result
+      i.getASuccessor+() = result and
+      forall(Expr child | child = this.getAChild*() implies child != result.getAst())
     )
   }
-
-  private predicate foo(Instruction i, Instruction j, boolean sameValueNum, boolean sameAst) {
-    i.getUnconvertedResultExpression() = this and
-    i.getASuccessor+() = j and
-    (if valueNumber(i) = valueNumber(j) then sameValueNum = true else sameValueNum = false) and
-    (if i.getAst() = j.getAst() then sameAst = true else sameAst = false) and
-    (sameValueNum = true and sameAst = true)
-  }
 }
 
 /** Returns a block guarded by the assertion of $value $op $call */
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
index 0eb4d1f53b1a..1bd2e18d34bb 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
@@ -1,4 +1,3 @@
-WARNING: Unused method foo (/Users/d10c/src/d10c-codeql/cpp/ql/src/Critical/MissingCheckScanf.ql:93,21-24)
 | test.cpp:30:7:30:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:30:7:30:7 | i | i | test.cpp:29:3:29:7 | call to scanf | scanf |
 | test.cpp:46:7:46:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:46:7:46:7 | i | i | test.cpp:45:3:45:7 | call to scanf | scanf |
 | test.cpp:63:7:63:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:63:7:63:7 | i | i | test.cpp:62:3:62:7 | call to scanf | scanf |
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
index b65999375d05..588e4d4e3e86 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
@@ -309,7 +309,7 @@ int main()
 	{
 		char substr[32];
 		int n;
-        while (sscanf(get_a_string(), "%31[^:]: %d", substr, &n) == 2) { // GOOD [FALSE POSITIVE]: there is a cycle from write to unguarded access!
+        while (sscanf(get_a_string(), "%31[^:]: %d", substr, &n) == 2) { // GOOD: cycle from write to unguarded access
 			use(*(int *)substr); // GOOD
             use(n); // GOOD
 		}

From 0132865471c903cc27799921180ec124dcbe77d3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Wed, 17 Aug 2022 21:20:56 +0200
Subject: [PATCH 13/17] WIP: Added a test case exercising the last uwsgi FP

The other problem is that this query is slow on a real database, like
unbit/uwsgi. Particularly during the step called:

ScanfOutput::getASubsequentSameValuedInstruction#dispred#f0820431#ff#antijoin_rhs#2

Perhaps it can be optimized further.
---
 .../MissingCheckScanf.expected                |  1 +
 .../Critical/MissingCheckScanf/test.cpp       | 21 ++++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
index 1bd2e18d34bb..55baf109c266 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
@@ -13,3 +13,4 @@
 | test.cpp:242:7:242:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:242:7:242:7 | i | i | test.cpp:241:3:241:7 | call to scanf | scanf |
 | test.cpp:252:8:252:12 | ptr_i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:252:8:252:12 | ptr_i | ptr_i | test.cpp:251:3:251:7 | call to scanf | scanf |
 | test.cpp:260:7:260:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:260:7:260:7 | i | i | test.cpp:259:3:259:7 | call to scanf | scanf |
+| test.cpp:354:25:354:25 | u | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:354:25:354:25 | u | u | test.cpp:353:6:353:11 | call to sscanf | sscanf |
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
index 588e4d4e3e86..40886478fb39 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
@@ -316,7 +316,7 @@ int main()
 	}
 }
 
-// Non-local cases:
+// --- Non-local cases ---
 
 bool my_scan_int(int &i)
 {
@@ -337,3 +337,22 @@ void my_scan_int_test()
 		use(i); // GOOD
 	}
 }
+
+// --- Can be OK'd given a sufficiently smart analysis ---
+
+char *my_string_copy() {
+	static const char SRC_STRING[] = "48656C6C6F";
+	static       char DST_STRING[] = ".....";
+
+	int len = sizeof(SRC_STRING) - 1;
+    const char *src = SRC_STRING;
+    char *ptr = DST_STRING;
+
+    for (int i = 0; i < len; i += 2) {
+		unsigned int u;
+	    sscanf(src + i, "%2x", &u);
+        *ptr++ = (char) u; // GOOD [FALSE POSITIVE]: src+i+{0,1} are always valid %x digits, so this should be OK.
+    }
+	*ptr++ = 0;
+	return DST_STRING;
+}

From 230f411e2a1a7d72561df06217ef4d2e8956c2d8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Thu, 18 Aug 2022 15:12:05 +0200
Subject: [PATCH 14/17] WIP: use explicit `this.` in method calls

To quiet the QL-on-QL "implicit this" warnings.
---
 cpp/ql/src/Critical/MissingCheckScanf.ql | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql
index ea4388bac447..7bda45b74280 100644
--- a/cpp/ql/src/Critical/MissingCheckScanf.ql
+++ b/cpp/ql/src/Critical/MissingCheckScanf.ql
@@ -42,9 +42,9 @@ class ScanfOutput extends Expr {
   }
 
   predicate hasGuardedAccess(Access e, boolean isGuarded) {
-    e = getAnAccess() and
+    e = this.getAnAccess() and
     if
-      exists(int value, int minGuard | minGuard = getMinimumGuardConstant() |
+      exists(int value, int minGuard | minGuard = this.getMinimumGuardConstant() |
         e.getBasicBlock() = blockGuardedBy(value, "==", call) and minGuard <= value
         or
         e.getBasicBlock() = blockGuardedBy(value, "<", call) and minGuard - 1 <= value
@@ -61,20 +61,22 @@ class ScanfOutput extends Expr {
    */
   Access getAnAccess() {
     exists(Instruction j | result = j.getAst() |
-      j = getASubsequentSameValuedInstruction() and
+      j = this.getASubsequentSameValuedInstruction() and
       forall(Instruction k |
-        k = [getAResetInstruction(), getAReuseInstruction()] implies j.getASuccessor+() = k
+        k = [this.getAResetInstruction(), this.getAReuseInstruction()]
+        implies
+        j.getASuccessor+() = k
       )
     )
   }
 
   private Instruction getAResetInstruction() {
-    result = getASubsequentSameValuedInstruction() and
+    result = this.getASubsequentSameValuedInstruction() and
     result = any(StoreInstruction s).getDestinationAddress()
   }
 
   private Instruction getAReuseInstruction() {
-    result = getASubsequentSameValuedInstruction() and
+    result = this.getASubsequentSameValuedInstruction() and
     exists(Expr e | result.getAst() = e |
       e instanceof ScanfOutput
       or

From 63a64ee87f1f72370903187f7a9f3dbbe9e129d3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Fri, 19 Aug 2022 11:26:37 +0200
Subject: [PATCH 15/17] WIP: Fix bad join order.

Offender: getASameValuedInstruction

This reduced its runtime on unbit/uwsgi from 1+min to 10ms.

Re-running MRVA top 100, and it still looks like there are a few DBs on
which it takes over 30 minutes to run this query. So further
optimization is warranted.
---
 cpp/ql/src/Critical/MissingCheckScanf.ql | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql
index 7bda45b74280..f01d473e7ef9 100644
--- a/cpp/ql/src/Critical/MissingCheckScanf.ql
+++ b/cpp/ql/src/Critical/MissingCheckScanf.ql
@@ -87,10 +87,10 @@ class ScanfOutput extends Expr {
   private Instruction getASubsequentSameValuedInstruction() {
     exists(Instruction i |
       i.getUnconvertedResultExpression() = this and
-      result = valueNumber(i).getAnInstruction() and
       i.getASuccessor+() = result and
-      forall(Expr child | child = this.getAChild*() implies child != result.getAst())
-    )
+      valueNumber(i) = valueNumber(result)
+    ) and
+    not exists(Expr child | child = this.getAChild*() | child = result.getAst())
   }
 }
 

From 362980b2000e80e7d1ca8e10017c147326080e69 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Fri, 19 Aug 2022 12:18:23 +0200
Subject: [PATCH 16/17] WIP: Found another false positive via opencv/opencv

---
 .../Critical/MissingCheckScanf/test.cpp          | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
index 40886478fb39..f0c953c11b6d 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
@@ -204,6 +204,22 @@ int main()
 		}
 	}
 
+	{
+		int i;
+
+		while(maybe()) {
+			if (maybe()) {
+				break;
+			}
+			else if (maybe() && (scanf("%d", &i) == 1)) { // GOOD [FALSE POSITIVE]
+				use(i); // GOOD [FALSE POSITIVE]
+			}
+			else if ((scanf("%d", &i) == 1) && maybe()) { // GOOD [FALSE POSITIVE]
+				use(i); // GOOD [FALSE POSITIVE]
+			}
+		}
+	}
+
 	// --- different initialization ---
 
 	{

From 0af6160ef091b2434ba9ab41964ec568a4ae960f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= <d10c@users.noreply.github.com>
Date: Wed, 24 Aug 2022 00:06:34 +0200
Subject: [PATCH 17/17] WIP: Fixed the loop-related FP !!

The query is now shorter, simpler, more correct, and after some local
testing, faster.

MRVA top 100 and DCA runs pending.
---
 cpp/ql/src/Critical/MissingCheckScanf.ql      | 52 ++++++++++---------
 .../MissingCheckScanf.expected                | 29 ++++++-----
 .../Critical/MissingCheckScanf/test.cpp       | 25 ++++++---
 3 files changed, 62 insertions(+), 44 deletions(-)

diff --git a/cpp/ql/src/Critical/MissingCheckScanf.ql b/cpp/ql/src/Critical/MissingCheckScanf.ql
index f01d473e7ef9..8dcde98633f3 100644
--- a/cpp/ql/src/Critical/MissingCheckScanf.ql
+++ b/cpp/ql/src/Critical/MissingCheckScanf.ql
@@ -20,10 +20,17 @@ import semmle.code.cpp.ir.ValueNumbering
 class ScanfOutput extends Expr {
   ScanfFunctionCall call;
   int varargIndex;
+  Instruction instr;
+  ValueNumber valNum;
 
   ScanfOutput() {
     this = call.getArgument(call.getTarget().getNumberOfParameters() + varargIndex) and
-    varargIndex >= 0
+    varargIndex >= 0 and
+    instr.getUnconvertedResultExpression() = this and
+    valueNumber(instr) = valNum and
+    // The following line is a kludge to prohibit more than one associated `instr` field,
+    // as would occur, for example, when `this` is an access to an array variable.
+    not instr instanceof ConvertInstruction
   }
 
   ScanfFunctionCall getCall() { result = call }
@@ -60,37 +67,32 @@ class ScanfOutput extends Expr {
    * but before it gets reset or reused in another `scanf` call.
    */
   Access getAnAccess() {
-    exists(Instruction j | result = j.getAst() |
-      j = this.getASubsequentSameValuedInstruction() and
-      forall(Instruction k |
-        k = [this.getAResetInstruction(), this.getAReuseInstruction()]
-        implies
-        j.getASuccessor+() = k
-      )
+    exists(Instruction dst |
+      this.bigStep(instr) = dst and
+      dst.getAst() = result and
+      valueNumber(dst) = valNum
     )
   }
 
-  private Instruction getAResetInstruction() {
-    result = this.getASubsequentSameValuedInstruction() and
-    result = any(StoreInstruction s).getDestinationAddress()
+  private Instruction bigStep(Instruction i) {
+    result = this.smallStep(i)
+    or
+    exists(Instruction j | j = this.bigStep(i) | result = this.smallStep(j))
   }
 
-  private Instruction getAReuseInstruction() {
-    result = this.getASubsequentSameValuedInstruction() and
-    exists(Expr e | result.getAst() = e |
-      e instanceof ScanfOutput
-      or
-      e.getParent().(AddressOfExpr) instanceof ScanfOutput
-    )
+  private Instruction smallStep(Instruction i) {
+    instr.getASuccessor*() = i and
+    i.getASuccessor() = result and
+    not this.isBarrier(result)
   }
 
-  private Instruction getASubsequentSameValuedInstruction() {
-    exists(Instruction i |
-      i.getUnconvertedResultExpression() = this and
-      i.getASuccessor+() = result and
-      valueNumber(i) = valueNumber(result)
-    ) and
-    not exists(Expr child | child = this.getAChild*() | child = result.getAst())
+  private predicate isBarrier(Instruction i) {
+    valueNumber(i) = valNum and
+    exists(Expr e | i.getAst() = e |
+      i = any(StoreInstruction s).getDestinationAddress()
+      or
+      [e, e.getParent().(AddressOfExpr)] instanceof ScanfOutput
+    )
   }
 }
 
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
index 55baf109c266..cb36c25b0e94 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/MissingCheckScanf.expected
@@ -1,16 +1,19 @@
 | test.cpp:30:7:30:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:30:7:30:7 | i | i | test.cpp:29:3:29:7 | call to scanf | scanf |
 | test.cpp:46:7:46:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:46:7:46:7 | i | i | test.cpp:45:3:45:7 | call to scanf | scanf |
 | test.cpp:63:7:63:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:63:7:63:7 | i | i | test.cpp:62:3:62:7 | call to scanf | scanf |
-| test.cpp:77:7:77:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:77:7:77:7 | i | i | test.cpp:76:3:76:8 | call to fscanf | fscanf |
-| test.cpp:84:7:84:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:84:7:84:7 | i | i | test.cpp:83:3:83:8 | call to sscanf | sscanf |
-| test.cpp:133:8:133:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:133:8:133:8 | i | i | test.cpp:131:7:131:11 | call to scanf | scanf |
-| test.cpp:142:8:142:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:142:8:142:8 | i | i | test.cpp:140:7:140:11 | call to scanf | scanf |
-| test.cpp:174:8:174:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:174:8:174:8 | i | i | test.cpp:173:7:173:11 | call to scanf | scanf |
-| test.cpp:193:8:193:8 | j | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 2. | test.cpp:193:8:193:8 | j | j | test.cpp:190:7:190:11 | call to scanf | scanf |
-| test.cpp:214:7:214:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:214:7:214:7 | i | i | test.cpp:213:3:213:7 | call to scanf | scanf |
-| test.cpp:222:7:222:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:222:7:222:7 | i | i | test.cpp:221:3:221:7 | call to scanf | scanf |
-| test.cpp:230:7:230:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:230:7:230:7 | i | i | test.cpp:229:3:229:7 | call to scanf | scanf |
-| test.cpp:242:7:242:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:242:7:242:7 | i | i | test.cpp:241:3:241:7 | call to scanf | scanf |
-| test.cpp:252:8:252:12 | ptr_i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:252:8:252:12 | ptr_i | ptr_i | test.cpp:251:3:251:7 | call to scanf | scanf |
-| test.cpp:260:7:260:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:260:7:260:7 | i | i | test.cpp:259:3:259:7 | call to scanf | scanf |
-| test.cpp:354:25:354:25 | u | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:354:25:354:25 | u | u | test.cpp:353:6:353:11 | call to sscanf | sscanf |
+| test.cpp:75:7:75:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:75:7:75:7 | i | i | test.cpp:74:3:74:7 | call to scanf | scanf |
+| test.cpp:87:7:87:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:87:7:87:7 | i | i | test.cpp:86:3:86:8 | call to fscanf | fscanf |
+| test.cpp:94:7:94:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:94:7:94:7 | i | i | test.cpp:93:3:93:8 | call to sscanf | sscanf |
+| test.cpp:143:8:143:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:143:8:143:8 | i | i | test.cpp:141:7:141:11 | call to scanf | scanf |
+| test.cpp:152:8:152:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:152:8:152:8 | i | i | test.cpp:150:7:150:11 | call to scanf | scanf |
+| test.cpp:184:8:184:8 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:184:8:184:8 | i | i | test.cpp:183:7:183:11 | call to scanf | scanf |
+| test.cpp:203:8:203:8 | j | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 2. | test.cpp:203:8:203:8 | j | j | test.cpp:200:7:200:11 | call to scanf | scanf |
+| test.cpp:227:9:227:9 | d | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 2. | test.cpp:227:9:227:9 | d | d | test.cpp:225:25:225:29 | call to scanf | scanf |
+| test.cpp:231:9:231:9 | d | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 2. | test.cpp:231:9:231:9 | d | d | test.cpp:229:14:229:18 | call to scanf | scanf |
+| test.cpp:243:7:243:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:243:7:243:7 | i | i | test.cpp:242:3:242:7 | call to scanf | scanf |
+| test.cpp:251:7:251:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:251:7:251:7 | i | i | test.cpp:250:3:250:7 | call to scanf | scanf |
+| test.cpp:259:7:259:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:259:7:259:7 | i | i | test.cpp:258:3:258:7 | call to scanf | scanf |
+| test.cpp:271:7:271:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:271:7:271:7 | i | i | test.cpp:270:3:270:7 | call to scanf | scanf |
+| test.cpp:281:8:281:12 | ptr_i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:281:8:281:12 | ptr_i | ptr_i | test.cpp:280:3:280:7 | call to scanf | scanf |
+| test.cpp:289:7:289:7 | i | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:289:7:289:7 | i | i | test.cpp:288:3:288:7 | call to scanf | scanf |
+| test.cpp:383:25:383:25 | u | $@ is read here, but may not have been written. It should be guarded by a check that $@() returns at least 1. | test.cpp:383:25:383:25 | u | u | test.cpp:382:6:382:11 | call to sscanf | sscanf |
diff --git a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
index f0c953c11b6d..087999a41b8e 100644
--- a/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
+++ b/cpp/ql/test/query-tests/Critical/MissingCheckScanf/test.cpp
@@ -68,6 +68,16 @@ int main()
 		}
 	}
 
+	{
+		int i; // Reset variable
+
+		scanf("%d", &i);
+		use(i); // BAD
+
+		i = 1;
+		use(i); // GOOD
+	}
+
 	// --- different scanf functions ---
 
 	{
@@ -205,17 +215,20 @@ int main()
 	}
 
 	{
-		int i;
+		char c[5];
+		int d;
 
 		while(maybe()) {
 			if (maybe()) {
 				break;
 			}
-			else if (maybe() && (scanf("%d", &i) == 1)) { // GOOD [FALSE POSITIVE]
-				use(i); // GOOD [FALSE POSITIVE]
+			else if (maybe() && (scanf("%5c %d", c, &d) == 1)) { // GOOD [FALSE POSITIVE]
+				use(*(int *)c); // GOOD
+				use(d); // BAD
 			}
-			else if ((scanf("%d", &i) == 1) && maybe()) { // GOOD [FALSE POSITIVE]
-				use(i); // GOOD [FALSE POSITIVE]
+			else if ((scanf("%5c %d", c, &d) == 1) && maybe()) { // GOOD [FALSE POSITIVE]
+				use(*(int *)c); // GOOD
+				use(d); // BAD
 			}
 		}
 	}
@@ -367,7 +380,7 @@ char *my_string_copy() {
     for (int i = 0; i < len; i += 2) {
 		unsigned int u;
 	    sscanf(src + i, "%2x", &u);
-        *ptr++ = (char) u; // GOOD [FALSE POSITIVE]: src+i+{0,1} are always valid %x digits, so this should be OK.
+        *ptr++ = (char) u; // GOOD [FALSE POSITIVE]? src+i+{0,1} are always valid %x digits, so this should be OK.
     }
 	*ptr++ = 0;
 	return DST_STRING;