Synthesise writes to self for classes/modules

This requires changing the CFG trees for classes and modules from
post-order to pre-order so that we can place the writes at the root node
of the tree, to prevent them overlapping with reads in the body of the
class/module.

We need to do this because classes and modules don't define their own
basic block, but re-use the surrounding one. This problem doesn't occur
for `self` variables in methods because each method has its own basic
block and we can place the write on the entry node of the bock.

Author: hmac

ruby/ql/lib/codeql/ruby/ast/Statement.qll

@@ -14,6 +14,9 @@ class Stmt extends AstNode, TStmt {
   /** Gets a control-flow node for this statement, if any. */
   CfgNodes::AstCfgNode getAControlFlowNode() { result.getNode() = this }
 
+  /** Gets a control-flow entry node for this statement, if any */
+  AstNode getAControlFlowEntryNode() { result = getAControlFlowEntryNode(this) }
+
   /** Gets the control-flow scope of this statement, if any. */
   CfgScope getCfgScope() { result = getCfgScope(this) }
 

ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll

@@ -808,19 +808,28 @@ module Trees {
     }
   }
 
+  /**
+   * Namespaces (i.e. modules or classes) behave like other `BodyStmt`s except they are
+   * executed in pre-order rather than post-order. We do this in order to insert a write for
+   * `self` before any subsequent reads in the namespace body.
+   */
   private class NamespaceTree extends BodyStmtTree, Namespace {
-    final override predicate first(AstNode first) {
-      this.firstInner(first)
-      or
-      not exists(this.getAChild(_)) and
-      first = this
-    }
+    final override predicate first(AstNode first) { first = this }
 
     final override predicate succ(AstNode pred, AstNode succ, Completion c) {
       BodyStmtTree.super.succ(pred, succ, c)
       or
-      succ = this and
-      this.lastInner(pred, c)
+      pred = this and
+      this.firstInner(succ) and
+      c instanceof SimpleCompletion
+    }
+
+    final override predicate last(AstNode last, Completion c) {
+      this.lastInner(last, c)
+      or
+      not exists(this.getAChild(_)) and
+      last = this and
+      c.isValidFor(this)
     }
   }
 

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

@@ -23,10 +23,14 @@ class SourceVariable = LocalVariable;
  */
 predicate variableWrite(BasicBlock bb, int i, SourceVariable v, boolean certain) {
   (
-    // We consider the `self` variable to have a single write at the entry to a method block.
+    // We consider the `self` variable to have a single write at the entry to a method block...
     v.(SelfVariable).getDeclaringScope() = bb.(BasicBlocks::EntryBasicBlock).getScope() and
     i = 0
     or
+    // ...or a class or module block.
+    bb.getNode(i).getNode() =
+      v.(SelfVariable).getDeclaringScope().(ModuleBase).getAControlFlowEntryNode()
+    or
     SsaImpl::uninitializedWrite(bb, i, v)
     or
     SsaImpl::capturedEntryWrite(bb, i, v)

ruby/ql/test/library-tests/controlflow/graph/Cfg.expected

@@ -1056,7 +1056,7 @@ cfg.rb:
 #-----|  -> ... = ...
 
 #   54| ... = ...
-#-----|  -> Object
+#-----|  -> Silly
 
 #   54| character
 #-----|  -> ?\x40
@@ -1065,7 +1065,7 @@ cfg.rb:
 #-----|  -> ... = ...
 
 #   58| Silly
-#-----|  -> x
+#-----|  -> Object
 
 #   58| Object
 #-----|  -> complex
@@ -1293,7 +1293,7 @@ cfg.rb:
 #-----|  -> self
 
 #   69| print
-#-----|  -> Silly
+#-----|  -> x
 
 #   69| exit print
 
@@ -1620,7 +1620,7 @@ cfg.rb:
 #-----|  -> #{...}
 
 #  113| ... if ...
-#-----|  -> @field
+#-----|  -> C
 
 #  113| call to puts
 #-----|  -> ... if ...
@@ -1642,7 +1642,7 @@ cfg.rb:
 #-----|  -> ... > ...
 
 #  115| C
-#-----|  -> swap
+#-----|  -> @field
 
 #  116| ... = ...
 #-----|  -> @@static_field
@@ -1654,7 +1654,7 @@ cfg.rb:
 #-----|  -> ... = ...
 
 #  117| ... = ...
-#-----|  -> C
+#-----|  -> swap
 
 #  117| @@static_field
 #-----|  -> 10
@@ -1663,7 +1663,7 @@ cfg.rb:
 #-----|  -> ... = ...
 
 #  120| ... = ...
-#-----|  -> nothing
+#-----|  -> M
 
 #  120| swap
 #-----|  -> -> { ... }
@@ -1701,7 +1701,7 @@ cfg.rb:
 #-----|  -> call to []
 
 #  122| M
-#-----|  -> EmptyClass
+#-----|  -> nothing
 
 #  123| ... = ...
 #-----|  -> some
@@ -1812,7 +1812,7 @@ cfg.rb:
 #-----|  -> #{...}
 
 #  130| ... = ...
-#-----|  -> M
+#-----|  -> EmptyClass
 
 #  130| Constant
 #-----|  -> 5
@@ -2832,7 +2832,7 @@ desugar.rb:
 #-----|  -> __synth__0
 
 #   25| m7
-#-----|  -> @x
+#-----|  -> X
 
 #   25| exit m7
 
@@ -2939,7 +2939,7 @@ desugar.rb:
 #-----|  -> call to []
 
 #   29| X
-#-----|  -> $global_var
+#-----|  -> @x
 
 #   30| ... = ...
 #-----|  -> @x
@@ -2978,7 +2978,7 @@ desugar.rb:
 #-----|  -> @@y
 
 #   34| ... = ...
-#-----|  -> X
+#-----|  -> $global_var
 
 #   34| @@y
 #-----|  -> 4
@@ -3907,7 +3907,7 @@ loops.rb:
 
 raise.rb:
 #    1| enter raise.rb
-#-----|  -> Exception
+#-----|  -> ExceptionA
 
 #    1| ExceptionA
 #-----|  -> Exception
@@ -3918,13 +3918,13 @@ raise.rb:
 #-----|  -> exit raise.rb
 
 #    1| Exception
-#-----|  -> ExceptionA
+#-----|  -> ExceptionB
 
 #    4| ExceptionB
-#-----|  -> m1
+#-----|  -> Exception
 
 #    4| Exception
-#-----|  -> ExceptionB
+#-----|  -> m1
 
 #    7| enter m1
 #-----|  -> x
@@ -5070,7 +5070,7 @@ raise.rb:
 #-----|  -> self
 
 #  158| m15
-#-----|  -> self
+#-----|  -> C
 
 #  158| exit m15
 
@@ -5134,13 +5134,13 @@ raise.rb:
 #-----| false -> self
 
 #  166| C
-#-----|  -> exit raise.rb (normal)
+#-----|  -> self
 
 #  167| enter m
 #-----|  -> self
 
 #  167| m
-#-----|  -> C
+#-----|  -> exit raise.rb (normal)
 
 #  167| exit m