CPP: Fix WrongTypeFormatArguments.ql char16_t * issues (and others)

change-notes/1.19/analysis-cpp.md

@@ -13,8 +13,8 @@
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. |
-| Wrong type of arguments to formatting function | Fewer false positive results | False positive results involving typedefs have been removed. |
 | Missing return statement (`cpp/missing-return`) | Visible by default | The precision of this query has been increased from 'medium' to 'high', which makes it visible by default in LGTM. It was 'medium' in release 1.17 and 1.18 because it had false positives due to an extractor bug that was fixed in 1.18. |
+| Wrong type of arguments to formatting function | Fewer false positive results | False positive results involving typedefs have been removed.  Expected argument types are determined more accurately, especially for wide string and pointer types.  Custom (non-standard) formatting functions are also identified more accurately. |
 
 ## Changes to QL libraries
 

cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql

@@ -25,7 +25,8 @@ private predicate formattingFunctionCallExpectedType(FormattingFunctionCall ffc,
       ffc.getTarget() = f and
       f.getFormatParameterIndex() = i and
       ffc.getArgument(i) = fl and
-      fl.getConversionType(pos) = expected
+      fl.getConversionType(pos) = expected and
+      count(fl.getConversionType(pos)) = 1
     )
 }
 

cpp/ql/src/semmle/code/cpp/commons/Printf.qll

@@ -220,6 +220,32 @@ class FormatLiteral extends Literal {
     getUse().getTarget().(FormattingFunction).isWideCharDefault()
   }
 
+  /**
+   * Gets the default character type expected for `%s` by this format literal.  Typically
+   * `char` or `wchar_t`.
+   */
+  Type getDefaultCharType() {
+    result = getUse().getTarget().(FormattingFunction).getDefaultCharType()
+  }
+
+  /**
+   * Gets the non-default character type expected for `%S` by this format literal.  Typically
+   * `wchar_t` or `char`.  On some snapshots there may be multiple results where we can't tell
+   * which is correct for a particular function.
+   */
+  Type getNonDefaultCharType() {
+    result = getUse().getTarget().(FormattingFunction).getNonDefaultCharType()
+  }
+
+  /**
+   * Gets the wide character type for this format literal.  This is usually `wchar_t`.  On some
+   * snapshots there may be multiple results where we can't tell which is correct for a
+   * particular function.
+   */
+  Type getWideCharType() {
+    result = getUse().getTarget().(FormattingFunction).getWideCharType()
+  }
+
   /**
    * Holds if this `FormatLiteral` is in a context that supports
    * Microsoft rules and extensions.
@@ -629,7 +655,6 @@ class FormatLiteral extends Literal {
     result = getConversionType2(n) or
     result = getConversionType3(n) or
     result = getConversionType4(n) or
-    result = getConversionType5(n) or
     result = getConversionType6(n) or
     result = getConversionType7(n) or
     result = getConversionType8(n) or
@@ -696,33 +721,35 @@ class FormatLiteral extends Literal {
   }
 
   /**
-   * Gets the 'effective' string type character, that is, 's' (meaning a char string) or
-   * 'S' (meaning a wide string).
-   *  - in the base case this is the same as the format type character.
-   *  - for a `wprintf` or similar function call, the meanings are reversed.
-   *  - the size prefixes 'l'/'w' (long) and 'h' (short) override the
-   *    type character to effectively 'S' or 's' respectively.
-   */
-  private string getEffectiveStringConversionChar(int n) {
-    exists(string len, string conv | this.parseConvSpec(n, _, _, _, _, _, len, conv) and (conv = "s" or conv = "S") |
-      (len = "l" and result = "S") or
-      (len = "w" and result = "S") or
-      (len = "h" and result = "s") or
-      (len != "l" and len != "w" and len != "h" and (result = "s" or result = "S") and (if isWideCharDefault() then result != conv else result = conv))
-    )
-  }
-
+   * Gets the string type required by the nth conversion specifier.
+   *  - in the base case this is the default for the formatting function
+   *    (e.g. `char` for `printf`, `wchar_t` for `wprintf`).
+   *  - the `%S` format character reverses wideness.
+   *  - the size prefixes 'l'/'w' and 'h' override the type character
+   *    to wide or single-byte characters respectively.
+   */
   private Type getConversionType4(int n) {
-    exists(string cnv, CharType t | cnv = this.getEffectiveStringConversionChar(n) |
-      cnv="s" and t = result.(PointerType).getBaseType()
-      and not t.isExplicitlySigned()
-      and not t.isExplicitlyUnsigned()
-    )
-  }
-
-  private Type getConversionType5(int n) {
-    exists(string cnv | cnv = this.getEffectiveStringConversionChar(n) |
-      cnv="S" and result.(PointerType).getBaseType().hasName("wchar_t") 
+    exists(string len, string conv |
+      this.parseConvSpec(n, _, _, _, _, _, len, conv) and
+      (
+        (
+          (conv = "s" or conv = "S") and
+          len = "h" and
+          result.(PointerType).getBaseType() instanceof PlainCharType
+        ) or (
+          (conv = "s" or conv = "S") and
+          (len = "l" or len = "w") and
+          result.(PointerType).getBaseType() = getWideCharType()
+        ) or (
+          conv = "s" and
+          (len != "l" and len != "w" and len != "h") and
+          result.(PointerType).getBaseType() = getDefaultCharType()
+        ) or (
+          conv = "S" and
+          (len != "l" and len != "w" and len != "h") and
+          result.(PointerType).getBaseType() = getNonDefaultCharType()
+        )
+      )
     )
   }
 

cpp/ql/src/semmle/code/cpp/models/implementations/Printf.qll

@@ -12,7 +12,8 @@ class Printf extends FormattingFunction {
       hasGlobalName("wprintf") or
       hasGlobalName("wprintf_s") or
       hasGlobalName("g_printf")
-    )
+    ) and
+    not exists(getDefinition().getFile().getRelativePath())
   }
 
   override int getFormatParameterIndex() { result=0 }
@@ -26,7 +27,15 @@ class Printf extends FormattingFunction {
  * The standard functions `fprintf`, `fwprintf` and their glib variants.
  */
 class Fprintf extends FormattingFunction {
-  Fprintf() { this instanceof TopLevelFunction and (hasGlobalName("fprintf") or hasGlobalName("fwprintf") or hasGlobalName("g_fprintf"))}
+  Fprintf() {
+    this instanceof TopLevelFunction and
+    (
+      hasGlobalName("fprintf") or
+      hasGlobalName("fwprintf") or
+      hasGlobalName("g_fprintf")
+    ) and
+    not exists(getDefinition().getFile().getRelativePath())
+  }
 
   override int getFormatParameterIndex() { result=1 }
   override predicate isWideCharDefault() { hasGlobalName("fwprintf") }
@@ -47,7 +56,8 @@ class Sprintf extends FormattingFunction {
       hasGlobalName("g_strdup_printf") or
       hasGlobalName("g_sprintf") or
       hasGlobalName("__builtin___sprintf_chk")
-    )
+    ) and
+    not exists(getDefinition().getFile().getRelativePath())
   }
 
   override predicate isWideCharDefault() {
@@ -100,7 +110,8 @@ class Snprintf extends FormattingFunction {
       or hasGlobalName("g_snprintf")
       or hasGlobalName("wnsprintf")
       or hasGlobalName("__builtin___snprintf_chk")
-    )
+    ) and
+    not exists(getDefinition().getFile().getRelativePath())
   }
 
   override int getFormatParameterIndex() {
@@ -133,10 +144,13 @@ class Snprintf extends FormattingFunction {
    * in the buffer.
    */
   predicate returnsFullFormatLength() {
-    hasGlobalName("snprintf") or
-    hasGlobalName("g_snprintf") or
-    hasGlobalName("__builtin___snprintf_chk") or
-    hasGlobalName("snprintf_s")
+    (
+      hasGlobalName("snprintf") or
+      hasGlobalName("g_snprintf") or
+      hasGlobalName("__builtin___snprintf_chk") or
+      hasGlobalName("snprintf_s")
+    ) and
+    not exists(getDefinition().getFile().getRelativePath())
   }
 
   override int getSizeParameterIndex() {
@@ -158,7 +172,8 @@ class StringCchPrintf extends FormattingFunction {
       or hasGlobalName("StringCbPrintfEx")
       or hasGlobalName("StringCbPrintf_l")
       or hasGlobalName("StringCbPrintf_lEx")
-    )
+    ) and
+    not exists(getDefinition().getFile().getRelativePath())
   }
 
   override int getFormatParameterIndex() {
@@ -187,7 +202,8 @@ class Syslog extends FormattingFunction {
   Syslog() {
     this instanceof TopLevelFunction and (
       hasGlobalName("syslog")
-    )
+    ) and
+    not exists(getDefinition().getFile().getRelativePath())
   }
 
   override int getFormatParameterIndex() { result=1 }

cpp/ql/src/semmle/code/cpp/models/interfaces/FormattingFunction.qll

@@ -7,6 +7,38 @@
  */
 
 import semmle.code.cpp.Function
+
+private Type stripTopLevelSpecifiersOnly(Type t) {
+  (
+    result = stripTopLevelSpecifiersOnly(t.(SpecifiedType).getBaseType())
+  ) or (
+    result = t and
+    not t instanceof SpecifiedType
+  )
+}
+
+/**
+ * A type that is used as a format string by any formatting function.
+ */
+Type getAFormatterWideType() {
+  exists(FormattingFunction ff |
+    result = stripTopLevelSpecifiersOnly(ff.getDefaultCharType()) and
+    result.getSize() != 1
+  )
+}
+
+/**
+ * A type that is used as a format string by any formatting function, or `wchar_t` if
+ * there is none.
+ */
+private Type getAFormatterWideTypeOrDefault() {
+  result = getAFormatterWideType() or
+  (
+    not exists(getAFormatterWideType()) and
+    result instanceof Wchar_t
+  )
+}
+
 /**
  * A standard library function that uses a `printf`-like formatting string.
  */
@@ -20,6 +52,43 @@ abstract class FormattingFunction extends Function {
    */
   predicate isWideCharDefault() { none() }
 
+  /**
+   * Gets the default character type expected for `%s` by this function.  Typically
+   * `char` or `wchar_t`.
+   */
+  Type getDefaultCharType() {
+    result = stripTopLevelSpecifiersOnly(getParameter(getFormatParameterIndex()).getType().
+      getUnderlyingType().(PointerType).getBaseType())
+  }
+
+  /**
+   * Gets the non-default character type expected for `%S` by this function.  Typically
+   * `wchar_t` or `char`.  On some snapshots there may be multiple results where we can't tell
+   * which is correct for a particular function.
+   */
+  Type getNonDefaultCharType() {
+  	(
+  	  getDefaultCharType().getSize() = 1 and
+  	  result = getAFormatterWideTypeOrDefault()
+  	) or (
+  	  getDefaultCharType().getSize() > 1 and
+  	  result instanceof PlainCharType
+  	)
+  }
+
+  /**
+   * Gets the wide character type for this function.  This is usually `wchar_t`.  On some
+   * snapshots there may be multiple results where we can't tell which is correct for a
+   * particular function.
+   */
+  Type getWideCharType() {
+    (
+      result = getDefaultCharType() or
+      result = getNonDefaultCharType()
+    ) and
+    result.getSize() > 1
+  }
+
   /**
    * Gets the position at which the output parameter, if any, occurs.
    */

cpp/ql/test/query-tests/Likely Bugs/Format/WrongNumberOfFormatArguments/TooManyFormatArguments.expected

@@ -1,5 +1,4 @@
 | custom_printf.cpp:31:5:31:12 | call to myPrintf | Format expects 2 arguments but given 3 |
-| custom_printf.cpp:44:2:44:7 | call to printf | Format expects 0 arguments but given 2 |
 | macros.cpp:12:2:12:31 | call to printf | Format expects 2 arguments but given 3 |
 | macros.cpp:16:2:16:30 | call to printf | Format expects 2 arguments but given 3 |
 | test.c:7:2:7:7 | call to printf | Format expects 0 arguments but given 1 |

cpp/ql/test/query-tests/Likely Bugs/Format/WrongNumberOfFormatArguments/WrongNumberOfFormatArguments.expected

@@ -1,5 +1,4 @@
 | custom_printf.cpp:29:5:29:12 | call to myPrintf | Format expects 2 arguments but given 1 |
-| custom_printf.cpp:45:2:45:7 | call to printf | Format expects 2 arguments but given 0 |
 | macros.cpp:14:2:14:37 | call to printf | Format expects 4 arguments but given 3 |
 | macros.cpp:21:2:21:36 | call to printf | Format expects 4 arguments but given 3 |
 | test.c:9:2:9:7 | call to printf | Format expects 1 arguments but given 0 |

cpp/ql/test/query-tests/Likely Bugs/Format/WrongNumberOfFormatArguments/custom_printf.cpp

@@ -41,6 +41,6 @@ void test_custom_printf2()
 {
 	//     notTheFormat  format    ...
 	printf(0,            "%i %i",  100, 200); // GOOD
-	printf("",           "%i %i",  100, 200); // GOOD [FALSE POSITIVE]
-	printf("%i %i",      ""        );         // GOOD [FALSE POSITIVE]
+	printf("",           "%i %i",  100, 200); // GOOD
+	printf("%i %i",      ""        );         // GOOD
 }

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_mixed_byte_wprintf/WrongTypeFormatArguments.expected

@@ -0,0 +1,10 @@
+| tests.cpp:18:15:18:22 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
+| tests.cpp:19:15:19:22 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
+| tests.cpp:25:17:25:23 | Hello | This argument should be of type 'wchar_t *' but is of type 'char *' |
+| tests.cpp:26:17:26:24 | Hello | This argument should be of type 'wchar_t *' but is of type 'char16_t *' |
+| tests.cpp:30:17:30:24 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
+| tests.cpp:31:17:31:24 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
+| tests.cpp:33:36:33:42 | Hello | This argument should be of type 'char16_t *' but is of type 'char *' |
+| tests.cpp:35:36:35:43 | Hello | This argument should be of type 'char16_t *' but is of type 'wchar_t *' |
+| tests.cpp:38:36:38:43 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
+| tests.cpp:39:36:39:43 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_mixed_byte_wprintf/WrongTypeFormatArguments.qlref

@@ -0,0 +1 @@
+Likely Bugs/Format/WrongTypeFormatArguments.ql

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_mixed_byte_wprintf/formattingFunction.expected

@@ -0,0 +1,3 @@
+| tests.cpp:8:5:8:10 | printf | char | char16_t, wchar_t | char16_t, wchar_t |
+| tests.cpp:9:5:9:11 | wprintf | wchar_t | char | wchar_t |
+| tests.cpp:10:5:10:12 | swprintf | char16_t | char | char16_t |

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_mixed_byte_wprintf/formattingFunction.ql

@@ -0,0 +1,8 @@
+import cpp
+
+from FormattingFunction f
+select
+  f,
+  concat(f.getDefaultCharType().toString(), ", "),
+  concat(f.getNonDefaultCharType().toString(), ", "),
+  concat(f.getWideCharType().toString(), ", ")

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_mixed_byte_wprintf/tests.cpp

@@ -0,0 +1,40 @@
+/*
+ * Test for custom definitions of *wprintf using different types than the
+ * platform wide character type.
+ */
+
+typedef unsigned int size_t;
+
+int printf(const char * format, ...);
+int wprintf(const wchar_t * format, ...); // on wchar_t
+int swprintf(char16_t * s, size_t n, const char16_t * format, ...); // on char16_t
+
+#define BUF_SIZE (4096)
+
+void tests() {
+	char16_t buffer[BUF_SIZE];
+
+	printf("%s", "Hello"); // GOOD
+	printf("%s", u"Hello"); // BAD: expecting char
+	printf("%s", L"Hello"); // BAD: expecting char
+
+	printf("%S", "Hello"); // BAD: expecting wchar_t or char16_t [NOT DETECTED]
+	printf("%S", u"Hello"); // GOOD
+	printf("%S", L"Hello"); // GOOD
+
+	wprintf(L"%s", "Hello"); // BAD: expecting wchar_t
+	wprintf(L"%s", u"Hello"); // BAD: expecting wchar_t
+	wprintf(L"%s", L"Hello"); // GOOD
+
+	wprintf(L"%S", "Hello"); // GOOD
+	wprintf(L"%S", u"Hello"); // BAD: expecting char
+	wprintf(L"%S", L"Hello"); // BAD: expecting char
+
+	swprintf(buffer, BUF_SIZE, u"%s", "Hello"); // BAD: expecting char16_t
+	swprintf(buffer, BUF_SIZE, u"%s", u"Hello"); // GOOD
+	swprintf(buffer, BUF_SIZE, u"%s", L"Hello"); // BAD: expecting char16_t
+
+	swprintf(buffer, BUF_SIZE, u"%S", "Hello"); // GOOD
+	swprintf(buffer, BUF_SIZE, u"%S", u"Hello"); // BAD: expecting char
+	swprintf(buffer, BUF_SIZE, u"%S", L"Hello"); // BAD: expecting char
+}

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_mixed_word_size/WrongTypeFormatArguments.expected

@@ -0,0 +1,2 @@
+| tests_32.cpp:14:16:14:23 | void_ptr | This argument should be of type 'long' but is of type 'void *' |
+| tests_64.cpp:14:16:14:23 | void_ptr | This argument should be of type 'long' but is of type 'void *' |

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_mixed_word_size/WrongTypeFormatArguments.qlref

@@ -0,0 +1 @@
+Likely Bugs/Format/WrongTypeFormatArguments.ql