Expand SSASourceVariable to include captured variables

Author: owen-mc

Diff for: go/ql/lib/semmle/go/Scopes.qll

@@ -255,8 +255,9 @@ class LocalVariable extends DeclaredVariable {
   }
 
   /** Holds if this variable is referenced inside a nested function. */
-  predicate isCaptured() {
-    this.getDeclaringFunction() != this.getAReference().getEnclosingFunction()
+  predicate isCaptured(FuncDef capturingContainer) {
+    capturingContainer = this.getAReference().getEnclosingFunction() and
+    capturingContainer != this.getDeclaringFunction()
   }
 }
 

Diff for: go/ql/lib/semmle/go/dataflow/GlobalValueNumbering.qll

@@ -408,9 +408,9 @@ private predicate mkPureCall(DataFlow::CallNode ce, Function f, GVN callee, GvnL
 private predicate incompleteSsa(ValueEntity v) {
   not v instanceof Field and
   (
-    not v instanceof SsaSourceVariable
+    not v = any(SsaSourceVariable ssv).getVariable()
     or
-    v.(SsaSourceVariable).mayHaveIndirectReferences()
+    v = any(SsaSourceVariable ssv | ssv.mayHaveIndirectReferences()).getVariable()
     or
     exists(Type tp | tp = v.(DeclaredVariable).getType().getUnderlyingType() |
       not tp instanceof BasicType
@@ -560,7 +560,7 @@ GVN globalValueNumber(DataFlow::Node nd) {
   or
   exists(DataFlow::SsaNode ssa |
     nd = ssa.getAUse() and
-    not incompleteSsa(ssa.getSourceVariable()) and
+    not incompleteSsa(ssa.getSourceVariable().getVariable()) and
     result = globalValueNumber(ssa)
   )
   or

Diff for: go/ql/lib/semmle/go/dataflow/SSA.qll

@@ -9,8 +9,44 @@ private import SsaImpl
  * A variable that can be SSA converted, that is, a local variable, but not a variable
  * declared in file scope.
  */
-class SsaSourceVariable extends LocalVariable {
-  SsaSourceVariable() { not this.getScope() instanceof FileScope }
+class SsaSourceVariable extends TSsaSourceVariable {
+  /** Holds if this `SsaSourceVariable` is a captured variable. */
+  predicate isCapturedVariable() { this instanceof MkCapturedVariableVariable }
+
+  /** Gets the variable corresponding to this `SsaSourceVariable`. */
+  NonFileScopeLocalVariable getVariable() {
+    this = MkNonCapturedVariableVariable(result) or
+    this = MkCapturedVariableVariable(_, result)
+  }
+
+  /** Gets the innermost function to which this `SsaSourceVariable` belongs. */
+  FuncDef getRoot() {
+    this instanceof MkNonCapturedVariableVariable and
+    result = this.getVariable().getDeclaringFunction()
+    or
+    this = MkCapturedVariableVariable(result, _)
+  }
+
+  /** Gets the type of this variable. */
+  Type getType() { result = this.getVariable().getType() }
+
+  /** Gets a textual representation of this `SsaSourceVariable`. */
+  string getName() {
+    this instanceof MkNonCapturedVariableVariable and result = this.getVariable().getName()
+    or
+    exists(FuncDef root, NonFileScopeLocalVariable v | this = MkCapturedVariableVariable(root, v) |
+      result = root.getName() + "(..)." + v.getName()
+      or
+      not exists(root.getName()) and
+      result = "<anonymous>(..)." + v.getName()
+    )
+  }
+
+  /** Gets a textual representation of this `SsaSourceVariable`. */
+  string toString() { result = this.getName() }
+
+  /** Gets the source location for this element. */
+  Location getLocation() { result = this.getVariable().getLocation() }
 
   /**
    * Holds if there may be indirect references of this variable that are not covered by `getAReference()`.
@@ -20,18 +56,18 @@ class SsaSourceVariable extends LocalVariable {
    */
   predicate mayHaveIndirectReferences() {
     // variables that have their address taken
-    exists(AddressExpr addr | addr.getOperand().stripParens() = this.getAReference())
+    exists(AddressExpr addr | addr.getOperand().stripParens() = this.getVariable().getAReference())
     or
     exists(DataFlow::MethodReadNode mrn |
-      mrn.getReceiver() = this.getARead() and
+      mrn.getReceiver() = this.getVariable().getARead() and
       mrn.getMethod().getReceiverType() instanceof PointerType
     )
     or
     // variables where there is an unresolved reference with the same name in the same
     // scope or a nested scope, suggesting that name resolution information may be incomplete
     exists(FunctionScope scope, FuncDef inner |
-      scope = this.getScope().(LocalScope).getEnclosingFunctionScope() and
-      unresolvedReference(this.getName(), inner) and
+      scope = this.getVariable().getScope().(LocalScope).getEnclosingFunctionScope() and
+      unresolvedReference(this.getVariable().getName(), inner) and
       inner.getScope().getOuterScope*() = scope
     )
   }
@@ -178,6 +214,8 @@ class SsaDefinition instanceof ZZZDefinition {
  * An SSA definition that corresponds to an explicit assignment or other variable definition.
  */
 class SsaExplicitDefinition extends SsaDefinition {
+  SsaExplicitDefinition() { not this instanceof SsaImplicitDefinition }
+
   /** Gets the instruction where the definition happens. */
   IR::Instruction getInstruction() {
     exists(BasicBlock bb, int i | this.definesAt(_, bb, i) | result = bb.getNode(i))
@@ -230,14 +268,21 @@ abstract class SsaImplicitDefinition extends SsaDefinition {
  */
 class SsaVariableCapture extends SsaImplicitDefinition {
   SsaVariableCapture() {
-    exists(BasicBlock bb, int i, SsaSourceVariable v | this.definesAt(v, bb, i) |
-      mayCapture(bb, i, v)
+    exists(BasicBlock bb, int j, SsaSourceVariable w |
+      this.definesAt(w, bb, j) and
+      mayCapture(bb, j, _, w)
     )
   }
 
   override string getKind() { result = "capture" }
 
   override string prettyPrintDef() { result = "capture variable " + this.getSourceVariable() }
+
+  override Location getLocation() {
+    exists(ReachableBasicBlock bb, int i | this.definesAt(_, bb, i) |
+      result = bb.getNode(i).getLocation()
+    )
+  }
 }
 
 /**

Diff for: go/ql/lib/semmle/go/dataflow/SsaImpl.qll

@@ -7,27 +7,42 @@
 import go
 private import codeql.ssa.Ssa as SsaImplCommon
 
+class NonFileScopeLocalVariable extends LocalVariable {
+  NonFileScopeLocalVariable() { not this.getScope() instanceof FileScope }
+}
+
+cached
+newtype TSsaSourceVariable =
+  MkNonCapturedVariableVariable(NonFileScopeLocalVariable v) or
+  MkCapturedVariableVariable(FuncDef root, NonFileScopeLocalVariable v) { v.isCaptured(root) }
+
 cached
 private module Internal {
   /** Holds if the `i`th node of `bb` defines `v`. */
   cached
   predicate defAt(ReachableBasicBlock bb, int i, SsaSourceVariable v) {
-    bb.getNode(i).(IR::Instruction).writes(v, _)
+    exists(IR::Instruction inst | inst = bb.getNode(i).(IR::Instruction) |
+      inst.writes(v.getVariable(), _) and
+      inst.getRoot() = v.getRoot()
+    )
   }
 
   /** Holds if the `i`th node of `bb` reads `v`. */
   cached
   predicate useAt(ReachableBasicBlock bb, int i, SsaSourceVariable v) {
-    bb.getNode(i).(IR::Instruction).reads(v)
+    exists(IR::Instruction inst | inst = bb.getNode(i).(IR::Instruction) |
+      inst.reads(v.getVariable()) and
+      inst.getRoot() = v.getRoot()
+    )
   }
 
   /**
    * Holds if `v` is a captured variable which is declared in `declFun` and read in `useFun`.
    */
-  private predicate readsCapturedVar(FuncDef useFun, SsaSourceVariable v, FuncDef declFun) {
+  private predicate readsCapturedVar(FuncDef useFun, NonFileScopeLocalVariable v, FuncDef declFun) {
     declFun = v.getDeclaringFunction() and
     useFun = any(IR::Instruction u | u.reads(v)).getRoot() and
-    v.isCaptured()
+    v.isCaptured(_)
   }
 
   /** Holds if the `i`th node of `bb` in function `f` is an entry node. */
@@ -50,8 +65,15 @@ private module Internal {
    * introduced depends on whether `v` is live at this point in the program.
    */
   cached
-  predicate mayCapture(ReachableBasicBlock bb, int i, SsaSourceVariable v) {
-    exists(FuncDef capturingContainer, FuncDef declContainer |
+  predicate mayCapture(
+    ReachableBasicBlock bb, int i, SsaSourceVariable capturedVar, SsaSourceVariable closureVar
+  ) {
+    exists(LocalVariable v, FuncDef capturingContainer, FuncDef declContainer |
+      v = capturedVar.getVariable() and
+      v = closureVar.getVariable() and
+      declContainer = capturedVar.getRoot() and
+      capturingContainer = closureVar.getRoot()
+    |
       // capture initial value of variable declared in enclosing scope
       readsCapturedVar(capturingContainer, v, declContainer) and
       capturingContainer != declContainer and
@@ -76,7 +98,7 @@ private module Internal {
   private predicate ref(ReachableBasicBlock bb, int i, SsaSourceVariable v, RefKind tp) {
     useAt(bb, i, v) and tp = ReadRef()
     or
-    (mayCapture(bb, i, v) or defAt(bb, i, v)) and
+    (mayCapture(bb, i, v, _) or defAt(bb, i, v)) and
     tp = WriteRef()
   }
 
@@ -114,7 +136,7 @@ private module Internal {
   /**
    * Holds if `v` is assigned outside its declaring function.
    */
-  private predicate assignedThroughClosure(SsaSourceVariable v) {
+  private predicate assignedThroughClosure(NonFileScopeLocalVariable v) {
     any(IR::Instruction def | def.writes(v, _)).getRoot() != v.getDeclaringFunction()
   }
 
@@ -407,6 +429,8 @@ private module Internal {
     predicate variableWrite(BasicBlock bb, int i, SourceVariable v, boolean certain) {
       defAt(bb, i, v) and
       certain = true
+      or
+      mayCapture(bb, i, _, v) and certain = true
     }
 
     /**
@@ -417,7 +441,7 @@ private module Internal {
     predicate variableRead(BasicBlock bb, int i, SourceVariable v, boolean certain) {
       useAt(bb, i, v) and certain = true
       or
-      mayCapture(bb, i, v) and certain = false
+      mayCapture(bb, i, v, _) and certain = true
     }
   }
 
@@ -434,4 +458,4 @@ private module Internal {
 
 import Internal
 
-predicate captures = Internal::mayCapture/3;
+predicate captures = Internal::mayCapture/4;

Diff for: go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll

@@ -291,7 +291,7 @@ module Public {
     /** Gets the unique definition of this SSA variable. */
     SsaDefinition getDefinition() { result = ssa }
 
-    override ControlFlow::Root getRoot() { result = ssa.getRoot() }
+    override ControlFlow::Root getRoot() { result = ssa.getSourceVariable().getRoot() }
 
     override Type getType() { result = ssa.getSourceVariable().getType() }
 

Diff for: go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll

@@ -114,7 +114,7 @@ private Field getASparselyUsedChannelTypedField() {
  */
 predicate jumpStep(Node n1, Node n2) {
   exists(ValueEntity v |
-    not v instanceof SsaSourceVariable and
+    not v.getScope() instanceof FunctionScope and
     not v instanceof Field and
     (
       any(Write w).writes(v, n1)

Diff for: go/ql/lib/semmle/go/frameworks/Beego.qll

@@ -53,7 +53,7 @@ module Beego {
     SsaWithFields v;
 
     BeegoOutputInstance() {
-      this = v.getBaseVariable().getSourceVariable() and
+      this = v.getBaseVariable().getSourceVariable().getVariable() and
       v.getType().(PointerType).getBaseType().hasQualifiedName(contextPackagePath(), "BeegoOutput")
     }
 

Diff for: go/ql/lib/semmle/go/frameworks/Fasthttp.qll

@@ -61,7 +61,7 @@ module Fasthttp {
     SsaWithFields v;
 
     ResponseWriter() {
-      this = v.getBaseVariable().getSourceVariable() and
+      this = v.getBaseVariable().getSourceVariable().getVariable() and
       (
         v.getType().hasQualifiedName(packagePath(), ["Response", "ResponseHeader"]) or
         v.getType().(PointerType).getBaseType().hasQualifiedName(packagePath(), "RequestCtx")

Diff for: go/ql/lib/semmle/go/frameworks/GinCors.qll

@@ -127,7 +127,7 @@ module GinCors {
     SsaWithFields v;
 
     GinConfig() {
-      this = v.getBaseVariable().getSourceVariable() and
+      this = v.getBaseVariable().getSourceVariable().getVariable() and
       v.getType().hasQualifiedName(packagePath(), "Config")
     }
 

Diff for: go/ql/lib/semmle/go/frameworks/Macaron.qll

@@ -9,7 +9,7 @@ private module Macaron {
     SsaWithFields v;
 
     Context() {
-      this = v.getBaseVariable().getSourceVariable() and
+      this = v.getBaseVariable().getSourceVariable().getVariable() and
       exists(Method m | m.hasQualifiedName("gopkg.in/macaron.v1", "Context", "Redirect") |
         v.getType().getMethod("Redirect") = m
       )

Diff for: go/ql/lib/semmle/go/frameworks/RsCors.qll

@@ -139,7 +139,7 @@ module RsCors {
     SsaWithFields v;
 
     RsOptions() {
-      this = v.getBaseVariable().getSourceVariable() and
+      this = v.getBaseVariable().getSourceVariable().getVariable() and
       v.getType().hasQualifiedName(packagePath(), "Options")
     }
 

Diff for: go/ql/lib/semmle/go/frameworks/stdlib/NetHttp.qll

@@ -13,7 +13,7 @@ module NetHttp {
     SsaWithFields v;
 
     StdlibResponseWriter() {
-      this = v.getBaseVariable().getSourceVariable() and
+      this = v.getBaseVariable().getSourceVariable().getVariable() and
       exists(Type t | t.implements("net/http", "ResponseWriter") | v.getType() = t)
     }
 

Diff for: go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll

@@ -319,7 +319,7 @@ class TypeSwitchVarFlowStateTransformer extends DataFlow::SsaNode, FlowStateTran
 
   TypeSwitchVarFlowStateTransformer() {
     exists(IR::TypeSwitchImplicitVariableInstruction insn, LocalVariable lv | insn.writes(lv, _) |
-      this.getSourceVariable() = lv and
+      this.getSourceVariable().getVariable() = lv and
       it = lv.getType().getUnderlyingType()
     )
   }

Diff for: go/ql/src/RedundantCode/DeadStoreOfLocal.ql

@@ -29,14 +29,14 @@ predicate isSimple(IR::Instruction nd) {
 
 from IR::Instruction def, SsaSourceVariable target, IR::Instruction rhs
 where
-  def.writes(target, rhs) and
+  def.writes(target.getVariable(), rhs) and
   not exists(SsaExplicitDefinition ssa | ssa.getInstruction() = def) and
   // exclude assignments in dead code
   def.getBasicBlock() instanceof ReachableBasicBlock and
   // exclude assignments with default values or simple expressions
   not isSimple(rhs) and
   // exclude variables that are not used at all
-  exists(target.getAReference()) and
+  exists(target.getVariable().getAReference()) and
   // exclude variables with indirect references
   not target.mayHaveIndirectReferences()
 select def, "This definition of " + target + " is never used."