Merge pull request #1119 from geoffw0/wprintf2

CPP: Better handling of %s/%c/%S/%C in Printf/FormattingFunction.qll

Author: jbj

change-notes/1.21/analysis-cpp.md

@@ -17,5 +17,6 @@
 | Memory is never freed (`cpp/memory-never-freed`) | More correct results | Support added for more Microsoft-specific allocation functions, including `LocalAlloc`, `GlobalAlloc`, `HeapAlloc` and `CoTaskMemAlloc`. |
 | Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | Resource allocation and deallocation functions are now determined more accurately. |
 | Comparison result is always the same | Fewer false positive results | The range analysis library is now more conservative about floating point values being possibly `NaN` |
+| Wrong type of arguments to formatting function (`cpp/wrong-type-format-argument`) | More correct results and fewer false positive results | This query now more accurately identifies wide and non-wide string/character format arguments on different platforms.  Platform detection has also been made more accurate for the purposes of this query. |
 
 ## Changes to QL libraries

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

@@ -302,7 +302,13 @@ class File extends Container, @file {
   predicate compiledAsMicrosoft() {
     exists(Compilation c |
       c.getAFileCompiled() = this and
-      c.getAnArgument() = "--microsoft"
+      (
+        c.getAnArgument() = "--microsoft" or
+        c.getAnArgument().toLowerCase().replaceAll("\\", "/").matches("%/cl.exe")
+      )
+    ) or exists(File parent |
+      parent.compiledAsMicrosoft() and
+      parent.getAnIncludedFile() = this
     )
   }
 

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

@@ -32,19 +32,19 @@ class AttributeFormattingFunction extends FormattingFunction {
  * A standard function such as `vprintf` that has a format parameter
  * and a variable argument list of type `va_arg`.
  */
-predicate primitiveVariadicFormatter(TopLevelFunction f, int formatParamIndex, boolean wide) {
+predicate primitiveVariadicFormatter(TopLevelFunction f, int formatParamIndex) {
   f.getName().regexpMatch("_?_?va?[fs]?n?w?printf(_s)?(_p)?(_l)?")
   and (
     if f.getName().matches("%\\_l")
     then formatParamIndex = f.getNumberOfParameters() - 3
     else formatParamIndex = f.getNumberOfParameters() - 2
-  ) and if f.getName().matches("%w%") then wide = true else wide = false
+  )
 }
 
 private
-predicate callsVariadicFormatter(Function f, int formatParamIndex, boolean wide) {
+predicate callsVariadicFormatter(Function f, int formatParamIndex) {
   exists(FunctionCall fc, int i |
-    variadicFormatter(fc.getTarget(), i, wide)
+    variadicFormatter(fc.getTarget(), i)
     and fc.getEnclosingFunction() = f
     and fc.getArgument(i) = f.getParameter(formatParamIndex).getAnAccess()
   )
@@ -54,11 +54,11 @@ predicate callsVariadicFormatter(Function f, int formatParamIndex, boolean wide)
  * Holds if `f` is a function such as `vprintf` that has a format parameter
  * (at `formatParamIndex`) and a variable argument list of type `va_arg`.  
  */
-predicate variadicFormatter(Function f, int formatParamIndex, boolean wide) {
-  primitiveVariadicFormatter(f, formatParamIndex, wide)
+predicate variadicFormatter(Function f, int formatParamIndex) {
+  primitiveVariadicFormatter(f, formatParamIndex)
   or (
     not f.isVarargs()
-    and callsVariadicFormatter(f, formatParamIndex, wide)
+    and callsVariadicFormatter(f, formatParamIndex)
   )
 }
 
@@ -68,12 +68,10 @@ predicate variadicFormatter(Function f, int formatParamIndex, boolean wide) {
  */
 class UserDefinedFormattingFunction extends FormattingFunction {
   UserDefinedFormattingFunction() {
-    isVarargs() and callsVariadicFormatter(this, _, _)
+    isVarargs() and callsVariadicFormatter(this, _)
   }
 
-  override int getFormatParameterIndex() { callsVariadicFormatter(this, result, _) }
-
-  override predicate isWideCharDefault() { callsVariadicFormatter(this, _, true) }
+  override int getFormatParameterIndex() { callsVariadicFormatter(this, result) }
 }
 
 /**
@@ -674,8 +672,8 @@ class FormatLiteral extends Literal {
   /**
    * Gets the char 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.
+   *    (e.g. `char` for `printf`, `char` or `wchar_t` for `wprintf`).
+   *  - the `%C` format character reverses wideness.
    *  - the size prefixes 'l'/'w' and 'h' override the type character
    *    to wide or single-byte characters respectively.
    */
@@ -721,8 +719,8 @@ class FormatLiteral extends Literal {
   /**
    * 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.
+   *    (e.g. `char *` for `printf`, `char *` or `wchar_t *` for `wprintf`).
+   *  - the `%S` format character reverses wideness on some platforms.
    *  - the size prefixes 'l'/'w' and 'h' override the type character
    *    to wide or single-byte characters respectively.
    */

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

@@ -22,7 +22,7 @@ private Type stripTopLevelSpecifiersOnly(Type t) {
  */
 Type getAFormatterWideType() {
   exists(FormattingFunction ff |
-    result = stripTopLevelSpecifiersOnly(ff.getDefaultCharType()) and
+    result = stripTopLevelSpecifiersOnly(ff.getFormatCharType()) and
     result.getSize() != 1
   )
 }
@@ -46,6 +46,14 @@ abstract class FormattingFunction extends Function {
   /** Gets the position at which the format parameter occurs. */
   abstract int getFormatParameterIndex();
 
+  /**
+   * Holds if this `FormattingFunction` is in a context that supports
+   * Microsoft rules and extensions.
+   */
+  predicate isMicrosoft() {
+    getFile().compiledAsMicrosoft()
+  }
+
   /**
    * Holds if the default meaning of `%s` is a `wchar_t *`, rather than
    * a `char *` (either way, `%S` will have the opposite meaning).
@@ -55,31 +63,44 @@ abstract class FormattingFunction extends Function {
   deprecated predicate isWideCharDefault() { none() }
 
   /**
-   * Gets the default character type expected for `%s` by this function.  Typically
-   * `char` or `wchar_t`.
+   * Gets the character type used in the format string for this function.
    */
-  Type getDefaultCharType() {
-    result = 
+  Type getFormatCharType() {
+    result =
       stripTopLevelSpecifiersOnly(
         stripTopLevelSpecifiersOnly(
           getParameter(getFormatParameterIndex()).getType().getUnderlyingType()
         ).(PointerType).getBaseType()
       )
   }
 
+  /**
+   * Gets the default character type expected for `%s` by this function.  Typically
+   * `char` or `wchar_t`.
+   */
+  Type getDefaultCharType() {
+    (
+      isMicrosoft() and
+      result = getFormatCharType()
+    ) or (
+      not isMicrosoft() and
+      result instanceof PlainCharType
+    )
+  }
+
   /**
    * 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
-  	)
+    (
+      getDefaultCharType().getSize() = 1 and
+      result = getWideCharType()
+    ) or (
+      not getDefaultCharType().getSize() = 1 and
+      result instanceof PlainCharType
+    )
   }
 
   /**
@@ -89,10 +110,12 @@ abstract class FormattingFunction extends Function {
    */
   Type getWideCharType() {
     (
-      result = getDefaultCharType() or
-      result = getNonDefaultCharType()
-    ) and
-    result.getSize() > 1
+      result = getFormatCharType() and
+      result.getSize() > 1
+    ) or (
+      not getFormatCharType().getSize() > 1 and
+      result = getAFormatterWideTypeOrDefault() // may have more than one result
+    )
   }
 
   /**

cpp/ql/src/semmle/code/cpp/security/PrintfLike.qll

@@ -4,7 +4,7 @@ import external.ExternalArtifact
 predicate printfLikeFunction(Function func, int formatArg) {
   (formatArg = func.(FormattingFunction).getFormatParameterIndex() and not func instanceof UserDefinedFormattingFunction)
   or
-  primitiveVariadicFormatter(func, formatArg, _)
+  primitiveVariadicFormatter(func, formatArg)
   or
   exists(ExternalData data |
     // TODO Do this \ to / conversion in the toolchain?

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

@@ -1,10 +1,14 @@
 | 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 *' |
+| 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 *' |
+| tests.cpp:45:37:45:43 | Hello | This argument should be of type 'char16_t *' but is of type 'char *' |
+| tests.cpp:47:37:47:44 | Hello | This argument should be of type 'char16_t *' but is of type 'wchar_t *' |

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

@@ -1,3 +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 |
+| tests.cpp:8:5:8:10 | printf | char | char | char16_t, wchar_t | char16_t, wchar_t |
+| tests.cpp:9:5:9:11 | wprintf | wchar_t | char | wchar_t | wchar_t |
+| tests.cpp:10:5:10:12 | swprintf | char16_t | char | char16_t | char16_t |

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

@@ -3,6 +3,7 @@ import cpp
 from FormattingFunction f
 select
   f,
+  concat(f.getFormatCharType().toString(), ", "),
   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

@@ -22,19 +22,27 @@ void tests() {
 	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
 
-	wprintf(L"%S", "Hello"); // GOOD
-	wprintf(L"%S", u"Hello"); // BAD: expecting char
-	wprintf(L"%S", L"Hello"); // BAD: expecting char
+	wprintf(L"%S", "Hello"); // BAD: expecting wchar_t
+	wprintf(L"%S", u"Hello"); // BAD: expecting wchar_t
+	wprintf(L"%S", L"Hello"); // GOOD
 
-	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
 
-	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
+	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"%hs", "Hello"); // GOOD
+	swprintf(buffer, BUF_SIZE, u"%hs", u"Hello"); // BAD: expecting char
+	swprintf(buffer, BUF_SIZE, u"%hs", L"Hello"); // BAD: expecting char
+
+	swprintf(buffer, BUF_SIZE, u"%ls", "Hello"); // BAD: expecting char16_t
+	swprintf(buffer, BUF_SIZE, u"%ls", u"Hello"); // GOOD
+	swprintf(buffer, BUF_SIZE, u"%ls", L"Hello"); // BAD: expecting char16_t
 }

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

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