Merge pull request #1008 from geoffw0/wprintf

CPP: Clean up and fix FormattingFunction, FormatLiteral

Author: jbj

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

@@ -215,8 +215,9 @@ class FormatLiteral extends Literal {
   /**
    * Holds if the default meaning of `%s` is a `wchar_t *`, rather than
    * a `char *` (either way, `%S` will have the opposite meaning).
+   * DEPRECATED: Use getDefaultCharType() instead.
    */
-  predicate isWideCharDefault() {
+  deprecated predicate isWideCharDefault() {
     getUse().getTarget().(FormattingFunction).isWideCharDefault()
   }
 
@@ -671,37 +672,34 @@ class FormatLiteral extends Literal {
   }
 
   /**
-   * Gets the 'effective' char type character, that is, 'c' (meaning a `char`) or
-   * 'C' (meaning a `wchar_t`).
-   *  - 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 'C' or 'c' respectively.
+   * 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.
+   *  - the size prefixes 'l'/'w' and 'h' override the type character
+   *    to wide or single-byte characters respectively.
    */
-  private string getEffectiveCharConversionChar(int n) {
-    exists(string len, string conv | this.parseConvSpec(n, _, _, _, _, _, len, conv) and (conv = "c" or conv = "C") |
-      (len = "l" and result = "C") or
-      (len = "w" and result = "C") or
-      (len = "h" and result = "c") or
-      (len != "l" and len != "w" and len != "h" and (result = "c" or result = "C") and (if isWideCharDefault() then result != conv else result = conv))
-    )
-  }
-
   private Type getConversionType1b(int n) {
-    exists(string cnv | cnv = this.getEffectiveCharConversionChar(n) |
+    exists(string len, string conv |
+      this.parseConvSpec(n, _, _, _, _, _, len, conv) and
       (
-        cnv = "c" and
-        result instanceof CharType and
-        not result.(CharType).isExplicitlySigned() and
-        not result.(CharType).isExplicitlyUnsigned()
-      ) or (
-        cnv = "C" and
-        isMicrosoft() and
-        result instanceof WideCharType
-      ) or (
-        cnv = "C" and
-        not isMicrosoft() and
-        result.hasName("wint_t")
+        (
+          (conv = "c" or conv = "C") and
+          len = "h" and
+          result instanceof PlainCharType
+        ) or (
+          (conv = "c" or conv = "C") and
+          (len = "l" or len = "w") and
+          result = getWideCharType()
+        ) or (
+          conv = "c" and
+          (len != "l" and len != "w" and len != "h") and
+          result = getDefaultCharType()
+        ) or (
+          conv = "C" and
+          (len != "l" and len != "w" and len != "h") and
+          result = getNonDefaultCharType()
+        )
       )
     )
   }
@@ -846,15 +844,7 @@ class FormatLiteral extends Literal {
          len = 1
       or (
         this.getConversionChar(n).toLowerCase()="c" and
-        if (this.getEffectiveCharConversionChar(n)="C" and
-            not isMicrosoft() and
-            not isWideCharDefault()) then (
-          len = 6 // MB_LEN_MAX
-            // the wint_t (wide character) argument is converted
-            // to a multibyte sequence by a call to the wcrtomb(3) 
-        ) else (
-          len = 1 // e.g. 'a'
-        )
+        len = 1 // e.g. 'a'
       ) or this.getConversionChar(n).toLowerCase()="f" and
          exists(int dot, int afterdot |
            (if this.getPrecision(n) = 0 then dot = 0 else dot = 1)

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

@@ -49,16 +49,22 @@ abstract class FormattingFunction extends Function {
   /**
    * Holds if the default meaning of `%s` is a `wchar_t *`, rather than
    * a `char *` (either way, `%S` will have the opposite meaning).
+   *
+   * DEPRECATED: Use getDefaultCharType() instead.
    */
-  predicate isWideCharDefault() { none() }
+  deprecated 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())
+    result = 
+      stripTopLevelSpecifiersOnly(
+        stripTopLevelSpecifiersOnly(
+          getParameter(getFormatParameterIndex()).getType().getUnderlyingType()
+        ).(PointerType).getBaseType()
+      )
   }
 
   /**

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

@@ -12,6 +12,7 @@ typedef struct _IO_FILE FILE;
 extern int printf(const char *fmt, ...);
 extern int vprintf(const char *fmt, va_list ap);
 extern int vfprintf(FILE *stream, const char *format, va_list ap);
+extern int wprintf(const wchar_t *format, ...);
 
 #include "printf1.h"
 #include "real_world.h"

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

@@ -1,4 +1,5 @@
 | common.h:12:12:12:17 | printf | char | wchar_t | wchar_t |
+| common.h:15:12:15:18 | wprintf | wchar_t | char | wchar_t |
 | format.h:4:13:4:17 | error | char | wchar_t | wchar_t |
 | real_world.h:8:12:8:18 | fprintf | char | wchar_t | wchar_t |
 | real_world.h:33:6:33:12 | msg_out | char | wchar_t | wchar_t |

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

@@ -101,3 +101,21 @@ void fun1(unsigned char* a, unsigned char* b) {
   printf("%td\n", pdt); // GOOD
   printf("%td\n", a-b); // GOOD
 }
+
+typedef unsigned int wint_t;
+
+void test_chars(char c, wchar_t wc, wint_t wt)
+{
+  printf("%c", c); // GOOD
+  printf("%c", wc); // BAD [NOT DETECTED]
+  printf("%c", wt); // BAD [NOT DETECTED]
+  printf("%C", c); // BAD [NOT DETECTED]
+  printf("%C", wc); // GOOD (converts to wint_t)
+  printf("%C", wt); // GOOD
+  wprintf(L"%c", c); // GOOD
+  wprintf(L"%c", wc); // BAD [NOT DETECTED]
+  wprintf(L"%c", wt); // BAD [NOT DETECTED]
+  wprintf(L"%C", c); // BAD [NOT DETECTED]
+  wprintf(L"%C", wc); // GOOD (converts to wint_t)
+  wprintf(L"%C", wt); // GOOD
+}

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Microsoft/common.h

@@ -12,6 +12,7 @@ typedef struct _IO_FILE FILE;
 extern int printf(const char *fmt, ...);
 extern int vprintf(const char *fmt, va_list ap);
 extern int vfprintf(FILE *stream, const char *format, va_list ap);
+extern int wprintf(const wchar_t *format, ...);
 
 #include "printf1.h"
 #include "real_world.h"

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Microsoft/printf1.h

@@ -101,3 +101,21 @@ void fun1(unsigned char* a, unsigned char* b) {
   printf("%td\n", pdt); // GOOD
   printf("%td\n", a-b); // GOOD
 }
+
+typedef unsigned int wint_t;
+
+void test_chars(char c, wchar_t wc, wint_t wt)
+{
+  printf("%c", c); // GOOD
+  printf("%c", wc); // BAD [NOT DETECTED]
+  printf("%c", wt); // BAD [NOT DETECTED]
+  printf("%C", c); // BAD [NOT DETECTED]
+  printf("%C", wc); // GOOD (converts to wint_t)
+  printf("%C", wt); // GOOD
+  wprintf(L"%c", c); // BAD [NOT DETECTED]
+  wprintf(L"%c", wc); // GOOD (converts to wint_t)
+  wprintf(L"%c", wt); // GOOD
+  wprintf(L"%C", c); // GOOD
+  wprintf(L"%C", wc); // BAD [NOT DETECTED]
+  wprintf(L"%C", wt); // BAD [NOT DETECTED]
+}