Merge pull request #2003 from geoffw0/formatarg

CPP: WrongTypeFormatArguments.ql Fix

Author: jbj

change-notes/1.23/analysis-cpp.md

@@ -24,6 +24,7 @@ The following changes in version 1.23 affect C/C++ analysis in all applications.
 | 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. |
 | Unclear comparison precedence (`cpp/comparison-precedence`) | Fewer false positive results | False positives involving template classes and functions have been fixed. |
 | Comparison of narrow type with wide type in loop condition (`cpp/comparison-with-wider-type`) | Higher precision | The precision of this query has been increased to "high" as the alerts from this query have proved to be valuable on real-world projects. With this precision, results are now displayed by default in LGTM. |
+| Non-constant format string (`cpp/non-constant-format`) | Fewer false positive results | Fixed false positives resulting from mistmatching declarations of a formatting function. |
 
 ## Changes to libraries
 

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

@@ -8,25 +8,32 @@ import semmle.code.cpp.commons.StringAnalysis
 import semmle.code.cpp.models.interfaces.FormattingFunction
 import semmle.code.cpp.models.implementations.Printf
 
+class PrintfFormatAttribute extends FormatAttribute {
+  PrintfFormatAttribute() {
+    getArchetype() = "printf" or
+    getArchetype() = "__printf__"
+  }
+}
+
 /**
  * A function that can be identified as a `printf` style formatting
  * function by its use of the GNU `format` attribute.
  */
 class AttributeFormattingFunction extends FormattingFunction {
-  FormatAttribute printf_attrib;
-
   override string getCanonicalQLClass() { result = "AttributeFormattingFunction" }
 
   AttributeFormattingFunction() {
-    printf_attrib = getAnAttribute() and
-    (
-      printf_attrib.getArchetype() = "printf" or
-      printf_attrib.getArchetype() = "__printf__"
-    ) and
-    exists(printf_attrib.getFirstFormatArgIndex()) // exclude `vprintf` style format functions
+    exists(PrintfFormatAttribute printf_attrib |
+      printf_attrib = getAnAttribute() and
+      exists(printf_attrib.getFirstFormatArgIndex()) // exclude `vprintf` style format functions
+    )
   }
 
-  override int getFormatParameterIndex() { result = printf_attrib.getFormatIndex() }
+  override int getFormatParameterIndex() {
+    forex(PrintfFormatAttribute printf_attrib | printf_attrib = getAnAttribute() |
+      result = printf_attrib.getFormatIndex()
+    )
+  }
 }
 
 /**

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

@@ -0,0 +1,19 @@
+
+__attribute__((format(printf, 1, 3)))
+void myMultiplyDefinedPrintf(const char *format, const char *extraArg, ...)
+{
+	// ...
+}
+
+__attribute__((format(printf, 1, 3)))
+void myMultiplyDefinedPrintf2(const char *format, const char *extraArg, ...);
+
+char *getString();
+
+void test_custom_printf1()
+{
+  myMultiplyDefinedPrintf("string", getString()); // GOOD
+  myMultiplyDefinedPrintf(getString(), "string"); // BAD [NOT DETECTED]
+  myMultiplyDefinedPrintf2("string", getString()); // GOOD (we can't tell which declaration is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf2(getString(), "string"); // GOOD (we can't tell which declaration is correct so we have to assume this is OK)
+}

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

@@ -0,0 +1,16 @@
+
+__attribute__((format(printf, 2, 3)))
+void myMultiplyDefinedPrintf(const char *extraArg, const char *format, ...); // this declaration does not match the definition
+
+__attribute__((format(printf, 2, 3)))
+void myMultiplyDefinedPrintf2(const char *extraArg, const char *format, ...);
+
+char *getString();
+
+void test_custom_printf2(char *string)
+{
+  myMultiplyDefinedPrintf("string", getString()); // GOOD
+  myMultiplyDefinedPrintf(getString(), "string"); // BAD [NOT DETECTED]
+  myMultiplyDefinedPrintf2("string", getString()); // GOOD (we can't tell which declaration is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf2(getString(), "string"); // GOOD (we can't tell which declaration is correct so we have to assume this is OK)
+}

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

@@ -0,0 +1,16 @@
+
+__attribute__((format(printf, 1, 3)))
+void myMultiplyDefinedPrintf(const char *format, const char *extraArg, ...)
+{
+	// ...
+}
+__attribute__((format(printf, 1, 3)))
+void myMultiplyDefinedPrintf2(const char *format, const char *extraArg, ...);
+
+void test_custom_printf1()
+{
+  myMultiplyDefinedPrintf("%i", "%f", 1); // GOOD
+  myMultiplyDefinedPrintf("%i", "%f", 1.0f); // BAD [NOT DETECTED]
+  myMultiplyDefinedPrintf2("%i", "%f", 1); // GOOD (we can't tell which declaration is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf2("%i", "%f", 1.0f); // GOOD (we can't tell which declaration is correct so we have to assume this is OK)
+}

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

@@ -0,0 +1,14 @@
+
+__attribute__((format(printf, 2, 3)))
+void myMultiplyDefinedPrintf(const char *extraArg, const char *format, ...); // this declaration does not match the definition
+
+__attribute__((format(printf, 2, 3)))
+void myMultiplyDefinedPrintf2(const char *extraArg, const char *format, ...);
+
+void test_custom_printf2()
+{
+  myMultiplyDefinedPrintf("%i", "%f", 1); // GOOD
+  myMultiplyDefinedPrintf("%i", "%f", 1.0f); // BAD [NOT DETECTED]
+  myMultiplyDefinedPrintf2("%i", "%f", 1); // GOOD (we can't tell which declaration is correct so we have to assume this is OK)
+  myMultiplyDefinedPrintf2("%i", "%f", 1.0f); // GOOD (we can't tell which declaration is correct so we have to assume this is OK)
+}