Address review comments

Author: Sauyon Lee

ql/src/semmle/go/controlflow/ControlFlowGraph.qll

@@ -57,6 +57,18 @@ module ControlFlow {
     /** Gets the basic block to which this node belongs. */
     BasicBlock getBasicBlock() { result.getANode() = this }
 
+    /** Holds if this node dominates `dominee` in the control-flow graph. */
+    pragma[inline]
+    predicate dominatesNode(ControlFlow::Node dominee) {
+      exists(ReachableBasicBlock thisbb, ReachableBasicBlock dbb, int i, int j |
+        this = thisbb.getNode(i) and dominee = dbb.getNode(j)
+      |
+        thisbb.strictlyDominates(dbb)
+        or
+        thisbb = dbb and i <= j
+      )
+    }
+
     /** Gets the innermost function or file to which this node belongs. */
     Root getRoot() { none() }
 

ql/src/semmle/go/dataflow/SSA.qll

@@ -316,26 +316,28 @@ private IR::Instruction accessPathAux(TSsaWithFields base, Field f) {
   )
 }
 
-abstract class SsaWithFields extends TSsaWithFields {
+class SsaWithFields extends TSsaWithFields {
   /**
    * Gets the SSA variable corresponding to the base of this SSA variable with fields.
    *
    * For example, the SSA variable corresponding to `a` for the SSA variable with fields
    * corresponding to `a.b`.
    */
-  abstract SsaVariable getBaseVariable();
-
-  /** Gets the type of this SSA variable with fields. */
-  abstract Type getType();
-
-  /** Gets a use in basic block `bb` that refers to this SSA variable with fields. */
-  abstract IR::Instruction getAUseIn(ReachableBasicBlock bb);
+  SsaVariable getBaseVariable() {
+    this = TRoot(result)
+    or
+    exists(SsaWithFields base, Field f | this = TStep(base, f) | result = base.getBaseVariable())
+  }
 
   /** Gets a use that refers to this SSA variable with fields. */
-  IR::Instruction getAUse() { result = this.getAUseIn(_) }
+  DataFlow::Node getAUse() { this = accessPath(result.asInstruction()) }
 
   /** Gets a textual representation of this element. */
-  abstract string toString();
+  string toString() {
+    exists(SsaVariable var | this = TRoot(var) | result = "(" + var + ")")
+    or
+    exists(SsaWithFields base, Field f | this = TStep(base, f) | result = base + "." + f.getName())
+  }
 
   /**
    * Holds if this element is at the specified location.
@@ -350,35 +352,3 @@ abstract class SsaWithFields extends TSsaWithFields {
     this.getBaseVariable().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
   }
 }
-
-private class SsaWithFieldsRoot extends SsaWithFields, TRoot {
-  SsaVariable self;
-
-  SsaWithFieldsRoot() { this = TRoot(self) }
-
-  override SsaVariable getBaseVariable() { result = self }
-
-  override Type getType() { result = self.getType() }
-
-  override IR::Instruction getAUseIn(ReachableBasicBlock bb) { result = self.getAUseIn(bb) }
-
-  override string toString() { result = "(" + self.toString() + ")" }
-}
-
-private class SsaWithFieldsStep extends SsaWithFields, TStep {
-  SsaWithFields base;
-  Field f;
-
-  SsaWithFieldsStep() { this = TStep(base, f) }
-
-  override SsaVariable getBaseVariable() { result = base.getBaseVariable() }
-
-  override Type getType() { result = f.getType() }
-
-  override IR::FieldReadInstruction getAUseIn(ReachableBasicBlock bb) {
-    result.getBase() = base.getAUseIn(bb) and
-    result.getField() = f
-  }
-
-  override string toString() { result = base.toString() + "." + f.getName() }
-}

ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll

@@ -748,11 +748,11 @@ abstract class BarrierGuard extends Node {
 
   /** Gets a node guarded by this guard. */
   final Node getAGuardedNode() {
-    exists(ControlFlow::ConditionGuardNode guard, Node nd |
-      exists(SsaWithFields var | result.asInstruction() = var.getAUse() |
-        guards(guard, nd, var) and
-        guard.dominates(result.asInstruction().getBasicBlock())
-      )
+    exists(ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields var |
+      result = var.getAUse()
+    |
+      guards(guard, nd, var) and
+      guard.dominates(result.asInstruction().getBasicBlock())
     )
   }
 
@@ -764,7 +764,7 @@ abstract class BarrierGuard extends Node {
    */
   pragma[noinline]
   private predicate guards(ControlFlow::ConditionGuardNode guard, Node nd, SsaWithFields ap) {
-    guards(guard, nd) and nd.asInstruction() = ap.getAUse()
+    guards(guard, nd) and nd = ap.getAUse()
   }
 
   /**

ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll

@@ -58,19 +58,13 @@ module OpenUrlRedirect {
    */
   class PathAssignmentBarrier extends Barrier, Read {
     PathAssignmentBarrier() {
-      exists(Write w, Field f, Read writeBase, SsaWithFields var |
+      exists(Write w, Field f, SsaWithFields var |
         f.getName() = "Path" and
         hasHostnameSanitizingSubstring(w.getRhs()) and
-        this.asInstruction() = var.getAUse() and
-        writeBase.asInstruction() = var.getAUse()
+        this = var.getAUse()
       |
-        w.writesField(writeBase, f, _) and
-        w.getBasicBlock().(ReachableBasicBlock).dominates(this.asInstruction().getBasicBlock()) and
-        (
-          not w.getBasicBlock() = this.asInstruction().getBasicBlock()
-          or
-          w.getASuccessor+() = this.asInstruction()
-        )
+        w.writesField(var.getAUse(), f, _) and
+        w.dominatesNode(insn)
       )
     }
   }