Merge pull request #17775 from github/calumgrant/bmn/wrong-type-format-arguments-test

C++: Reduce FPs in cpp/wrong-type-format-argument due to extraction errors

cpp/ql/lib/change-notes/2014-10-16-implicitly-declared-fns.md

@@ -0,0 +1,5 @@
+---
+category: feature
+---
+* Added the predicate `mayBeFromImplicitlyDeclaredFunction()` to the `Call` class to represent calls that may be the return value of an implicitly declared C function.
+* Added the predicate `getAnExplicitDeclarationEntry()` to the `Function` class to get a `FunctionDeclarationEntry` that is not implicit.

cpp/ql/lib/semmle/code/cpp/Function.qll

@@ -230,6 +230,14 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function {
       )
   }
 
+  /**
+   * Gets a non-implicit function declaration entry.
+   */
+  FunctionDeclarationEntry getAnExplicitDeclarationEntry() {
+    result = this.getADeclarationEntry() and
+    not result.isImplicit()
+  }
+
   private predicate declEntry(FunctionDeclarationEntry fde) {
     fun_decls(unresolveElement(fde), underlyingElement(this), _, _, _) and
     // If one .cpp file specializes a function, and another calls the

cpp/ql/lib/semmle/code/cpp/exprs/Call.qll

@@ -149,6 +149,11 @@ class Call extends Expr, NameQualifiableElement, TCall {
     variableAddressEscapesTreeNonConst(va, this.getQualifier().getFullyConverted()) and
     i = -1
   }
+
+  /** Holds if this expression could be the return value of an implicitly declared function. */
+  predicate mayBeFromImplicitlyDeclaredFunction() {
+    this.getTarget().getADeclarationEntry().isImplicit()
+  }
 }
 
 /**

cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll

@@ -91,7 +91,7 @@ private class Sprintf extends FormattingFunction, NonThrowingFunction {
   override int getFirstFormatArgumentIndex() {
     if this.hasName("__builtin___sprintf_chk")
     then result = 4
-    else result = this.getNumberOfParameters()
+    else result = super.getFirstFormatArgumentIndex()
   }
 }
 

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

@@ -42,6 +42,21 @@ private Type getAFormatterWideTypeOrDefault() {
  * A standard library function that uses a `printf`-like formatting string.
  */
 abstract class FormattingFunction extends ArrayFunction, TaintFunction {
+  int firstFormatArgumentIndex;
+
+  FormattingFunction() {
+    firstFormatArgumentIndex > 0 and
+    if this.hasDefinition()
+    then firstFormatArgumentIndex = this.getDefinition().getNumberOfParameters()
+    else
+      if this instanceof BuiltInFunction
+      then firstFormatArgumentIndex = this.getNumberOfParameters()
+      else
+        forex(FunctionDeclarationEntry fde | fde = this.getAnExplicitDeclarationEntry() |
+          firstFormatArgumentIndex = fde.getNumberOfParameters()
+        )
+  }
+
   /** Gets the position at which the format parameter occurs. */
   abstract int getFormatParameterIndex();
 
@@ -121,33 +136,7 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction {
    * the first format specifier in the format string. We ignore all
    * implicit function definitions.
    */
-  int getFirstFormatArgumentIndex() {
-    // 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)
-    if this.hasDefinition()
-    then result = this.getDefinition().getNumberOfParameters()
-    else result = this.getNumberOfExplicitParameters()
-  }
-
-  /**
-   * Gets a non-implicit function declaration entry.
-   */
-  private FunctionDeclarationEntry getAnExplicitDeclarationEntry() {
-    result = this.getADeclarationEntry() and
-    not result.isImplicit()
-  }
-
-  /**
-   * Gets the number of parameters, excluding any parameters that have been defined
-   * from implicit function declarations. If there is some inconsistency in the number
-   * of parameters, then don't return anything.
-   */
-  private int getNumberOfExplicitParameters() {
-    forex(FunctionDeclarationEntry fde | fde = this.getAnExplicitDeclarationEntry() |
-      result = fde.getNumberOfParameters()
-    )
-  }
+  int getFirstFormatArgumentIndex() { result = firstFormatArgumentIndex }
 
   /**
    * Gets the position of the buffer size argument, if any.

cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql

@@ -170,7 +170,8 @@ where
   ) and
   not arg.isAffectedByMacro() and
   not arg.isFromUninstantiatedTemplate(_) and
-  not actual.getUnspecifiedType() instanceof ErroneousType
+  not actual.getUnspecifiedType() instanceof ErroneousType and
+  not arg.(Call).mayBeFromImplicitlyDeclaredFunction()
 select arg,
   "This format specifier for type '" + expected.getName() + "' does not match the argument type '" +
     actual.getUnspecifiedType().getName() + "'."

cpp/ql/src/change-notes/2024-10-15-wrong-type-format-argument.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Remove results from the `cpp/wrong-type-format-argument` ("Wrong type of arguments to formatting function") query if the argument is the return value of an implicitly declared function.

cpp/ql/test/library-tests/exprs/implicitly_declared/error_type.expected

@@ -0,0 +1 @@
+| file://:0:0:0:0 | <error expr> |

cpp/ql/test/library-tests/exprs/implicitly_declared/error_type.ql

@@ -0,0 +1,5 @@
+import cpp
+
+from Expr e
+where e.getType() instanceof ErroneousType
+select e

cpp/ql/test/library-tests/exprs/implicitly_declared/errors.expected

@@ -0,0 +1,2 @@
+| file://:0:0:0:0 | There was an error during this compilation |
+| implicit.cpp:5:5:5:5 | identifier 'g' is undefined |

cpp/ql/test/library-tests/exprs/implicitly_declared/errors.ql

@@ -0,0 +1,4 @@
+import cpp
+
+from Diagnostic d
+select d

cpp/ql/test/library-tests/exprs/implicitly_declared/functions.expected

@@ -0,0 +1,6 @@
+| implicit2.c:1:7:1:7 | g | file://:0:0:0:0 | float |
+| implicit2.c:1:7:1:7 | g | file://:0:0:0:0 | int |
+| implicit.c:1:6:1:6 | f | file://:0:0:0:0 | void |
+| implicit.c:3:5:3:5 | g | file://:0:0:0:0 | float |
+| implicit.c:3:5:3:5 | g | file://:0:0:0:0 | int |
+| implicit.cpp:3:6:3:6 | f | file://:0:0:0:0 | void |

cpp/ql/test/library-tests/exprs/implicitly_declared/functions.ql

@@ -0,0 +1,5 @@
+import cpp
+
+from Function fn
+where fn.fromSource()
+select fn, fn.getType()

cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.c

@@ -0,0 +1,4 @@
+void f() {
+    f();
+    g();
+}

cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.cpp

@@ -0,0 +1,6 @@
+// semmle-extractor-options: --expect_errors
+
+void f() {
+    f();
+    g();
+}

cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.expected

@@ -0,0 +1 @@
+| implicit.c:3:5:3:5 | call to g |

cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.ql

@@ -0,0 +1,5 @@
+import cpp
+
+from Call c
+where c.mayBeFromImplicitlyDeclaredFunction()
+select c

cpp/ql/test/library-tests/exprs/implicitly_declared/implicit2.c

@@ -0,0 +1 @@
+float g();

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

@@ -0,0 +1 @@
+| tests.c:7:18:7:18 | 1 | This format specifier for type 'char *' does not match the argument type 'int'. |

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.qlref

@@ -0,0 +1 @@
+Likely Bugs/Format/WrongTypeFormatArguments.ql

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

@@ -0,0 +1,11 @@
+// semmle-extractor-options: --expect_errors
+
+int printf(const char * format, ...);
+int fprintf();
+
+void f() {
+    printf("%s", 1); // BAD
+    printf("%s", implicit_function()); // GOOD - we should ignore the type
+    sprintf(0, "%s", ""); // GOOD
+    fprintf(0, "%s", ""); // GOOD
+}

cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/stdlib.h

@@ -17,3 +17,4 @@ void *malloc(size_t size);
 double strtod(const char *ptr, char **endptr);
 char *getenv(const char *name);
 ssize_t read(int fd, void *buffer, size_t count);
+int snprintf(char *s, size_t n, const char *format, ...);