Proposed amends to MaD subtypes promoted methods PR

go/ql/lib/semmle/go/Types.qll

@@ -483,7 +483,7 @@ class StructType extends @structtype, CompositeType {
   /**
    * Holds if there is an embedded field at `depth`, with either type `tp` or a pointer to `tp`.
    */
-  private predicate hasEmbeddedField(Type tp, int depth) {
+  predicate hasEmbeddedField(Type tp, int depth) {
     exists(Field f | this.hasFieldCand(_, f, depth, true) |
       tp = f.getType() or
       tp = f.getType().(PointerType).getBaseType()

go/ql/lib/semmle/go/dataflow/ExternalFlow.qll

@@ -475,25 +475,21 @@ SourceSinkInterpretationInput::SourceOrSinkElement interpretElement(
   // Go does not need to distinguish functions with signature
   signature = "" and
   exists(string p | p = interpretPackage(pkg) |
-    exists(Field f | f.hasQualifiedName(p, type, name) |
-      result.asEntity() = f and
-      result.hasTypeInfo(p, type, subtypes)
+    result.hasTypeInfo(p, type, subtypes) and
+    (
+      result.asEntity().(Field).hasQualifiedName(p, type, name) or
+      result.asEntity().(Method).hasQualifiedName(p, type, name)
     )
     or
-    exists(Method m | m.hasQualifiedName(p, type, name) |
-      result.asEntity() = m and
-      result.hasTypeInfo(p, type, subtypes)
-      or
-      subtypes = true and
-      // p.type is an interface and we include types which implement it
-      exists(Method m2, string pkg2, string type2 |
-        m2.getReceiverType().implements(p, type) and
-        m2.getName() = name and
-        m2.getReceiverBaseType().hasQualifiedName(pkg2, type2)
-      |
-        result.asEntity() = m2 and
-        result.hasTypeInfo(pkg2, type2, subtypes)
-      )
+    subtypes = true and
+    // p.type is an interface and we include types which implement it
+    exists(Method m2, string pkg2, string type2 |
+      m2.getReceiverType().implements(p, type) and
+      m2.getName() = name and
+      m2.getReceiverBaseType().hasQualifiedName(pkg2, type2)
+    |
+      result.asEntity() = m2 and
+      result.hasTypeInfo(pkg2, type2, subtypes)
     )
     or
     type = "" and

go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll

@@ -149,6 +149,9 @@ module SourceSinkInterpretationInput implements
     )
   }
 
+  // Note that due to embedding, which is currently implemented via some Methods
+  // or Fields having multiple qualified names, a given Method or Field is liable
+  // to have more than one SourceOrSinkElement, one for each of the names it claims.
   private newtype TSourceOrSinkElement =
     TMethodEntityElement(Method m, string pkg, string type, boolean subtypes) {
       m.hasQualifiedName(pkg, type, _) and subtypes = [true, false]
@@ -175,7 +178,7 @@ module SourceSinkInterpretationInput implements
     AstNode asAstNode() { this = TAstElement(result) }
 
     /**
-     * Holds if this source or sink element is a method that was specified
+     * Holds if this source or sink element is a method or field that was specified
      * with the given values for `pkg`, `type` and `subtypes`.
      */
     predicate hasTypeInfo(string pkg, string type, boolean subtypes) {
@@ -241,7 +244,7 @@ module SourceSinkInterpretationInput implements
         (
           not callTarget instanceof Method
           or
-          ensureCorrectTypeInfo(result, cn.getReceiver())
+          elementAppliesToQualifier(result, cn.getReceiver())
         )
       )
     }
@@ -268,55 +271,54 @@ module SourceSinkInterpretationInput implements
     }
   }
 
-  private predicate ensureCorrectTypeInfo(SourceOrSinkElement sse, DataFlow::Node recv) {
+  /**
+   * Holds if method or field spec `sse` applies in the context of qualifier `qual`.
+   *
+   * Note that naively checking `sse.asEntity()`'s qualified name is not correct, because
+   * `Method`s and `Field`s may have multiple qualified names due to embedding. We must instead
+   * check that the specific name given be `sse.hasTypeInfo` refers to either `qual`'s type
+   * or to a type it embeds.
+   */
+  private predicate elementAppliesToQualifier(SourceOrSinkElement sse, DataFlow::Node qual) {
     (
-      exists(DataFlow::CallNode cn | cn.getReceiver() = recv and cn.getTarget() = sse.asEntity())
+      exists(DataFlow::CallNode cn | cn.getReceiver() = qual and cn.getTarget() = sse.asEntity())
       or
-      exists(DataFlow::FieldReadNode frn | frn.getBase() = recv and frn.getField() = sse.asEntity())
+      exists(DataFlow::FieldReadNode frn | frn.getBase() = qual and frn.getField() = sse.asEntity())
       or
-      exists(DataFlow::Write fw | fw.writesField(recv, sse.asEntity(), _))
+      exists(DataFlow::Write fw | fw.writesField(qual, sse.asEntity(), _))
     ) and
-    exists(string pkg, string typename, boolean subtypes, Type syntacticRecvBaseType |
+    exists(
+      string pkg, string typename, boolean subtypes, Type syntacticQualBaseType, Type targetType
+    |
       sse.hasTypeInfo(pkg, typename, subtypes) and
-      syntacticRecvBaseType = lookThroughPointerType(getSyntacticRecv(recv).getType())
+      targetType.hasQualifiedName(pkg, typename) and
+      syntacticQualBaseType = lookThroughPointerType(getSyntacticQualifier(qual).getType())
     |
       subtypes = [true, false] and
-      syntacticRecvBaseType.hasQualifiedName(pkg, typename)
+      syntacticQualBaseType = targetType
       or
       subtypes = true and
       (
-        // `syntacticRecvBaseType`'s underlying type might be an interface type and `sse`
-        // might be a method defined on an interface which is a subtype of it.
-        exists(Type t |
-          t = syntacticRecvBaseType.getUnderlyingType().(InterfaceType).getAnEmbeddedInterface() and
-          t.hasQualifiedName(pkg, typename) and
-          sse.asEntity().(Method).hasQualifiedName(pkg, typename, _)
-        )
+        // `syntacticQualBaseType`'s underlying type might be an interface type and `sse`
+        // might refer to a method defined on an interface embedded within it.
+        targetType =
+          syntacticQualBaseType.getUnderlyingType().(InterfaceType).getAnEmbeddedInterface()
         or
-        // `syntacticRecvBaseType`'s underlying type might be a struct type and `sse`
-        // might be a promoted method or field.
-        exists(StructType st, Field field1, Field field2, int depth1, int depth2, Type t1, Type t2 |
-          st = syntacticRecvBaseType.getUnderlyingType() and
-          field1 = st.getFieldAtDepth(_, depth1) and
-          field2 = st.getFieldAtDepth(_, depth2) and
-          t1 = lookThroughPointerType(field1.getType()) and
-          t2 = lookThroughPointerType(field2.getType()) and
-          (
-            field1 = field2
-            or
-            field2 = t1.getUnderlyingType().(StructType).getFieldAtDepth(_, depth2 - depth1 - 1)
-          ) and
-          matchTypeInfo(sse, t1)
-        |
-          sse.asEntity().(Method).getReceiverBaseType() = t2
-          or
-          sse.asEntity().(Field).getDeclaringType() = t2.getUnderlyingType()
-        )
+        // `syntacticQualBaseType`'s underlying type might be a struct type and `sse`
+        // might refer to an embedded method or field.
+        syntacticQualBaseType.getUnderlyingType().(StructType).hasEmbeddedField(targetType, _)
       )
     )
   }
 
-  private DataFlow::Node getSyntacticRecv(DataFlow::Node n) {
+  /**
+   * Gets `underlying`, where `n` if of the form `implicitDeref?(underlying.implicitFieldRead1.implicitFieldRead2...)`
+   *
+   * For Go syntax like `qualifier.method()` or `qualifier.field`, this is the type of `qualifier`, before any
+   * implicit dereference is interposed because `qualifier` is of pointer type, or implicit field accesses
+   * navigate to any embedded struct types that truly host `field`.
+   */
+  private DataFlow::Node getSyntacticQualifier(DataFlow::Node n) {
     exists(DataFlow::Node n2 |
       // look through implicit dereference, if there is one
       not exists(n.asInstruction().(IR::EvalImplicitDerefInstruction).getOperand()) and
@@ -329,29 +331,19 @@ module SourceSinkInterpretationInput implements
   }
 
   private DataFlow::Node skipImplicitFieldReads(DataFlow::Node n) {
-    not exists(getImplicitFieldReadInstruction(n)) and result = n
+    not exists(lookThroughImplicitFieldRead(n)) and result = n
     or
-    result = skipImplicitFieldReads(getImplicitFieldReadInstruction(n))
+    result = skipImplicitFieldReads(lookThroughImplicitFieldRead(n))
   }
 
-  pragma[inline]
-  private DataFlow::Node getImplicitFieldReadInstruction(DataFlow::Node n) {
+  private DataFlow::Node lookThroughImplicitFieldRead(DataFlow::Node n) {
     result.asInstruction() =
       n.(DataFlow::InstructionNode)
           .asInstruction()
           .(IR::ImplicitFieldReadInstruction)
           .getBaseInstruction()
   }
 
-  bindingset[sse, t]
-  pragma[inline_late]
-  private predicate matchTypeInfo(SourceOrSinkElement sse, Type t) {
-    exists(string pkg, string typename |
-      sse.hasTypeInfo(pkg, typename, true) and
-      t.hasQualifiedName(pkg, typename)
-    )
-  }
-
   /** Provides additional sink specification logic. */
   bindingset[c]
   predicate interpretOutput(string c, InterpretNode mid, InterpretNode node) {
@@ -371,7 +363,7 @@ module SourceSinkInterpretationInput implements
       exists(DataFlow::FieldReadNode frn | frn = n |
         c = "" and
         frn.getField() = e.asEntity() and
-        ensureCorrectTypeInfo(e, frn.getBase())
+        elementAppliesToQualifier(e, frn.getBase())
       )
     )
   }
@@ -392,7 +384,7 @@ module SourceSinkInterpretationInput implements
     |
       c = "" and
       fw.writesField(base, f, node.asNode()) and
-      ensureCorrectTypeInfo(e, base)
+      elementAppliesToQualifier(e, base)
     )
   }
 }