Merge pull request #18386 from MathiasVP/more-robust-param-name-matching

C++: Resolve `typedef`s when matching MaD parameters

cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll

@@ -479,15 +479,87 @@ private Function getFullyTemplatedFunction(Function f) {
   )
 }
 
+/** Prefixes `const` to `s` if `t` is const, or returns `s` otherwise. */
+bindingset[s, t]
+private string withConst(string s, Type t) {
+  if t.isConst() then result = "const " + s else result = s
+}
+
+/** Prefixes `volatile` to `s` if `t` is const, or returns `s` otherwise. */
+bindingset[s, t]
+private string withVolatile(string s, Type t) {
+  if t.isVolatile() then result = "volatile " + s else result = s
+}
+
+/**
+ * Returns `s` prefixed with appropriate specifiers from `t`, or `s` if `t` has
+ * no relevant specifiers.
+ */
+bindingset[s, t]
+private string withSpecifiers(string s, Type t) {
+  // An `int` that is both const and volatile will be printed as
+  // `const volatile int` to match the behavior of `Type.getName` which
+  // is generated by the extractor.
+  result = withConst(withVolatile(s, t), t)
+}
+
+/**
+ * Gets the string version of `t`, after resolving typedefs. The boolean `needsSpace` is `true`
+ * if a space should be appended before concatenating any additional symbols
+ * (such as `*` or `&`) when recursively constructing the type name.
+ */
+private string getTypeName(Type t, boolean needsSpace) {
+  // We don't care about template instantiations since we always base models
+  // on the uninstantiated templates
+  not t.isFromTemplateInstantiation(_) and
+  (
+    exists(DerivedType dt, string s, boolean needsSpace0 |
+      dt = t and s = withSpecifiers(getTypeName(dt.getBaseType(), needsSpace0), dt)
+    |
+      dt instanceof ReferenceType and
+      not dt instanceof RValueReferenceType and
+      needsSpace = false and
+      (if needsSpace0 = true then result = s + " &" else result = s + "&")
+      or
+      dt instanceof RValueReferenceType and
+      needsSpace = false and
+      (if needsSpace0 = true then result = s + " &&" else result = s + "&&")
+      or
+      dt instanceof PointerType and
+      needsSpace = false and
+      (if needsSpace0 = true then result = s + " *" else result = s + "*")
+      or
+      not dt instanceof ReferenceType and
+      not dt instanceof PointerType and
+      result = s and
+      needsSpace = needsSpace0
+    )
+    or
+    not t instanceof DerivedType and
+    not t instanceof TypedefType and
+    result = t.getName() and
+    (if result.matches(["%*", "%&", "%]"]) then needsSpace = false else needsSpace = true)
+  )
+  or
+  result = getTypeName(t.(TypedefType).getBaseType(), needsSpace)
+}
+
 /**
- * Gets the type name of the `n`'th parameter of `f` without any template
- * arguments.
+ * Gets a type name for the `n`'th parameter of `f` without any template
+ * arguments. The result may be a string representing a type for which the
+ * typedefs have been resolved.
  */
 bindingset[f]
 pragma[inline_late]
 string getParameterTypeWithoutTemplateArguments(Function f, int n) {
-  exists(string s, string base, string specifiers |
-    s = f.getParameter(n).getType().getName() and
+  exists(string s, string base, string specifiers, Type t |
+    t = f.getParameter(n).getType() and
+    // The name of the string can either be the possibly typedefed name
+    // or an alternative name where typedefs has been resolved.
+    // `getTypeName(t, _)` is almost equal to `t.resolveTypedefs().getName()`,
+    // except that `t.resolveTypedefs()` doesn't have a result when the
+    // resulting type doesn't appear in the database.
+    s = [t.getName(), getTypeName(t, _)] and
     parseAngles(s, base, _, specifiers) and
     result = base + specifiers
   )
@@ -902,18 +974,19 @@ private Element interpretElement0(
   )
 }
 
-/** Gets the source/sink/summary element corresponding to the supplied parameters. */
-Element interpretElement(
-  string namespace, string type, boolean subtypes, string name, string signature, string ext
-) {
-  elementSpec(namespace, type, subtypes, name, signature, ext) and
-  exists(Element e | e = interpretElement0(namespace, type, subtypes, name, signature) |
-    ext = "" and result = e
-  )
-}
-
 cached
 private module Cached {
+  /** Gets the source/sink/summary element corresponding to the supplied parameters. */
+  cached
+  Element interpretElement(
+    string namespace, string type, boolean subtypes, string name, string signature, string ext
+  ) {
+    elementSpec(namespace, type, subtypes, name, signature, ext) and
+    exists(Element e | e = interpretElement0(namespace, type, subtypes, name, signature) |
+      ext = "" and result = e
+    )
+  }
+
   /**
    * Holds if `node` is specified as a source with the given kind in a CSV flow
    * model.

cpp/ql/test/library-tests/dataflow/models-as-data/FlowSummaryNode.expected

@@ -74,3 +74,5 @@
 | tests.cpp:436:6:436:25 | [summary] to write: Argument[1] in madCallArg0WithValue | PostUpdateNode | madCallArg0WithValue | madCallArg0WithValue |
 | tests.cpp:437:5:437:36 | [summary param] 1 in madCallReturnValueIgnoreFunction | ParameterNode | madCallReturnValueIgnoreFunction | madCallReturnValueIgnoreFunction |
 | tests.cpp:437:5:437:36 | [summary] to write: ReturnValue in madCallReturnValueIgnoreFunction | ReturnNode | madCallReturnValueIgnoreFunction | madCallReturnValueIgnoreFunction |
+| tests.cpp:459:5:459:31 | [summary param] *0 in parameter_ref_to_return_ref | ParameterNode | parameter_ref_to_return_ref | parameter_ref_to_return_ref |
+| tests.cpp:459:5:459:31 | [summary] to write: ReturnValue[*] in parameter_ref_to_return_ref | ReturnNode | parameter_ref_to_return_ref | parameter_ref_to_return_ref |

cpp/ql/test/library-tests/dataflow/models-as-data/SummaryCall.expected

@@ -29,6 +29,7 @@ summarizedCallables
 | tests.cpp:435:9:435:38 | madCallArg0ReturnToReturnFirst |
 | tests.cpp:436:6:436:25 | madCallArg0WithValue |
 | tests.cpp:437:5:437:36 | madCallReturnValueIgnoreFunction |
+| tests.cpp:459:5:459:31 | parameter_ref_to_return_ref |
 sourceCallables
 | tests.cpp:3:5:3:10 | source |
 | tests.cpp:4:6:4:14 | sourcePtr |
@@ -218,3 +219,14 @@ sourceCallables
 | tests.cpp:441:6:441:17 | dontUseValue |
 | tests.cpp:441:23:441:23 | x |
 | tests.cpp:443:6:443:27 | test_function_pointers |
+| tests.cpp:456:19:456:19 | X |
+| tests.cpp:457:8:457:35 | StructWithTypedefInParameter<X> |
+| tests.cpp:457:8:457:35 | StructWithTypedefInParameter<int> |
+| tests.cpp:458:12:458:15 | Type |
+| tests.cpp:459:5:459:31 | parameter_ref_to_return_ref |
+| tests.cpp:459:45:459:45 | x |
+| tests.cpp:459:45:459:45 | x |
+| tests.cpp:462:6:462:37 | test_parameter_ref_to_return_ref |
+| tests.cpp:463:6:463:6 | x |
+| tests.cpp:464:36:464:36 | s |
+| tests.cpp:465:6:465:6 | y |

cpp/ql/test/library-tests/dataflow/models-as-data/testModels.qll

@@ -98,6 +98,7 @@ private class TestSummaries extends SummaryModelCsv {
         ";;false;madCallArg0ReturnToReturnFirst;;;Argument[0].ReturnValue;ReturnValue.Field[first];value",
         ";;false;madCallArg0WithValue;;;Argument[1];Argument[0].Parameter[0];value",
         ";;false;madCallReturnValueIgnoreFunction;;;Argument[1];ReturnValue;value",
+        ";StructWithTypedefInParameter<T>;true;parameter_ref_to_return_ref;(const T &);;Argument[*0];ReturnValue[*];value"
       ]
   }
 }

cpp/ql/test/library-tests/dataflow/models-as-data/tests.cpp

@@ -452,3 +452,16 @@ void test_function_pointers() {
 	madCallReturnValueIgnoreFunction(&sink, source());
 	sink(madCallReturnValueIgnoreFunction(&dontUseValue, source())); // $ ir
 }
+
+template<typename X>
+struct StructWithTypedefInParameter {
+	typedef X Type;
+	X& parameter_ref_to_return_ref(const Type& x); // $ interpretElement
+};
+
+void test_parameter_ref_to_return_ref() {
+	int x = source();
+	StructWithTypedefInParameter<int> s;
+	int y = s.parameter_ref_to_return_ref(x);
+	sink(y); // $ ir
+}