Desugar setter assignment operations

2026-04-20 22:44:52 +02:00 · 2021-05-18 16:07:46 +02:00
parent b173cc332a
commit 3f412e4fad
8 changed files with 367 additions and 35 deletions
--- a/ql/src/codeql_ruby/ast/Call.qll
+++ b/ql/src/codeql_ruby/ast/Call.qll
@@ -92,7 +92,7 @@ class MethodCall extends Call, TMethodCall {
   */
  Block getBlock() { none() }

-  override string toString() { result = "call to " + concat(this.getMethodName(), "/") }
+  override string toString() { result = "call to " + this.getMethodName() }

  final override AstNode getAChild(string pred) {
    result = super.getAChild(pred)
@@ -115,6 +115,8 @@ class SetterMethodCall extends MethodCall {
    this instanceof LhsExpr
    or
    this = any(Assignment a).getDesugared()
+    or
+    this = any(Assignment a).getDesugared().(StmtSequence).getAStmt()
  }

  final override string getAPrimaryQlClass() { result = "SetterMethodCall" }
--- a/ql/src/codeql_ruby/ast/internal/Synthesis.qll
+++ b/ql/src/codeql_ruby/ast/internal/Synthesis.qll
@@ -82,6 +82,30 @@ class Synthesis extends TSynthesis {
  final string toString() { none() }
 }

+/**
+ * Use this predicate in `Synthesis::child` to generate an assignment of `value` to
+ * synthesized variable `v`, where the assignment is a child of `assignParent` at
+ * index `assignIndex`.
+ */
+bindingset[v, assignParent, assignIndex, value]
+private predicate assign(
+  AstNode parent, int i, Child child, TLocalVariableSynth v, AstNode assignParent, int assignIndex,
+  AstNode value
+) {
+  parent = assignParent and
+  i = assignIndex and
+  child = SynthChild(AssignExprKind())
+  or
+  parent = getSynthChild(assignParent, assignIndex) and
+  (
+    i = 0 and
+    child = SynthChild(LocalVariableAccessSynthKind(v))
+    or
+    i = 1 and
+    child = RealChild(value)
+  )
+}
+
 private SynthKind getCallKind(MethodCall mc) {
  result = MethodCallKind(methodCallName(mc))
  or
@@ -253,4 +277,130 @@ private module AssignOperationDesugar {
      )
    }
  }
+
+  /** Gets an assignment operation where the LHS is method call `mc`. */
+  private AssignOperation assignOperationMethodCall(MethodCallReal mc) {
+    result.getLeftOperand() = mc
+  }
+
+  /**
+   * ```rb
+   * foo[bar] += y
+   * ```
+   *
+   * desguars to
+   *
+   * ```rb
+   * __synth__0 = foo;
+   * __synth__1 = bar;
+   * __synth__2 = __synth__0.[](__synth__1) + y;
+   * __synth__0.[]=(__synth__1, __synth__2);
+   * __synth__2;
+   * ```
+   */
+  private class MethodCallAssignOperationSynthesis extends Synthesis {
+    final override predicate child(AstNode parent, int i, Child child, LocationOption l) {
+      exists(AssignOperation ao, MethodCallReal mc | ao = assignOperationMethodCall(mc) |
+        parent = ao and
+        i = -1 and
+        child = SynthChild(StmtSequenceKind()) and
+        l = NoneLocation()
+        or
+        exists(AstNode seq, AstNode receiver |
+          seq = getSynthChild(ao, -1) and receiver = mc.getReceiverReal()
+        |
+          // `__synth__0 = foo`
+          assign(parent, i, child, TLocalVariableSynth(ao, 0), seq, 0, receiver) and
+          l = getSomeLocation(receiver)
+          or
+          // `__synth__1 = bar`
+          exists(Expr arg, int j | arg = mc.getArgumentReal(j - 1) |
+            assign(parent, i, child, TLocalVariableSynth(ao, j), seq, j, arg) and
+            l = getSomeLocation(arg)
+          )
+          or
+          // `__synth__2 = __synth__0.[](__synth__1) + y`
+          exists(int opAssignIndex | opAssignIndex = mc.getNumberOfArgumentsReal() + 1 |
+            parent = seq and
+            i = opAssignIndex and
+            child = SynthChild(AssignExprKind()) and
+            l = SomeLocation(getAssignOperationLocation(ao))
+            or
+            exists(AstNode assign | assign = getSynthChild(seq, opAssignIndex) |
+              parent = assign and
+              i = 0 and
+              child =
+                SynthChild(LocalVariableAccessSynthKind(TLocalVariableSynth(ao, opAssignIndex))) and
+              l = SomeLocation(getAssignOperationLocation(ao))
+              or
+              parent = assign and
+              i = 1 and
+              child = SynthChild(getKind(ao)) and
+              l = SomeLocation(getAssignOperationLocation(ao))
+              or
+              // `__synth__0.[](__synth__1) + y`
+              exists(AstNode op | op = getSynthChild(assign, 1) |
+                parent = op and
+                i = 0 and
+                child = SynthChild(getCallKind(mc)) and
+                l = getSomeLocation(mc)
+                or
+                parent = getSynthChild(op, 0) and
+                child = SynthChild(LocalVariableAccessSynthKind(TLocalVariableSynth(ao, i))) and
+                (
+                  i = 0 and
+                  l = getSomeLocation(receiver)
+                  or
+                  l = getSomeLocation(mc.getArgumentReal(i - 1))
+                )
+                or
+                parent = op and
+                i = 1 and
+                child = RealChild(ao.getRightOperand()) and
+                l = NoneLocation()
+              )
+            )
+            or
+            // `__synth__0.[]=(__synth__1, __synth__2);`
+            parent = seq and
+            i = opAssignIndex + 1 and
+            child = SynthChild(getCallKind(mc)) and
+            l = getSomeLocation(mc)
+            or
+            exists(AstNode setter | setter = getSynthChild(seq, opAssignIndex + 1) |
+              parent = setter and
+              child = SynthChild(LocalVariableAccessSynthKind(TLocalVariableSynth(ao, i))) and
+              (
+                i = 0 and
+                l = getSomeLocation(receiver)
+                or
+                l = getSomeLocation(mc.getArgumentReal(i - 1))
+              )
+              or
+              parent = setter and
+              i = opAssignIndex + 1 and
+              child =
+                SynthChild(LocalVariableAccessSynthKind(TLocalVariableSynth(ao, opAssignIndex))) and
+              l = SomeLocation(getAssignOperationLocation(ao))
+            )
+            or
+            parent = seq and
+            i = opAssignIndex + 2 and
+            child = SynthChild(LocalVariableAccessSynthKind(TLocalVariableSynth(ao, opAssignIndex))) and
+            l = SomeLocation(getAssignOperationLocation(ao))
+          )
+        )
+      )
+    }
+
+    final override predicate localVariable(AstNode n, int i) {
+      exists(MethodCallReal mc | n = assignOperationMethodCall(mc) |
+        i in [0 .. mc.getNumberOfArgumentsReal() + 1]
+      )
+    }
+
+    final override predicate excludeFromControlFlowTree(AstNode n) {
+      exists(assignOperationMethodCall(n))
+    }
+  }
 }
--- a/ql/src/codeql_ruby/controlflow/CfgNodes.qll
+++ b/ql/src/codeql_ruby/controlflow/CfgNodes.qll
@@ -246,6 +246,9 @@ module ExprNodes {
    /** Gets the `n`th argument of this call. */
    final ExprCfgNode getArgument(int n) { e.hasCfgChild(e.getArgument(n), this, result) }

+    /** Gets the number of arguments of this call. */
+    final int getNumberOfArguments() { result = e.getNumberOfArguments() }
+
    /** Gets the receiver of this call. */
    final ExprCfgNode getReceiver() { e.hasCfgChild(e.(MethodCall).getReceiver(), this, result) }
  }
--- a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll
+++ b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll
@@ -286,16 +286,6 @@ module Trees {
    final override ControlFlowTree getChildNode(int i) { result = this.getElement(i) }
  }

-  private class AssignOperationTree extends StandardPostOrderTree, AssignOperation {
-    AssignOperationTree() { not this.getLeftOperand() instanceof VariableAccess }
-
-    final override ControlFlowTree getChildNode(int i) {
-      result = this.getLeftOperand() and i = 0
-      or
-      result = this.getRightOperand() and i = 1
-    }
-  }
-
  private class AssignExprTree extends StandardPostOrderTree, AssignExpr {
    AssignExprTree() { not this.getLeftOperand() instanceof MethodCall }

--- a/ql/src/codeql_ruby/typetracking/TypeTrackerSpecific.qll
+++ b/ql/src/codeql_ruby/typetracking/TypeTrackerSpecific.qll
@@ -72,12 +72,12 @@ predicate returnStep(DataFlowPrivate::ReturnNode nodeFrom, Node nodeTo) {
 */
 predicate basicStoreStep(Node nodeFrom, LocalSourceNode nodeTo, string content) {
  // TODO: support SetterMethodCall inside TuplePattern
-  exists(ExprNodes::AssignmentCfgNode assignment, ExprNodes::MethodCallCfgNode call |
-    assignment.getLhs() = call and
+  exists(ExprNodes::MethodCallCfgNode call |
    content = getSetterCallAttributeName(call.getExpr()) and
    nodeTo.(DataFlowPublic::ExprNode).getExprNode() = call.getReceiver() and
    call.getExpr() instanceof AST::SetterMethodCall and
-    assignment.getRhs() = nodeFrom.(DataFlowPublic::ExprNode).getExprNode()
+    call.getArgument(call.getNumberOfArguments() - 1) =
+      nodeFrom.(DataFlowPublic::ExprNode).getExprNode()
  )
 }

--- a/ql/test/library-tests/ast/AstDesugar.expected
+++ b/ql/test/library-tests/ast/AstDesugar.expected
@@ -19,6 +19,77 @@ calls/calls.rb:
 #  315|     getReceiver: [Self] self
 #  315|   getArgument: [IntegerLiteral] 0
 #  315|   getArgument: [IntegerLiteral] 10
+#  318| [StmtSequence] ...
+#  318|   getStmt: [SetterMethodCall] call to count=
+#  318|     getReceiver: [LocalVariableAccess] __synth__0
+#  318|     getArgument: [LocalVariableAccess] __synth__1
+#  318|   getStmt: [AssignExpr] ... = ...
+#  318|     getAnOperand/getRightOperand: [Self] self
+#  318|     getAnOperand/getLeftOperand: [LocalVariableAccess] __synth__0
+#  318|   getStmt: [AssignExpr] ... = ...
+#  318|     getAnOperand/getRightOperand: [AddExpr] ... + ...
+#  318|       getAnOperand/getLeftOperand: [MethodCall] call to count
+#  318|         getReceiver: [LocalVariableAccess] __synth__0
+#  318|       getAnOperand/getRightOperand: [IntegerLiteral] 1
+#  318|     getAnOperand/getLeftOperand: [LocalVariableAccess] __synth__1
+#  318|   getStmt: [LocalVariableAccess] __synth__1
+#  319| [StmtSequence] ...
+#  319|   getStmt: [AssignExpr] ... = ...
+#  319|     getAnOperand/getRightOperand: [MethodCall] call to foo
+#  319|       getReceiver: [Self] self
+#  319|     getAnOperand/getLeftOperand: [LocalVariableAccess] __synth__0
+#  319|   getStmt: [ElementReference, SetterMethodCall] ...[...]
+#  319|     getReceiver: [LocalVariableAccess] __synth__0
+#  319|     getArgument: [LocalVariableAccess] __synth__1
+#  319|     getArgument: [LocalVariableAccess] __synth__2
+#  319|   getStmt: [AssignExpr] ... = ...
+#  319|     getAnOperand/getRightOperand: [IntegerLiteral] 0
+#  319|     getAnOperand/getLeftOperand: [LocalVariableAccess] __synth__1
+#  319|   getStmt: [AssignExpr] ... = ...
+#  319|     getAnOperand/getRightOperand: [AddExpr] ... + ...
+#  319|       getAnOperand/getLeftOperand: [ElementReference] ...[...]
+#  319|         getReceiver: [LocalVariableAccess] __synth__0
+#  319|         getArgument: [LocalVariableAccess] __synth__1
+#  319|       getAnOperand/getRightOperand: [IntegerLiteral] 1
+#  319|     getAnOperand/getLeftOperand: [LocalVariableAccess] __synth__2
+#  319|   getStmt: [LocalVariableAccess] __synth__2
+#  320| [StmtSequence] ...
+#  320|   getStmt: [AssignExpr] ... = ...
+#  320|     getAnOperand/getRightOperand: [MethodCall] call to bar
+#  320|       getReceiver: [MethodCall] call to foo
+#  320|         getReceiver: [Self] self
+#  320|     getAnOperand/getLeftOperand: [LocalVariableAccess] __synth__0
+#  320|   getStmt: [ElementReference, SetterMethodCall] ...[...]
+#  320|     getReceiver: [LocalVariableAccess] __synth__0
+#  320|     getArgument: [LocalVariableAccess] __synth__1
+#  320|     getArgument: [LocalVariableAccess] __synth__2
+#  320|     getArgument: [LocalVariableAccess] __synth__3
+#  320|     getArgument: [LocalVariableAccess] __synth__4
+#  320|   getStmt: [AssignExpr] ... = ...
+#  320|     getAnOperand/getRightOperand: [IntegerLiteral] 0
+#  320|     getAnOperand/getLeftOperand: [LocalVariableAccess] __synth__1
+#  320|   getStmt: [AssignExpr] ... = ...
+#  320|     getAnOperand/getRightOperand: [MethodCall] call to baz
+#  320|       getReceiver: [MethodCall] call to foo
+#  320|         getReceiver: [Self] self
+#  320|     getAnOperand/getLeftOperand: [LocalVariableAccess] __synth__2
+#  320|   getStmt: [AssignExpr] ... = ...
+#  320|     getAnOperand/getRightOperand: [AddExpr] ... + ...
+#  320|       getAnOperand/getLeftOperand: [MethodCall] call to boo
+#  320|         getReceiver: [MethodCall] call to foo
+#  320|           getReceiver: [Self] self
+#  320|       getAnOperand/getRightOperand: [IntegerLiteral] 1
+#  320|     getAnOperand/getLeftOperand: [LocalVariableAccess] __synth__3
+#  320|   getStmt: [AssignExpr] ... = ...
+#  320|     getAnOperand/getRightOperand: [MulExpr] ... * ...
+#  320|       getAnOperand/getLeftOperand: [ElementReference] ...[...]
+#  320|         getReceiver: [LocalVariableAccess] __synth__0
+#  320|         getArgument: [LocalVariableAccess] __synth__1
+#  320|         getArgument: [LocalVariableAccess] __synth__2
+#  320|         getArgument: [LocalVariableAccess] __synth__3
+#  320|       getAnOperand/getRightOperand: [IntegerLiteral] 2
+#  320|     getAnOperand/getLeftOperand: [LocalVariableAccess] __synth__4
+#  320|   getStmt: [LocalVariableAccess] __synth__4
 control/loops.rb:
 #   10| [AssignExpr] ... = ...
 #   10|   getAnOperand/getLeftOperand: [LocalVariableAccess] sum
--- a/ql/test/library-tests/ast/calls/calls.expected
+++ b/ql/test/library-tests/ast/calls/calls.expected
@@ -37,10 +37,21 @@ callsWithArguments
 | calls.rb:315:1:315:6 | ...[...] | []= | 1 | calls.rb:315:10:315:11 | 10 |
 | calls.rb:316:22:316:27 | ...[...] | []= | 0 | calls.rb:316:26:316:26 | 4 |
 | calls.rb:317:5:317:10 | ...[...] | []= | 0 | calls.rb:317:9:317:9 | 5 |
+| calls.rb:318:1:318:10 | call to count= | count= | 1 | calls.rb:318:12:318:13 | __synth__1 |
+| calls.rb:319:1:319:6 | ...[...] | [] | 0 | calls.rb:319:5:319:5 | __synth__1 |
 | calls.rb:319:1:319:6 | ...[...] | []= | 0 | calls.rb:319:5:319:5 | 0 |
+| calls.rb:319:1:319:6 | ...[...] | []= | 0 | calls.rb:319:5:319:5 | __synth__1 |
+| calls.rb:319:1:319:6 | ...[...] | []= | 2 | calls.rb:319:8:319:9 | __synth__2 |
+| calls.rb:320:1:320:32 | ...[...] | [] | 0 | calls.rb:320:9:320:9 | __synth__1 |
+| calls.rb:320:1:320:32 | ...[...] | [] | 1 | calls.rb:320:12:320:18 | __synth__2 |
+| calls.rb:320:1:320:32 | ...[...] | [] | 2 | calls.rb:320:21:320:31 | __synth__3 |
 | calls.rb:320:1:320:32 | ...[...] | []= | 0 | calls.rb:320:9:320:9 | 0 |
+| calls.rb:320:1:320:32 | ...[...] | []= | 0 | calls.rb:320:9:320:9 | __synth__1 |
+| calls.rb:320:1:320:32 | ...[...] | []= | 1 | calls.rb:320:12:320:18 | __synth__2 |
 | calls.rb:320:1:320:32 | ...[...] | []= | 1 | calls.rb:320:12:320:18 | call to baz |
 | calls.rb:320:1:320:32 | ...[...] | []= | 2 | calls.rb:320:21:320:31 | ... + ... |
+| calls.rb:320:1:320:32 | ...[...] | []= | 2 | calls.rb:320:21:320:31 | __synth__3 |
+| calls.rb:320:1:320:32 | ...[...] | []= | 4 | calls.rb:320:34:320:35 | __synth__4 |
 callsWithReceiver
 | calls.rb:2:1:2:5 | call to foo | calls.rb:2:1:2:5 | self |
 | calls.rb:5:1:5:10 | call to bar | calls.rb:5:1:5:3 | Foo |
@@ -220,11 +231,17 @@ callsWithReceiver
 | calls.rb:316:22:316:27 | ...[...] | calls.rb:316:22:316:24 | call to foo |
 | calls.rb:317:5:317:7 | call to foo | calls.rb:317:5:317:7 | self |
 | calls.rb:317:5:317:10 | ...[...] | calls.rb:317:5:317:7 | call to foo |
+| calls.rb:318:1:318:10 | call to count | calls.rb:318:1:318:4 | __synth__0 |
+| calls.rb:318:1:318:10 | call to count= | calls.rb:318:1:318:4 | __synth__0 |
 | calls.rb:318:1:318:10 | call to count= | calls.rb:318:1:318:4 | self |
 | calls.rb:319:1:319:3 | call to foo | calls.rb:319:1:319:3 | self |
+| calls.rb:319:1:319:6 | ...[...] | calls.rb:319:1:319:3 | __synth__0 |
+| calls.rb:319:1:319:6 | ...[...] | calls.rb:319:1:319:3 | __synth__0 |
 | calls.rb:319:1:319:6 | ...[...] | calls.rb:319:1:319:3 | call to foo |
 | calls.rb:320:1:320:3 | call to foo | calls.rb:320:1:320:3 | self |
 | calls.rb:320:1:320:7 | call to bar | calls.rb:320:1:320:3 | call to foo |
+| calls.rb:320:1:320:32 | ...[...] | calls.rb:320:1:320:7 | __synth__0 |
+| calls.rb:320:1:320:32 | ...[...] | calls.rb:320:1:320:7 | __synth__0 |
 | calls.rb:320:1:320:32 | ...[...] | calls.rb:320:1:320:7 | call to bar |
 | calls.rb:320:12:320:14 | call to foo | calls.rb:320:12:320:14 | self |
 | calls.rb:320:12:320:18 | call to baz | calls.rb:320:12:320:14 | call to foo |
@@ -277,5 +294,8 @@ setterCalls
 | calls.rb:316:22:316:27 | ...[...] |
 | calls.rb:317:5:317:10 | ...[...] |
 | calls.rb:318:1:318:10 | call to count= |
+| calls.rb:318:1:318:10 | call to count= |
+| calls.rb:319:1:319:6 | ...[...] |
 | calls.rb:319:1:319:6 | ...[...] |
 | calls.rb:320:1:320:32 | ...[...] |
+| calls.rb:320:1:320:32 | ...[...] |
--- a/ql/test/library-tests/controlflow/graph/Cfg.expected
+++ b/ql/test/library-tests/controlflow/graph/Cfg.expected
@@ -2179,22 +2179,52 @@ desugar.rb:
 #-----|  -> exit m4

 #   13| x
-#-----|  -> x
-
-#   14| ... += ...
-#-----|  -> exit m4 (normal)
-
-#   14| call to count=
-#-----|  -> 1
+#-----|  -> __synth__0

 #   14| call to foo
-#-----|  -> call to count=
+#-----|  -> ... = ...

 #   14| x
 #-----|  -> call to foo

+#   14| ...
+#-----|  -> exit m4 (normal)
+
+#   14| call to count=
+#-----|  -> __synth__1
+
+#   14| ... = ...
+#-----|  -> __synth__1
+
+#   14| __synth__0
+#-----|  -> __synth__1
+
+#   14| __synth__0
+#-----|  -> x
+
+#   14| call to count
+#-----|  -> 1
+
+#   14| __synth__0
+#-----|  -> call to count
+
+#   14| ... = ...
+#-----|  -> __synth__0
+
+#   14| __synth__1
+#-----|  -> ...
+
+#   14| ... + ...
+#-----|  -> ... = ...
+
+#   14| __synth__1
+#-----|  -> call to count=
+
+#   14| __synth__1
+#-----|  -> __synth__0
+
 #   14| 1
-#-----|  -> ... += ...
+#-----|  -> ... + ...

 #   17| enter m5
 #-----|  -> x
@@ -2211,31 +2241,70 @@ desugar.rb:
 #-----|  -> y

 #   17| y
-#-----|  -> x
-
-#   18| ... += ...
-#-----|  -> exit m5 (normal)
-
-#   18| ...[...]
-#-----|  -> 1
+#-----|  -> __synth__0

 #   18| call to foo
-#-----|  -> 0
+#-----|  -> ... = ...

 #   18| x
 #-----|  -> call to foo

+#   18| ...
+#-----|  -> exit m5 (normal)
+
+#   18| ... = ...
+#-----|  -> __synth__1
+
+#   18| ...[...]
+#-----|  -> __synth__4
+
+#   18| __synth__0
+#-----|  -> x
+
+#   18| __synth__0
+#-----|  -> __synth__1
+
+#   18| ...[...]
+#-----|  -> 1
+
+#   18| __synth__0
+#-----|  -> __synth__1
+
 #   18| 0
-#-----|  -> y
+#-----|  -> ... = ...
+
+#   18| ... = ...
+#-----|  -> __synth__2
+
+#   18| __synth__1
+#-----|  -> 0
+
+#   18| __synth__1
+#-----|  -> __synth__2
+
+#   18| __synth__1
+#-----|  -> __synth__2

 #   18| call to bar
-#-----|  -> x
+#-----|  -> ... = ...

 #   18| y
 #-----|  -> call to bar

+#   18| ... = ...
+#-----|  -> __synth__3
+
+#   18| __synth__2
+#-----|  -> y
+
+#   18| __synth__2
+#-----|  -> __synth__3
+
+#   18| __synth__2
+#-----|  -> __synth__3
+
 #   18| ... + ...
-#-----|  -> ...[...]
+#-----|  -> ... = ...

 #   18| call to baz
 #-----|  -> 3
@@ -2243,11 +2312,38 @@ desugar.rb:
 #   18| x
 #-----|  -> call to baz

+#   18| ... = ...
+#-----|  -> __synth__4
+
+#   18| __synth__3
+#-----|  -> x
+
+#   18| __synth__3
+#-----|  -> __synth__4
+
+#   18| __synth__3
+#-----|  -> ...[...]
+
 #   18| 3
 #-----|  -> ... + ...

+#   18| ... = ...
+#-----|  -> __synth__0
+
+#   18| __synth__4
+#-----|  -> ...
+
+#   18| ... + ...
+#-----|  -> ... = ...
+
+#   18| __synth__4
+#-----|  -> __synth__0
+
+#   18| __synth__4
+#-----|  -> ...[...]
+
 #   18| 1
-#-----|  -> ... += ...
+#-----|  -> ... + ...

 #   21| X
 #-----|  -> $global_var