Merge pull request github#1987 from geoffw0/toomanyformat

CPP: WrongNumberOfFormatArguments.ql Fix

Author: jbj

change-notes/1.23/analysis-cpp.md

@@ -18,6 +18,8 @@ The following changes in version 1.23 affect C/C++ analysis in all applications.
 | Hard-coded Japanese era start date in call (`cpp/japanese-era/constructor-or-method-with-exact-era-date`) | Deprecated | This query has been deprecated.  Use the new combined query Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) instead. |
 | Hard-coded Japanese era start date in struct (`cpp/japanese-era/struct-with-exact-era-date`) | Deprecated | This query has been deprecated.  Use the new combined query Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) instead. |
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) | More correct results | This query now checks for the beginning date of the Reiwa era (1st May 2019). |
+| Too few arguments to formatting function (`cpp/wrong-number-format-arguments`) | Fewer false positive results | Fixed false positives resulting from mistmatching declarations of a formatting function. |
+| Too many arguments to formatting function (`cpp/too-many-format-arguments`) | Fewer false positive results | Fixed false positives resulting from mistmatching declarations of a formatting function. |
 
 ## Changes to QL libraries
 

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

@@ -169,7 +169,11 @@ class FormattingFunctionCall extends Expr {
    * Gets the number of arguments to this call that are parameters to the
    * format string.
    */
-  int getNumFormatArgument() { result = count(this.getFormatArgument(_)) }
+  int getNumFormatArgument() {
+    result = count(this.getFormatArgument(_)) and
+    // format arguments must be known
+    exists(getTarget().(FormattingFunction).getFirstFormatArgumentIndex())
+  }
 }
 
 /**

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

@@ -115,7 +115,19 @@ abstract class FormattingFunction extends Function {
    * Gets the position of the first format argument, corresponding with
    * the first format specifier in the format string.
    */
-  int getFirstFormatArgumentIndex() { result = getNumberOfParameters() }
+  int getFirstFormatArgumentIndex() {
+    result = getNumberOfParameters() and
+    // the formatting function either has a definition in the snapshot, or all
+    // `DeclarationEntry`s agree on the number of parameters (otherwise we don't
+    // really know the correct number)
+    (
+      hasDefinition()
+      or
+      forall(FunctionDeclarationEntry fde | fde = getADeclarationEntry() |
+        result = fde.getNumberOfParameters()
+      )
+    )
+  }
 
   /**
    * Gets the position of the buffer size argument, if any.

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

@@ -1,3 +1,6 @@
+| a.c:18:3:18:25 | call to myMultiplyDefinedPrintf | Format expects 1 arguments but given 2 |
+| b.c:15:3:15:25 | call to myMultiplyDefinedPrintf | Format expects 1 arguments but given 2 |
+| c.c:7:3:7:25 | call to myMultiplyDefinedPrintf | Format expects 1 arguments but given 2 |
 | custom_printf.cpp:31:5:31:12 | call to myPrintf | Format expects 2 arguments but given 3 |
 | 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 |

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

@@ -1,3 +1,6 @@
+| a.c:16:3:16:25 | call to myMultiplyDefinedPrintf | Format expects 1 arguments but given 0 |
+| b.c:13:3:13:25 | call to myMultiplyDefinedPrintf | Format expects 1 arguments but given 0 |
+| c.c:5:3:5:25 | call to myMultiplyDefinedPrintf | Format expects 1 arguments but given 0 |
 | custom_printf.cpp:29:5:29:12 | call to myPrintf | Format expects 2 arguments but given 1 |
 | 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 |

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

@@ -0,0 +1,25 @@
+
+__attribute__((format(printf, 1, 3)))
+void myMultiplyDefinedPrintf(const char *format, int extraArg, ...)
+{
+	// ...
+}
+
+__attribute__((format(printf, 1, 3)))
+void myMultiplyDefinedPrintf2(const char *format, int extraArg, ...);
+
+__attribute__((format(printf, 2, 3)))
+void myMultiplyDefinedPrintf3(const char *extraArg, const char *format, ...);
+
+void test_custom_printf1()
+{
+  myMultiplyDefinedPrintf("%i", 0); // BAD (too few format arguments)
+  myMultiplyDefinedPrintf("%i", 0, 1); // GOOD
+  myMultiplyDefinedPrintf("%i", 0, 1, 2); // BAD (too many format arguments)
+  myMultiplyDefinedPrintf2("%i", 0); // GOOD (we can't tell which definition is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf2("%i", 0, 1); // GOOD (we can't tell which definition is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf2("%i", 0, 1, 2); // BAD (too many format arguments regardless of which definition is correct) [NOT DETECTED]
+  myMultiplyDefinedPrintf3("%s", "%s"); // GOOD (we can't tell which definition is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf3("%s", "%s", "%s"); // GOOD (we can't tell which definition is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf3("%s", "%s", "%s", "%s"); // BAD (too many format arguments regardless of which definition is correct) [NOT DETECTED]
+}

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

@@ -0,0 +1,22 @@
+
+__attribute__((format(printf, 1, 2)))
+void myMultiplyDefinedPrintf(const char *format, ...); // this declaration does not match the definition
+
+__attribute__((format(printf, 1, 2)))
+void myMultiplyDefinedPrintf2(const char *format, ...);
+
+__attribute__((format(printf, 1, 2)))
+void myMultiplyDefinedPrintf3(const char *format, ...);
+
+void test_custom_printf2()
+{
+  myMultiplyDefinedPrintf("%i", 0); // BAD (too few format arguments)
+  myMultiplyDefinedPrintf("%i", 0, 1); // GOOD
+  myMultiplyDefinedPrintf("%i", 0, 1, 2); // BAD (too many format arguments)
+  myMultiplyDefinedPrintf2("%i", 0); // GOOD (we can't tell which definition is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf2("%i", 0, 1); // GOOD (we can't tell which definition is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf2("%i", 0, 1, 2); // BAD (too many format arguments regardless of which definition is correct) [NOT DETECTED]
+  myMultiplyDefinedPrintf3("%s", "%s"); // GOOD (we can't tell which definition is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf3("%s", "%s", "%s"); // GOOD (we can't tell which definition is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf3("%s", "%s", "%s", "%s"); // BAD (too many format arguments regardless of which definition is correct) [NOT DETECTED]
+}

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

@@ -0,0 +1,14 @@
+
+void test_custom_printf2()
+{
+  // (implicitly defined)
+  myMultiplyDefinedPrintf("%i", 0); // BAD (too few format arguments)
+  myMultiplyDefinedPrintf("%i", 0, 1); // GOOD
+  myMultiplyDefinedPrintf("%i", 0, 1, 2); // BAD (too many format arguments)
+  myMultiplyDefinedPrintf2("%i", 0); // GOOD (we can't tell which definition is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf2("%i", 0, 1); // GOOD (we can't tell which definition is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf2("%i", 0, 1, 2); // BAD (too many format arguments regardless of which definition is correct) [NOT DETECTED]
+  myMultiplyDefinedPrintf3("%s", "%s"); // GOOD (we can't tell which definition is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf3("%s", "%s", "%s"); // GOOD (we can't tell which definition is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf3("%s", "%s", "%s", "%s"); // BAD (too many format arguments regardless of which definition is correct) [NOT DETECTED]
+}