Merge pull request github#6186 from geoffw0/formatarg

C++: Fix FPs from cpp/wrong-type-format-argument

Author: rdmarsh2

cpp/change-notes/2021-06-30-wrong-type-format-argument.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The 'Wrong type of arguments to formatting function' (cpp/wrong-type-format-argument) query is now more accepting of the string and character formatting differences between Microsoft and non-Microsoft platforms.  There are now fewer false positive results.

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

@@ -19,28 +19,32 @@ import cpp
  * Holds if the argument corresponding to the `pos` conversion specifier
  * of `ffc` is expected to have type `expected`.
  */
-pragma[noopt]
 private predicate formattingFunctionCallExpectedType(
   FormattingFunctionCall ffc, int pos, Type expected
 ) {
-  exists(FormattingFunction f, int i, FormatLiteral fl |
-    ffc instanceof FormattingFunctionCall and
-    ffc.getTarget() = f and
-    f.getFormatParameterIndex() = i and
-    ffc.getArgument(i) = fl and
-    fl.getConversionType(pos) = expected
-  )
+  ffc.getFormat().(FormatLiteral).getConversionType(pos) = expected
+}
+
+/**
+ * Holds if the argument corresponding to the `pos` conversion specifier
+ * of `ffc` could alternatively have type `expected`, for example on a different
+ * platform.
+ */
+private predicate formattingFunctionCallAlternateType(
+  FormattingFunctionCall ffc, int pos, Type expected
+) {
+  ffc.getFormat().(FormatLiteral).getConversionTypeAlternate(pos) = expected
 }
 
 /**
  * Holds if the argument corresponding to the `pos` conversion specifier
- * of `ffc` is expected to have type `expected` and the corresponding
- * argument `arg` has type `actual`.
+ * of `ffc` is `arg` and has type `actual`.
  */
 pragma[noopt]
-predicate formatArgType(FormattingFunctionCall ffc, int pos, Type expected, Expr arg, Type actual) {
+predicate formattingFunctionCallActualType(
+  FormattingFunctionCall ffc, int pos, Expr arg, Type actual
+) {
   exists(Expr argConverted |
-    formattingFunctionCallExpectedType(ffc, pos, expected) and
     ffc.getConversionArgument(pos) = arg and
     argConverted = arg.getFullyConverted() and
     actual = argConverted.getType()
@@ -72,7 +76,8 @@ class ExpectedType extends Type {
   ExpectedType() {
     exists(Type t |
       (
-        formatArgType(_, _, t, _, _) or
+        formattingFunctionCallExpectedType(_, _, t) or
+        formattingFunctionCallAlternateType(_, _, t) or
         formatOtherArgType(_, _, t, _, _)
       ) and
       this = t.getUnspecifiedType()
@@ -91,7 +96,11 @@ class ExpectedType extends Type {
  */
 predicate trivialConversion(ExpectedType expected, Type actual) {
   exists(Type exp, Type act |
-    formatArgType(_, _, exp, _, act) and
+    (
+      formattingFunctionCallExpectedType(_, _, exp) or
+      formattingFunctionCallAlternateType(_, _, exp)
+    ) and
+    formattingFunctionCallActualType(_, _, _, act) and
     expected = exp.getUnspecifiedType() and
     actual = act.getUnspecifiedType()
   ) and
@@ -146,9 +155,13 @@ int sizeof_IntType() { exists(IntType it | result = it.getSize()) }
 from FormattingFunctionCall ffc, int n, Expr arg, Type expected, Type actual
 where
   (
-    formatArgType(ffc, n, expected, arg, actual) and
+    formattingFunctionCallExpectedType(ffc, n, expected) and
+    formattingFunctionCallActualType(ffc, n, arg, actual) and
     not exists(Type anyExpected |
-      formatArgType(ffc, n, anyExpected, arg, actual) and
+      (
+        formattingFunctionCallExpectedType(ffc, n, anyExpected) or
+        formattingFunctionCallAlternateType(ffc, n, anyExpected)
+      ) and
       trivialConversion(anyExpected.getUnspecifiedType(), actual.getUnspecifiedType())
     )
     or

cpp/ql/src/semmle/code/cpp/File.qll

@@ -272,20 +272,16 @@ class File extends Container, @file {
    * are compiled by a Microsoft compiler are detected by this predicate.
    */
   predicate compiledAsMicrosoft() {
-    exists(Compilation c |
-      c.getAFileCompiled() = this and
+    exists(File f, Compilation c |
+      c.getAFileCompiled() = f and
       (
         c.getAnArgument() = "--microsoft" or
         c.getAnArgument()
             .toLowerCase()
             .replaceAll("\\", "/")
             .matches(["%/cl.exe", "%/clang-cl.exe"])
-      )
-    )
-    or
-    exists(File parent |
-      parent.compiledAsMicrosoft() and
-      parent.getAnIncludedFile() = this
+      ) and
+      f.getAnIncludedFile*() = this
     )
   }
 
@@ -358,6 +354,11 @@ class File extends Container, @file {
   string getShortName() { files(underlyingElement(this), _, result, _, _) }
 }
 
+/**
+ * Holds if any file was compiled by a Microsoft compiler.
+ */
+predicate anyFileCompiledAsMicrosoft() { any(File f).compiledAsMicrosoft() }
+
 /**
  * A C/C++ header file, as determined (mainly) by file extension.
  *

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

@@ -306,7 +306,7 @@ class FormatLiteral extends Literal {
    * Holds if this `FormatLiteral` is in a context that supports
    * Microsoft rules and extensions.
    */
-  predicate isMicrosoft() { any(File f).compiledAsMicrosoft() }
+  predicate isMicrosoft() { anyFileCompiledAsMicrosoft() }
 
   /**
    * Gets the format string, with '%%' and '%@' replaced by '_' (to avoid processing
@@ -869,6 +869,33 @@ class FormatLiteral extends Literal {
     )
   }
 
+  /**
+   * Gets an alternate argument type that would be required by the nth
+   * conversion specifier on a Microsoft or non-Microsoft platform, opposite
+   * to that of the snapshot. This may be useful for answering 'what might
+   * happen' questions.
+   */
+  Type getConversionTypeAlternate(int n) {
+    exists(string len, string conv |
+      this.parseConvSpec(n, _, _, _, _, _, len, conv) and
+      (len != "l" and len != "w" and len != "h") and
+      getUse().getTarget().(FormattingFunction).getFormatCharType().getSize() > 1 and // wide function
+      (
+        conv = "c" and
+        result = getNonDefaultCharType()
+        or
+        conv = "C" and
+        result = getDefaultCharType()
+        or
+        conv = "s" and
+        result.(PointerType).getBaseType() = getNonDefaultCharType()
+        or
+        conv = "S" and
+        result.(PointerType).getBaseType() = getDefaultCharType()
+      )
+    )
+  }
+
   /**
    * 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

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

@@ -50,7 +50,7 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction {
    * Holds if this `FormattingFunction` is in a context that supports
    * Microsoft rules and extensions.
    */
-  predicate isMicrosoft() { any(File f).compiledAsMicrosoft() }
+  predicate isMicrosoft() { anyFileCompiledAsMicrosoft() }
 
   /**
    * Holds if the default meaning of `%s` is a `wchar_t *`, rather than

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

@@ -3,12 +3,8 @@
 | tests.cpp:21:15:21:21 | Hello | This argument should be of type 'char16_t *' but is of type 'char *' |
 | tests.cpp:21:15:21:21 | 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 'char *' but is of type 'char16_t *' |
-| tests.cpp:27:17:27:24 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
-| tests.cpp:29:17:29:23 | Hello | This argument should be of type 'wchar_t *' but is of type 'char *' |
 | tests.cpp:30:17:30:24 | Hello | This argument should be of type 'wchar_t *' but is of type 'char16_t *' |
-| tests.cpp:34:36:34:43 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
 | tests.cpp:35:36:35:43 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
-| tests.cpp:37:36:37:42 | Hello | This argument should be of type 'char16_t *' but is of type 'char *' |
 | tests.cpp:39:36:39:43 | Hello | This argument should be of type 'char16_t *' but is of type 'wchar_t *' |
 | tests.cpp:42:37:42:44 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
 | tests.cpp:43:37:43:44 | 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/tests.cpp

@@ -24,17 +24,17 @@ void tests() {
 
 	wprintf(L"%s", "Hello"); // GOOD
 	wprintf(L"%s", u"Hello"); // BAD: expecting char
-	wprintf(L"%s", L"Hello"); // BAD: expecting char
+	wprintf(L"%s", L"Hello"); // BAD: expecting char [NOT DETECTED; correct on Microsoft platforms]
 
-	wprintf(L"%S", "Hello"); // BAD: expecting wchar_t
+	wprintf(L"%S", "Hello"); // BAD: expecting wchar_t [NOT DETECTED; correct on Microsoft platforms]
 	wprintf(L"%S", u"Hello"); // BAD: expecting wchar_t
 	wprintf(L"%S", L"Hello"); // GOOD
 
 	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", u"Hello"); // BAD: expecting char [NOT DETECTED; correct on Microsoft platforms]
 	swprintf(buffer, BUF_SIZE, u"%s", L"Hello"); // BAD: expecting char
 
-	swprintf(buffer, BUF_SIZE, u"%S", "Hello"); // BAD: expecting char16_t
+	swprintf(buffer, BUF_SIZE, u"%S", "Hello"); // BAD: expecting char16_t [NOT DETECTED; correct on Microsoft platforms]
 	swprintf(buffer, BUF_SIZE, u"%S", u"Hello"); // GOOD
 	swprintf(buffer, BUF_SIZE, u"%S", L"Hello"); // BAD: expecting char16_t
 

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

@@ -1,3 +1,2 @@
-| printf.cpp:31:31:31:37 | test | This argument should be of type 'char *' but is of type 'char16_t *' |
 | printf.cpp:43:29:43:35 | test | This argument should be of type 'char *' but is of type 'char16_t *' |
 | printf.cpp:50:29:50:35 | test | This argument should be of type 'char16_t *' but is of type 'wchar_t *' |

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

@@ -28,7 +28,7 @@ int sprintf(char *dest, char *format, ...);
 void test1() {
 	WCHAR string[20];
 
-	swprintf(string, u"test %s", u"test"); // BAD: `char16_t` string parameter read as `char` string
+	swprintf(string, u"test %s", u"test"); // BAD: `char16_t` string parameter read as `char` string [NOT DETECTED; correct on Microsoft platforms]
 }
 
 void test2() {

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

@@ -11,8 +11,6 @@
 | printf1.h:45:18:45:20 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
 | printf1.h:46:18:46:20 | ull | This argument should be of type 'unsigned int' but is of type 'unsigned long long' |
 | printf1.h:130:18:130:18 | 0 | This argument should be of type 'void *' but is of type 'int' |
-| printf1.h:154:18:154:19 | wc | This argument should be of type 'char *' but is of type 'wchar_t *' |
-| printf1.h:155:18:155:18 | c | This argument should be of type 'wchar_t *' but is of type 'char *' |
 | printf1.h:168:19:168:19 | i | This argument should be of type 'long long' but is of type 'int' |
 | printf1.h:169:19:169:20 | ui | This argument should be of type 'unsigned long long' but is of type 'unsigned int' |
 | real_world.h:61:21:61:22 | & ... | This argument should be of type 'int *' but is of type 'short *' |