From 760884051cb2f1d5e9afdcb343f52890103dc8c1 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Tue, 29 Oct 2019 16:14:34 +0000
Subject: [PATCH 1/8] CPP: Add test cases using various combinations of width
 and precision specifiers, positional arguments, and flags.

---
 .../WrongTypeFormatArguments.expected         | 31 +++++++
 .../Linux_signed_chars/printf1.h              | 83 +++++++++++++++++++
 2 files changed, 114 insertions(+)

diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/WrongTypeFormatArguments.expected b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/WrongTypeFormatArguments.expected
index 65f4f546976..441b62b1c2d 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/WrongTypeFormatArguments.expected	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/WrongTypeFormatArguments.expected	
@@ -16,6 +16,37 @@
 | printf1.h:114:18:114:18 | d | This argument should be of type 'long double' but is of type 'double' |
 | printf1.h:147:19:147:19 | i | This argument should be of type 'long long' but is of type 'int' |
 | printf1.h:148:19:148:20 | ui | This argument should be of type 'unsigned long long' but is of type 'unsigned int' |
+| printf1.h:159:18:159:18 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:160:18:160:18 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:167:17:167:17 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:168:18:168:18 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:169:19:169:19 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:174:17:174:17 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:175:18:175:18 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:176:19:176:19 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:180:17:180:17 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:181:20:181:20 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:183:18:183:18 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:184:21:184:21 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:186:19:186:19 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:187:22:187:22 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:189:19:189:19 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:190:22:190:22 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:192:19:192:19 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:193:22:193:22 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:194:25:194:25 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:213:28:213:28 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:214:28:214:28 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:215:28:215:28 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:216:28:216:28 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:221:18:221:18 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:222:20:222:20 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:233:22:233:22 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:233:25:233:25 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:234:22:234:22 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:234:25:234:25 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:235:22:235:22 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:235:25:235:25 | i | This argument should be of type 'char *' but is of type 'int' |
 | real_world.h:61:21:61:22 | & ... | This argument should be of type 'int *' but is of type 'short *' |
 | real_world.h:62:22:62:23 | & ... | This argument should be of type 'short *' but is of type 'int *' |
 | real_world.h:63:22:63:24 | & ... | This argument should be of type 'short *' but is of type 'unsigned int *' |
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/printf1.h b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/printf1.h
index ab9a9e18635..6713648267e 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/printf1.h	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/printf1.h	
@@ -151,3 +151,86 @@ void fun4()
   printf("%qi\n", ll); // GOOD
   printf("%qu\n", ull); // GOOD
 }
+
+void complexFormatSymbols(int i, const char *s)
+{
+  // positional arguments
+  printf("%1$i", i, s); // GOOD
+  printf("%2$s", i, s); // GOOD [FALSE POSITIVE]
+  printf("%1$s", i, s); // BAD
+  printf("%2$i", i, s); // BAD [NOT DETECTED]
+
+  // width / precision
+  printf("%4i", i); // GOOD
+  printf("%.4i", i); // GOOD
+  printf("%4.4i", i); // GOOD
+  printf("%4s", i); // BAD
+  printf("%.4s", i); // BAD
+  printf("%4.4s", i); // BAD
+
+  printf("%4s", s); // GOOD
+  printf("%.4s", s); // GOOD
+  printf("%4.4s", s); // GOOD
+  printf("%4i", s); // BAD
+  printf("%.4i", s); // BAD
+  printf("%4.4i", s); // BAD
+
+  // variable width / precision
+  printf("%*s", i, s); // GOOD
+  printf("%*s", s, s); // BAD
+  printf("%*s", i, i); // BAD
+  printf("%.*s", i, s); // GOOD
+  printf("%.*s", s, s); // BAD
+  printf("%.*s", i, i); // BAD
+  printf("%*.4s", i, s); // GOOD
+  printf("%*.4s", s, s); // BAD
+  printf("%*.4s", i, i); // BAD
+  printf("%4.*s", i, s); // GOOD
+  printf("%4.*s", s, s); // BAD
+  printf("%4.*s", i, i); // BAD
+  printf("%*.*s", i, i, s); // GOOD
+  printf("%*.*s", s, i, s); // BAD
+  printf("%*.*s", i, s, s); // BAD
+  printf("%*.*s", i, i, i); // BAD
+
+  // positional arguments mixed with variable width / precision
+  printf("%2$*1$s", i, s); // GOOD
+  printf("%2$*2$s", i, s); // BAD [NOT DETECTED]
+  printf("%1$*1$s", i, s); // BAD [NOT DETECTED]
+
+  printf("%2$*1$.4s", i, s); // GOOD
+  printf("%2$*2$.4s", i, s); // BAD [NOT DETECTED]
+  printf("%1$*1$.4s", i, s); // BAD [NOT DETECTED]
+
+  printf("%2$.*1$s", i, s); // GOOD
+  printf("%2$.*2$s", i, s); // BAD [NOT DETECTED]
+  printf("%1$.*1$s", i, s); // BAD [NOT DETECTED]
+
+  printf("%2$4.*1$s", i, s); // GOOD
+  printf("%2$4.*2$s", i, s); // BAD [NOT DETECTED]
+  printf("%1$4.*1$s", i, s); // BAD [NOT DETECTED]
+
+  printf("%2$*1$.*1$s", i, s); // GOOD [FALSE POSITIVE]
+  printf("%2$*2$.*1$s", i, s); // BAD
+  printf("%2$*1$.*2$s", i, s); // BAD
+  printf("%1$*1$.*1$s", i, s); // BAD
+
+  // left justify flag
+  printf("%-4s", s); // GOOD
+  printf("%1$-4s", s); // GOOD
+  printf("%-4i", s); // BAD
+  printf("%1$-4i", s); // BAD
+
+  printf("%1$-4s", s, i); // GOOD
+  printf("%2$-4s", s, i); // BAD [NOT DETECTED]
+
+  printf("%1$-.4s", s, i); // GOOD
+  printf("%2$-.4s", s, i); // BAD [NOT DETECTED]
+
+  printf("%1$-4.4s", s, i); // GOOD
+  printf("%2$-4.4s", s, i); // BAD [NOT DETECTED]
+ 
+  printf("%1$-*2$s", s, i); // GOOD [FALSE POSITIVE x2]
+  printf("%2$-*2$s", s, i); // BAD [ADDITIONAL RESULT IS A FALSE POSITIVE]
+  printf("%1$-*1$s", s, i); // BAD [ADDITIONAL RESULT IS A FALSE POSITIVE]
+}

From 27478640f2f1e9cded995baab01c3811c86b6b5a Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 30 Oct 2019 14:01:44 +0000
Subject: [PATCH 2/8] CPP: Bring the logic for argument indices together in
 getFormatArgumentIndexFor.

---
 cpp/ql/src/semmle/code/cpp/commons/Printf.qll | 76 ++++++++++++-------
 1 file changed, 48 insertions(+), 28 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
index 3ad27fa0394..e6948e6a4c3 100644
--- a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
+++ b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
@@ -131,16 +131,11 @@ class FormattingFunctionCall extends Expr {
   }
 
   /**
-   * Gets the argument corresponding to the nth conversion specifier
+   * Gets the argument corresponding to the nth conversion specifier.
    */
   Expr getConversionArgument(int n) {
-    exists(FormatLiteral fl, int b, int o |
-      fl = this.getFormat() and
-      b = sum(int i, int toSum | i < n and toSum = fl.getNumArgNeeded(i) | toSum) and
-      o = fl.getNumArgNeeded(n) and
-      o > 0 and
-      result = this.getFormatArgument(b + o - 1)
-    )
+    result = this
+          .getFormatArgument(this.getFormat().(FormatLiteral).getFormatArgumentIndexFor(n, 2))
   }
 
   /**
@@ -149,12 +144,8 @@ class FormattingFunctionCall extends Expr {
    * an explicit minimum field width).
    */
   Expr getMinFieldWidthArgument(int n) {
-    exists(FormatLiteral fl, int b |
-      fl = this.getFormat() and
-      b = sum(int i, int toSum | i < n and toSum = fl.getNumArgNeeded(i) | toSum) and
-      fl.hasImplicitMinFieldWidth(n) and
-      result = this.getFormatArgument(b)
-    )
+    result = this
+          .getFormatArgument(this.getFormat().(FormatLiteral).getFormatArgumentIndexFor(n, 0))
   }
 
   /**
@@ -163,13 +154,8 @@ class FormattingFunctionCall extends Expr {
    * precision).
    */
   Expr getPrecisionArgument(int n) {
-    exists(FormatLiteral fl, int b, int o |
-      fl = this.getFormat() and
-      b = sum(int i, int toSum | i < n and toSum = fl.getNumArgNeeded(i) | toSum) and
-      (if fl.hasImplicitMinFieldWidth(n) then o = 1 else o = 0) and
-      fl.hasImplicitPrecision(n) and
-      result = this.getFormatArgument(b + o)
-    )
+    result = this
+          .getFormatArgument(this.getFormat().(FormatLiteral).getFormatArgumentIndexFor(n, 1))
   }
 
   /**
@@ -784,19 +770,53 @@ class FormatLiteral extends Literal {
     )
   }
 
+  /**
+   * Holds if the nth conversion specifier of this format string (if `mode = 2`), it's
+   * minimum field width (if `mode = 0`) or it's precision (if `mode = 1`) requires a
+   * format argument.
+   *
+   * Most conversion specifiers require a format argument, whereas minimum field width
+   * and precision only require a format argument if they are present and a `*` was
+   * used for it's value in the format string.
+   */
+  private predicate hasFormatArgumentIndexFor(int n, int mode) {
+    mode = 0 and
+    this.hasImplicitMinFieldWidth(n)
+    or
+    mode = 1 and
+    this.hasImplicitPrecision(n)
+    or
+    mode = 2 and
+    exists(this.getConvSpecOffset(n)) and
+    not this.getConversionChar(n) = "m"
+  }
+
+  /**
+   * Gets the format argument index for the nth conversion specifier of this format
+   * string (if `mode = 2`), it's minimum field width (if `mode = 0`) or it's
+   * precision (if `mode = 1`).  Has no result if that element is not present.  Does
+   * not account for positional arguments (`$`).
+   */
+  int getFormatArgumentIndexFor(int n, int mode) {
+    hasFormatArgumentIndexFor(n, mode) and
+    result = count(int n2, int mode2 |
+      hasFormatArgumentIndexFor(n2, mode2) and
+      (
+        n2 < n
+        or
+        n2 = n and
+        mode2 < mode
+      )
+    )
+  }
+
   /**
    * Gets the number of arguments required by the nth conversion specifier
    * of this format string.
    */
   int getNumArgNeeded(int n) {
     exists(this.getConvSpecOffset(n)) and
-    not this.getConversionChar(n) = "%" and
-    exists(int n1, int n2, int n3 |
-      (if this.hasImplicitMinFieldWidth(n) then n1 = 1 else n1 = 0) and
-      (if this.hasImplicitPrecision(n) then n2 = 1 else n2 = 0) and
-      (if this.getConversionChar(n) = "m" then n3 = 0 else n3 = 1) and
-      result = n1 + n2 + n3
-    )
+    result = count(int mode | hasFormatArgumentIndexFor(n, mode))
   }
 
   /**

From 2430bf4c83b3a3537f4702b6595902bacf2248ba Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 30 Oct 2019 14:03:57 +0000
Subject: [PATCH 3/8] CPP: Deprecate helper version of getNumArgNeeded.

---
 cpp/ql/src/semmle/code/cpp/commons/Printf.qll | 21 +++++++++++--------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
index e6948e6a4c3..678a32736ca 100644
--- a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
+++ b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
@@ -800,21 +800,24 @@ class FormatLiteral extends Literal {
   int getFormatArgumentIndexFor(int n, int mode) {
     hasFormatArgumentIndexFor(n, mode) and
     result = count(int n2, int mode2 |
-      hasFormatArgumentIndexFor(n2, mode2) and
-      (
-        n2 < n
-        or
-        n2 = n and
-        mode2 < mode
+        hasFormatArgumentIndexFor(n2, mode2) and
+        (
+          n2 < n
+          or
+          n2 = n and
+          mode2 < mode
+        )
       )
-    )
   }
 
   /**
    * Gets the number of arguments required by the nth conversion specifier
    * of this format string.
+   *
+   * DEPRECATED.  This was a helper function for `getNumArgNeeded` and is no
+   * longer required.
    */
-  int getNumArgNeeded(int n) {
+  deprecated int getNumArgNeeded(int n) {
     exists(this.getConvSpecOffset(n)) and
     result = count(int mode | hasFormatArgumentIndexFor(n, mode))
   }
@@ -828,7 +831,7 @@ class FormatLiteral extends Literal {
       // At least one conversion specifier has a parameter field, in which case,
       // they all should have.
       result = max(string s | this.getParameterField(_) = s + "$" | s.toInt())
-    else result = sum(int n, int toSum | toSum = this.getNumArgNeeded(n) | toSum)
+    else result = count(int n, int mode | hasFormatArgumentIndexFor(n, mode))
   }
 
   /**

From dff21e02dbb952bf24dc60115bf337432802bfdd Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 30 Oct 2019 15:03:53 +0000
Subject: [PATCH 4/8] CPP: Fully support positional arguments.

---
 cpp/ql/src/semmle/code/cpp/commons/Printf.qll | 57 +++++++++++++++++--
 .../WrongTypeFormatArguments.expected         | 20 ++++---
 .../Linux_signed_chars/printf1.h              | 36 ++++++------
 3 files changed, 82 insertions(+), 31 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
index 678a32736ca..7919c8c83d0 100644
--- a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
+++ b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
@@ -134,8 +134,15 @@ class FormattingFunctionCall extends Expr {
    * Gets the argument corresponding to the nth conversion specifier.
    */
   Expr getConversionArgument(int n) {
-    result = this
-          .getFormatArgument(this.getFormat().(FormatLiteral).getFormatArgumentIndexFor(n, 2))
+    exists(FormatLiteral fl |
+      fl = this.getFormat() and
+      (
+        result = this.getFormatArgument(fl.getParameterFieldPositional(n))
+        or
+        result = this.getFormatArgument(fl.getFormatArgumentIndexFor(n, 2)) and
+        not exists(fl.getParameterFieldPositional(n))
+      )
+    )
   }
 
   /**
@@ -144,8 +151,15 @@ class FormattingFunctionCall extends Expr {
    * an explicit minimum field width).
    */
   Expr getMinFieldWidthArgument(int n) {
-    result = this
-          .getFormatArgument(this.getFormat().(FormatLiteral).getFormatArgumentIndexFor(n, 0))
+    exists(FormatLiteral fl |
+      fl = this.getFormat() and
+      (
+        result = this.getFormatArgument(fl.getMinFieldWidthPositional(n))
+        or
+        result = this.getFormatArgument(fl.getFormatArgumentIndexFor(n, 0)) and
+        not exists(fl.getMinFieldWidthPositional(n))
+      )
+    )
   }
 
   /**
@@ -154,8 +168,15 @@ class FormattingFunctionCall extends Expr {
    * precision).
    */
   Expr getPrecisionArgument(int n) {
-    result = this
-          .getFormatArgument(this.getFormat().(FormatLiteral).getFormatArgumentIndexFor(n, 1))
+    exists(FormatLiteral fl |
+      fl = this.getFormat() and
+      (
+        result = this.getFormatArgument(fl.getPrecisionPositional(n))
+        or
+        result = this.getFormatArgument(fl.getFormatArgumentIndexFor(n, 1)) and
+        not exists(fl.getPrecisionPositional(n))
+      )
+    )
   }
 
   /**
@@ -354,6 +375,14 @@ class FormatLiteral extends Literal {
    */
   string getParameterField(int n) { this.parseConvSpec(n, _, result, _, _, _, _, _) }
 
+  /**
+   * Gets the parameter field of the nth conversion specifier (if it has one) as a
+   * zero-based number.
+   */
+  int getParameterFieldPositional(int n) {
+    result = this.getParameterField(n).regexpCapture("([0-9]*)\\$", 1).toInt() - 1
+  }
+
   /**
    * Gets the flags of the nth conversion specifier.
    */
@@ -423,6 +452,14 @@ class FormatLiteral extends Literal {
    */
   int getMinFieldWidth(int n) { result = this.getMinFieldWidthOpt(n).toInt() }
 
+  /**
+   * Gets the zero-based parameter number of the minimum field width of the nth
+   * conversion specifier, if it is implicit and uses a positional parameter.
+   */
+  int getMinFieldWidthPositional(int n) {
+    result = this.getMinFieldWidthOpt(n).regexpCapture("\\*([0-9]*)\\$", 1).toInt() - 1
+  }
+
   /**
    * Gets the precision of the nth conversion specifier (empty string if none is given).
    */
@@ -453,6 +490,14 @@ class FormatLiteral extends Literal {
     else result = this.getPrecisionOpt(n).regexpCapture("\\.([0-9]*)", 1).toInt()
   }
 
+  /**
+   * Gets the zero-based parameter number of the precision of the nth conversion
+   * specifier, if it is implicit and uses a positional parameter.
+   */
+  int getPrecisionPositional(int n) {
+    result = this.getPrecisionOpt(n).regexpCapture("\\.\\*([0-9]*)\\$", 1).toInt() - 1
+  }
+
   /**
    * Gets the length flag of the nth conversion specifier.
    */
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/WrongTypeFormatArguments.expected b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/WrongTypeFormatArguments.expected
index 441b62b1c2d..c67057710bb 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/WrongTypeFormatArguments.expected	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/WrongTypeFormatArguments.expected	
@@ -16,8 +16,8 @@
 | printf1.h:114:18:114:18 | d | This argument should be of type 'long double' but is of type 'double' |
 | printf1.h:147:19:147:19 | i | This argument should be of type 'long long' but is of type 'int' |
 | printf1.h:148:19:148:20 | ui | This argument should be of type 'unsigned long long' but is of type 'unsigned int' |
-| printf1.h:159:18:159:18 | i | This argument should be of type 'char *' but is of type 'int' |
 | printf1.h:160:18:160:18 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:161:21:161:21 | s | This argument should be of type 'int' but is of type 'char *' |
 | printf1.h:167:17:167:17 | i | This argument should be of type 'char *' but is of type 'int' |
 | printf1.h:168:18:168:18 | i | This argument should be of type 'char *' but is of type 'int' |
 | printf1.h:169:19:169:19 | i | This argument should be of type 'char *' but is of type 'int' |
@@ -35,18 +35,24 @@
 | printf1.h:192:19:192:19 | s | This argument should be of type 'int' but is of type 'char *' |
 | printf1.h:193:22:193:22 | s | This argument should be of type 'int' but is of type 'char *' |
 | printf1.h:194:25:194:25 | i | This argument should be of type 'char *' but is of type 'int' |
-| printf1.h:213:28:213:28 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:198:24:198:24 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:199:21:199:21 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:202:26:202:26 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:203:23:203:23 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:206:25:206:25 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:207:22:207:22 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:210:26:210:26 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:211:23:211:23 | i | This argument should be of type 'char *' but is of type 'int' |
 | printf1.h:214:28:214:28 | s | This argument should be of type 'int' but is of type 'char *' |
 | printf1.h:215:28:215:28 | s | This argument should be of type 'int' but is of type 'char *' |
-| printf1.h:216:28:216:28 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:216:25:216:25 | i | This argument should be of type 'char *' but is of type 'int' |
 | printf1.h:221:18:221:18 | s | This argument should be of type 'int' but is of type 'char *' |
 | printf1.h:222:20:222:20 | s | This argument should be of type 'int' but is of type 'char *' |
-| printf1.h:233:22:233:22 | s | This argument should be of type 'int' but is of type 'char *' |
-| printf1.h:233:25:233:25 | i | This argument should be of type 'char *' but is of type 'int' |
-| printf1.h:234:22:234:22 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:225:23:225:23 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:228:24:228:24 | i | This argument should be of type 'char *' but is of type 'int' |
+| printf1.h:231:25:231:25 | i | This argument should be of type 'char *' but is of type 'int' |
 | printf1.h:234:25:234:25 | i | This argument should be of type 'char *' but is of type 'int' |
 | printf1.h:235:22:235:22 | s | This argument should be of type 'int' but is of type 'char *' |
-| printf1.h:235:25:235:25 | i | This argument should be of type 'char *' but is of type 'int' |
 | real_world.h:61:21:61:22 | & ... | This argument should be of type 'int *' but is of type 'short *' |
 | real_world.h:62:22:62:23 | & ... | This argument should be of type 'short *' but is of type 'int *' |
 | real_world.h:63:22:63:24 | & ... | This argument should be of type 'short *' but is of type 'unsigned int *' |
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/printf1.h b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/printf1.h
index 6713648267e..4517cfadf56 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/printf1.h	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/printf1.h	
@@ -156,9 +156,9 @@ void complexFormatSymbols(int i, const char *s)
 {
   // positional arguments
   printf("%1$i", i, s); // GOOD
-  printf("%2$s", i, s); // GOOD [FALSE POSITIVE]
+  printf("%2$s", i, s); // GOOD
   printf("%1$s", i, s); // BAD
-  printf("%2$i", i, s); // BAD [NOT DETECTED]
+  printf("%2$i", i, s); // BAD
 
   // width / precision
   printf("%4i", i); // GOOD
@@ -195,22 +195,22 @@ void complexFormatSymbols(int i, const char *s)
 
   // positional arguments mixed with variable width / precision
   printf("%2$*1$s", i, s); // GOOD
-  printf("%2$*2$s", i, s); // BAD [NOT DETECTED]
-  printf("%1$*1$s", i, s); // BAD [NOT DETECTED]
+  printf("%2$*2$s", i, s); // BAD
+  printf("%1$*1$s", i, s); // BAD
 
   printf("%2$*1$.4s", i, s); // GOOD
-  printf("%2$*2$.4s", i, s); // BAD [NOT DETECTED]
-  printf("%1$*1$.4s", i, s); // BAD [NOT DETECTED]
+  printf("%2$*2$.4s", i, s); // BAD
+  printf("%1$*1$.4s", i, s); // BAD
 
   printf("%2$.*1$s", i, s); // GOOD
-  printf("%2$.*2$s", i, s); // BAD [NOT DETECTED]
-  printf("%1$.*1$s", i, s); // BAD [NOT DETECTED]
+  printf("%2$.*2$s", i, s); // BAD
+  printf("%1$.*1$s", i, s); // BAD
 
   printf("%2$4.*1$s", i, s); // GOOD
-  printf("%2$4.*2$s", i, s); // BAD [NOT DETECTED]
-  printf("%1$4.*1$s", i, s); // BAD [NOT DETECTED]
+  printf("%2$4.*2$s", i, s); // BAD
+  printf("%1$4.*1$s", i, s); // BAD
 
-  printf("%2$*1$.*1$s", i, s); // GOOD [FALSE POSITIVE]
+  printf("%2$*1$.*1$s", i, s); // GOOD
   printf("%2$*2$.*1$s", i, s); // BAD
   printf("%2$*1$.*2$s", i, s); // BAD
   printf("%1$*1$.*1$s", i, s); // BAD
@@ -222,15 +222,15 @@ void complexFormatSymbols(int i, const char *s)
   printf("%1$-4i", s); // BAD
 
   printf("%1$-4s", s, i); // GOOD
-  printf("%2$-4s", s, i); // BAD [NOT DETECTED]
+  printf("%2$-4s", s, i); // BAD
 
   printf("%1$-.4s", s, i); // GOOD
-  printf("%2$-.4s", s, i); // BAD [NOT DETECTED]
+  printf("%2$-.4s", s, i); // BAD
 
   printf("%1$-4.4s", s, i); // GOOD
-  printf("%2$-4.4s", s, i); // BAD [NOT DETECTED]
- 
-  printf("%1$-*2$s", s, i); // GOOD [FALSE POSITIVE x2]
-  printf("%2$-*2$s", s, i); // BAD [ADDITIONAL RESULT IS A FALSE POSITIVE]
-  printf("%1$-*1$s", s, i); // BAD [ADDITIONAL RESULT IS A FALSE POSITIVE]
+  printf("%2$-4.4s", s, i); // BAD
+
+  printf("%1$-*2$s", s, i); // GOOD
+  printf("%2$-*2$s", s, i); // BAD
+  printf("%1$-*1$s", s, i); // BAD
 }

From a9fbe221bac76831fb86f4152a4477f74b3751dc Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 30 Oct 2019 15:44:41 +0000
Subject: [PATCH 5/8] CPP: Try to make the predicate names and qldoc a bit more
 consistent.

---
 cpp/ql/src/semmle/code/cpp/commons/Printf.qll | 26 +++++++++----------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
index 7919c8c83d0..c91923e30cf 100644
--- a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
+++ b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
@@ -137,10 +137,10 @@ class FormattingFunctionCall extends Expr {
     exists(FormatLiteral fl |
       fl = this.getFormat() and
       (
-        result = this.getFormatArgument(fl.getParameterFieldPositional(n))
+        result = this.getFormatArgument(fl.getParameterFieldValue(n))
         or
         result = this.getFormatArgument(fl.getFormatArgumentIndexFor(n, 2)) and
-        not exists(fl.getParameterFieldPositional(n))
+        not exists(fl.getParameterFieldValue(n))
       )
     )
   }
@@ -154,10 +154,10 @@ class FormattingFunctionCall extends Expr {
     exists(FormatLiteral fl |
       fl = this.getFormat() and
       (
-        result = this.getFormatArgument(fl.getMinFieldWidthPositional(n))
+        result = this.getFormatArgument(fl.getMinFieldWidthParameterFieldValue(n))
         or
         result = this.getFormatArgument(fl.getFormatArgumentIndexFor(n, 0)) and
-        not exists(fl.getMinFieldWidthPositional(n))
+        not exists(fl.getMinFieldWidthParameterFieldValue(n))
       )
     )
   }
@@ -171,10 +171,10 @@ class FormattingFunctionCall extends Expr {
     exists(FormatLiteral fl |
       fl = this.getFormat() and
       (
-        result = this.getFormatArgument(fl.getPrecisionPositional(n))
+        result = this.getFormatArgument(fl.getPrecisionParameterFieldValue(n))
         or
         result = this.getFormatArgument(fl.getFormatArgumentIndexFor(n, 1)) and
-        not exists(fl.getPrecisionPositional(n))
+        not exists(fl.getPrecisionParameterFieldValue(n))
       )
     )
   }
@@ -379,7 +379,7 @@ class FormatLiteral extends Literal {
    * Gets the parameter field of the nth conversion specifier (if it has one) as a
    * zero-based number.
    */
-  int getParameterFieldPositional(int n) {
+  int getParameterFieldValue(int n) {
     result = this.getParameterField(n).regexpCapture("([0-9]*)\\$", 1).toInt() - 1
   }
 
@@ -454,9 +454,9 @@ class FormatLiteral extends Literal {
 
   /**
    * Gets the zero-based parameter number of the minimum field width of the nth
-   * conversion specifier, if it is implicit and uses a positional parameter.
+   * conversion specifier, if it is implicit and uses a parameter field (such as `*1$`).
    */
-  int getMinFieldWidthPositional(int n) {
+  int getMinFieldWidthParameterFieldValue(int n) {
     result = this.getMinFieldWidthOpt(n).regexpCapture("\\*([0-9]*)\\$", 1).toInt() - 1
   }
 
@@ -492,9 +492,9 @@ class FormatLiteral extends Literal {
 
   /**
    * Gets the zero-based parameter number of the precision of the nth conversion
-   * specifier, if it is implicit and uses a positional parameter.
+   * specifier, if it is implicit and uses a parameter field (such as `*1$`).
    */
-  int getPrecisionPositional(int n) {
+  int getPrecisionParameterFieldValue(int n) {
     result = this.getPrecisionOpt(n).regexpCapture("\\.\\*([0-9]*)\\$", 1).toInt() - 1
   }
 
@@ -837,8 +837,8 @@ class FormatLiteral extends Literal {
   }
 
   /**
-   * Gets the format argument index for the nth conversion specifier of this format
-   * string (if `mode = 2`), it's minimum field width (if `mode = 0`) or it's
+   * Gets the computed format argument index for the nth conversion specifier of this
+   * format string (if `mode = 2`), it's minimum field width (if `mode = 0`) or it's
    * precision (if `mode = 1`).  Has no result if that element is not present.  Does
    * not account for positional arguments (`$`).
    */

From 695d4ff511c935b6fa4367eba887d73a52f8fa14 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Wed, 30 Oct 2019 15:51:29 +0000
Subject: [PATCH 6/8] CPP: Change note.

---
 change-notes/1.23/analysis-cpp.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/change-notes/1.23/analysis-cpp.md b/change-notes/1.23/analysis-cpp.md
index c0b38ac0bf4..9a78476a2c5 100644
--- a/change-notes/1.23/analysis-cpp.md
+++ b/change-notes/1.23/analysis-cpp.md
@@ -25,6 +25,7 @@ The following changes in version 1.23 affect C/C++ analysis in all applications.
 | Unclear comparison precedence (`cpp/comparison-precedence`) | Fewer false positive results | False positives involving template classes and functions have been fixed. |
 | Comparison of narrow type with wide type in loop condition (`cpp/comparison-with-wider-type`) | Higher precision | The precision of this query has been increased to "high" as the alerts from this query have proved to be valuable on real-world projects. With this precision, results are now displayed by default in LGTM. |
 | Non-constant format string (`cpp/non-constant-format`) | Fewer false positive results | Fixed false positives resulting from mistmatching declarations of a formatting function. |
+| Wrong type of arguments to formatting function (`cpp/wrong-type-format-argument`) | More correct results and fewer false positive results | This query now understands explicitly specified argument numbers in format strings, such as the `1$` in `%1$s`. |
 
 ## Changes to libraries
 

From ed87f2588629a8e575436bee45b7e6fd89f2f4f8 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 4 Nov 2019 15:16:58 +0000
Subject: [PATCH 7/8] CPP: Performance improvement.

---
 cpp/ql/src/semmle/code/cpp/commons/Printf.qll | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
index c91923e30cf..8200cb7fb9c 100644
--- a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
+++ b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
@@ -844,14 +844,10 @@ class FormatLiteral extends Literal {
    */
   int getFormatArgumentIndexFor(int n, int mode) {
     hasFormatArgumentIndexFor(n, mode) and
-    result = count(int n2, int mode2 |
-        hasFormatArgumentIndexFor(n2, mode2) and
-        (
-          n2 < n
-          or
-          n2 = n and
-          mode2 < mode
-        )
+    (3 * n) + mode = rank[result + 1](int n2, int mode2 |
+        hasFormatArgumentIndexFor(n2, mode2)
+      |
+        (3 * n2) + mode2
       )
   }
 

From a4250be72f70361b2f88c89d9e0bd1b115b13f49 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Mon, 4 Nov 2019 15:54:55 +0000
Subject: [PATCH 8/8] CPP: Un-deprecate getNumArgNeeded(n).  Turns out I missed
 a place where it's used.

---
 cpp/ql/src/semmle/code/cpp/commons/Printf.qll | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
index 8200cb7fb9c..5bb60564c1b 100644
--- a/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
+++ b/cpp/ql/src/semmle/code/cpp/commons/Printf.qll
@@ -854,11 +854,8 @@ class FormatLiteral extends Literal {
   /**
    * Gets the number of arguments required by the nth conversion specifier
    * of this format string.
-   *
-   * DEPRECATED.  This was a helper function for `getNumArgNeeded` and is no
-   * longer required.
    */
-  deprecated int getNumArgNeeded(int n) {
+  int getNumArgNeeded(int n) {
     exists(this.getConvSpecOffset(n)) and
     result = count(int mode | hasFormatArgumentIndexFor(n, mode))
   }