Merge pull request #1 from hvitved/rb/data-flow-layer-capture2

Ruby: Make sure to always generate SSA definitions for namespace self-variables

Author: asgerf

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll

@@ -262,9 +262,6 @@ private module Cached {
       )
     } or
     TSsaDefinitionNode(Ssa::Definition def) or
-    TRawNamespaceSelf(Namespace ns) {
-      not exists(Ssa::SelfDefinition def | def.getSourceVariable() = ns.getModuleSelfVariable())
-    } or
     TNormalParameterNode(Parameter p) {
       p instanceof SimpleParameter or
       p instanceof OptionalParameter or
@@ -405,8 +402,6 @@ private module Cached {
     or
     // Needed for stores in type tracking
     TypeTrackerSpecific::storeStepIntoSourceNode(_, n, _)
-    or
-    n instanceof TRawNamespaceSelf
   }
 
   cached
@@ -528,38 +523,6 @@ class SsaSelfDefinitionNode extends LocalSourceNode, SsaDefinitionNode {
   Scope getSelfScope() { result = self.getDeclaringScope() }
 }
 
-/**
- * A representative for the created or re-opened class/module in a `Namespace` that does
- * not have an SSA definition for `self`.
- */
-class RawNamespaceSelf extends NodeImpl, TRawNamespaceSelf {
-  private Namespace namespace;
-
-  RawNamespaceSelf() { this = TRawNamespaceSelf(namespace) }
-
-  /** Gets the namespace for which this represents the created or re-opened class/module. */
-  Namespace getNamespace() { result = namespace }
-
-  override CfgScope getCfgScope() { result = namespace.getCfgScope() }
-
-  override Location getLocationImpl() { result = namespace.getLocation() }
-
-  override string toStringImpl() { result = namespace.toString() }
-}
-
-/**
- * Gets a representative for the created or re-opened class/module in a `Namespace`.
- *
- * This is usually the SSA definition for `self`, but if this node does not exist due to lack of liveness,
- * it is the `RawNamespaceSelf` node.
- */
-LocalSourceNode getNamespaceSelf(Namespace namespace) {
-  result.(SsaDefinitionNode).getDefinition().(Ssa::SelfDefinition).getSourceVariable() =
-    namespace.getModuleSelfVariable()
-  or
-  result = TRawNamespaceSelf(namespace)
-}
-
 /**
  * A value returning statement, viewed as a node in a data flow graph.
  *

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll

@@ -370,9 +370,11 @@ private module Cached {
   LocalSourceNode getConstantAccessNode(ConstantAccess access) {
     // Namespaces don't evaluate to the constant being accessed, they return the value of their last statement.
     // Use the definition of 'self' in the namespace as the representative in this case.
-    if access instanceof Namespace
-    then result = getNamespaceSelf(access)
-    else result.asExpr().getExpr() = access
+    result.(SsaDefinitionNode).getDefinition().(Ssa::SelfDefinition).getSourceVariable() =
+      access.(Namespace).getModuleSelfVariable()
+    or
+    not access instanceof Namespace and
+    result.asExpr().getExpr() = access
   }
 
   cached

ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll

@@ -1,6 +1,7 @@
 private import codeql.ssa.Ssa as SsaImplCommon
 private import codeql.ruby.AST
 private import codeql.ruby.CFG as Cfg
+private import codeql.ruby.controlflow.internal.ControlFlowGraphImplShared as ControlFlowGraphImplShared
 private import codeql.ruby.ast.Variable
 private import Cfg::CfgNodes::ExprNodes
 
@@ -53,6 +54,9 @@ private module SsaInput implements SsaImplCommon::InputSig {
     or
     capturedExitRead(bb, i, v) and
     certain = false
+    or
+    namespaceSelfExitRead(bb, i, v) and
+    certain = false
   }
 }
 
@@ -94,6 +98,21 @@ private predicate capturedExitRead(Cfg::AnnotatedExitBasicBlock bb, int i, Local
   i = bb.length()
 }
 
+/**
+ * Holds if a pseudo read of namespace self-variable `v` should be inserted
+ * at index `i` in basic block `bb`. We do this to ensure that namespace
+ * self-variables always get an SSA definition.
+ */
+private predicate namespaceSelfExitRead(Cfg::AnnotatedExitBasicBlock bb, int i, SelfVariable v) {
+  exists(Namespace ns, AstNode last |
+    v.getDeclaringScope() = ns and
+    last = ControlFlowGraphImplShared::getAControlFlowExitNode(ns) and
+    if last = ns
+    then bb.getNode(i).getAPredecessor().getNode() = last
+    else bb.getNode(i).getNode() = last
+  )
+}
+
 /**
  * Holds if captured variable `v` is read directly inside `scope`,
  * or inside a (transitively) nested scope of `scope`.

ruby/ql/test/library-tests/dataflow/type-tracker/TypeTracker.expected

@@ -1,5 +1,5 @@
 track
-| type_tracker.rb:1:1:10:3 | Container | type tracker without call steps | type_tracker.rb:1:1:10:3 | Container |
+| type_tracker.rb:1:1:10:3 | self (Container) | type tracker without call steps | type_tracker.rb:1:1:10:3 | self (Container) |
 | type_tracker.rb:1:1:10:3 | self (type_tracker.rb) | type tracker with call steps | type_tracker.rb:18:1:21:3 | self (positional) |
 | type_tracker.rb:1:1:10:3 | self (type_tracker.rb) | type tracker with call steps | type_tracker.rb:18:1:21:3 | self in positional |
 | type_tracker.rb:1:1:10:3 | self (type_tracker.rb) | type tracker with call steps | type_tracker.rb:25:1:28:3 | self (keyword) |
@@ -357,7 +357,7 @@ track
 | type_tracker.rb:52:5:52:13 | ...[...] | type tracker without call steps | type_tracker.rb:34:1:53:3 | return return in throughArray |
 | type_tracker.rb:52:5:52:13 | ...[...] | type tracker without call steps | type_tracker.rb:52:5:52:13 | ...[...] |
 trackEnd
-| type_tracker.rb:1:1:10:3 | Container | type_tracker.rb:1:1:10:3 | Container |
+| type_tracker.rb:1:1:10:3 | self (Container) | type_tracker.rb:1:1:10:3 | self (Container) |
 | type_tracker.rb:1:1:10:3 | self (type_tracker.rb) | type_tracker.rb:1:1:10:3 | self (type_tracker.rb) |
 | type_tracker.rb:1:1:10:3 | self (type_tracker.rb) | type_tracker.rb:18:1:21:3 | self (positional) |
 | type_tracker.rb:1:1:10:3 | self (type_tracker.rb) | type_tracker.rb:18:1:21:3 | self in positional |