Merge pull request #6186 from geoffw0/formatarg

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

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` is expected to have type `expected` and the corresponding
- * argument `arg` has type `actual`.
+ * 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 `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