Merge pull request #157 from owen-mc/isresult-consistency

max-schaefer · web-flow · commit b37bdec66c43 · 2020-05-29T17:13:21.000+01:00
Make FunctionOutput.isResult(0) and CallNode.getResult(0) match single results
diff --git a/ql/src/experimental/frameworks/Gin.qll b/ql/src/experimental/frameworks/Gin.qll
@@ -72,8 +72,6 @@ private module Gin {
           )
         |
           this = call.getResult(0)
-          or
-          this = call.getResult()
         )
         or
         // Field reads:
@@ -104,8 +102,6 @@ private module Gin {
           call.getTarget().hasQualifiedName(packagePath, typeName, ["ByName", "Get"])
         |
           this = call.getResult(0)
-          or
-          this = call.getResult()
         )
       )
     }
diff --git a/ql/src/semmle/go/dataflow/FunctionInputsAndOutputs.qll b/ql/src/semmle/go/dataflow/FunctionInputsAndOutputs.qll
@@ -100,7 +100,11 @@ private class ResultInput extends FunctionInput, TInResult {
 
   override predicate isResult() { index = -1 }
 
-  override predicate isResult(int i) { i = index and i >= 0 }
+  override predicate isResult(int i) {
+    i = 0 and isResult()
+    or
+    i = index and i >= 0
+  }
 
   override DataFlow::Node getEntryNode(DataFlow::CallNode c) {
     exists(DataFlow::PostUpdateNode pun, DataFlow::Node init |
@@ -181,7 +185,11 @@ private class OutResult extends FunctionOutput, TOutResult {
 
   override predicate isResult() { index = -1 }
 
-  override predicate isResult(int i) { i = index and i >= 0 }
+  override predicate isResult(int i) {
+    i = 0 and isResult()
+    or
+    i = index and i >= 0
+  }
 
   override DataFlow::Node getEntryNode(FuncDef f) {
     // return expressions
diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowPrivate.qll b/ql/src/semmle/go/dataflow/internal/DataFlowPrivate.qll
@@ -3,8 +3,7 @@ private import DataFlowUtil
 private import DataFlowImplCommon
 
 private newtype TReturnKind =
-  TSingleReturn() or
-  TMultiReturn(int i) { exists(SignatureType st | exists(st.getResultType(i))) }
+  MkReturnKind(int i) { exists(SignatureType st | exists(st.getResultType(i))) }
 
 /**
  * A return kind. A return kind describes how a value can be returned
@@ -13,23 +12,14 @@ private newtype TReturnKind =
  */
 class ReturnKind extends TReturnKind {
   /** Gets a textual representation of this return kind. */
-  string toString() {
-    this = TSingleReturn() and
-    result = "return"
-    or
-    exists(int i | this = TMultiReturn(i) | result = "return[" + i + "]")
-  }
+  string toString() { exists(int i | this = MkReturnKind(i) | result = "return[" + i + "]") }
 }
 
 /** A data flow node that represents returning a value from a function. */
 class ReturnNode extends ResultNode {
   ReturnKind kind;
 
-  ReturnNode() {
-    exists(int nr | nr = fd.getType().getNumResult() |
-      if nr = 1 then kind = TSingleReturn() else kind = TMultiReturn(i)
-    )
-  }
+  ReturnNode() { kind = MkReturnKind(i) }
 
   /** Gets the kind of this returned value. */
   ReturnKind getKind() { result = kind }
@@ -40,12 +30,7 @@ class OutNode extends DataFlow::Node {
   DataFlow::CallNode call;
   int i;
 
-  OutNode() {
-    this = call.getResult() and
-    i = -1
-    or
-    this = call.getResult(i)
-  }
+  OutNode() { this = call.getResult(i) }
 
   /** Gets the underlying call. */
   DataFlowCall getCall() { result = call.asExpr() }
@@ -56,11 +41,8 @@ class OutNode extends DataFlow::Node {
  * `kind`.
  */
 OutNode getAnOutNode(DataFlowCall call, ReturnKind kind) {
-  exists(DataFlow::CallNode c | c.asExpr() = call |
-    kind = TSingleReturn() and
-    result = c.getResult()
-    or
-    exists(int i | kind = TMultiReturn(i) | result = c.getResult(i))
+  exists(DataFlow::CallNode c, int i | c.asExpr() = call and kind = MkReturnKind(i) |
+    result = c.getResult(i)
   )
 }
 
diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
@@ -327,8 +327,15 @@ class CallNode extends ExprNode {
   /** Gets a function passed as the `i`th argument of this call. */
   FunctionNode getCallback(int i) { result.getASuccessor*() = this.getArgument(i) }
 
-  /** Gets the data-flow node corresponding to the `i`th result of this call. */
-  Node getResult(int i) { result = extractTupleElement(this, i) }
+  /**
+   * Gets the data-flow node corresponding to the `i`th result of this call.
+   *
+   * If there is a single result then it is considered to be the 0th result. */
+  Node getResult(int i) {
+    i = 0 and result = getResult()
+    or
+    result = extractTupleElement(this, i)
+  }
 
   /**
    * Gets the data-flow node corresponding to the result of this call.
@@ -338,6 +345,9 @@ class CallNode extends ExprNode {
    */
   Node getResult() { not getType() instanceof TupleType and result = this }
 
+  /** Gets a result of this call. */
+  Node getAResult() { result = this.getResult(_) }
+
   /** Gets the data flow node corresponding to the receiver of this call, if any. */
   Node getReceiver() { result = getACalleeSource().(MethodReadNode).getReceiver() }
 }
diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll
@@ -62,7 +62,7 @@ module PathFilePath {
 
     override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) {
       inp.isParameter(_) and
-      (outp.isResult() or outp.isResult(_))
+      outp.isResult(_)
     }
   }
 }
@@ -472,7 +472,7 @@ module Path {
 
     override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) {
       inp.isParameter(_) and
-      (outp.isResult() or outp.isResult(_))
+      outp.isResult(_)
     }
   }
 }
@@ -684,8 +684,7 @@ module URL {
     }
 
     override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) {
-      inp.isReceiver() and
-      if getName() = "Password" then outp.isResult(0) else outp.isResult()
+      inp.isReceiver() and outp.isResult(0)
     }
   }
 
diff --git a/ql/src/semmle/go/security/ReflectedXssCustomizations.qll b/ql/src/semmle/go/security/ReflectedXssCustomizations.qll
@@ -75,7 +75,7 @@ module ReflectedXss {
         pred.getStringValue().regexpMatch("^[^<].*")
         or
         // json data cannot begin with `<`
-        pred = any(EncodingJson::MarshalFunction mf).getOutput().getExitNode(_)
+        exists(EncodingJson::MarshalFunction mf | pred = mf.getOutput().getNode(mf.getACall()))
       )
     )
   }
diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getEntryNode.expected b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionInput_getEntryNode.expected
@@ -16,3 +16,4 @@
 | receiver | reset.go:12:2:12:21 | call to Reset | reset.go:12:2:12:7 | reader |
 | receiver | tst.go:10:2:10:29 | call to ReadFrom | tst.go:10:2:10:12 | bytesBuffer |
 | result | tst.go:9:17:9:33 | call to new | tst.go:9:2:9:12 | definition of bytesBuffer |
+| result 0 | tst.go:9:17:9:33 | call to new | tst.go:9:2:9:12 | definition of bytesBuffer |
diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_getExitNode.expected
@@ -7,7 +7,11 @@
 | result | main.go:53:2:53:22 | call to op2 | main.go:53:2:53:22 | call to op2 |
 | result | main.go:53:14:53:21 | call to bump | main.go:53:14:53:21 | call to bump |
 | result | tst.go:9:17:9:33 | call to new | tst.go:9:17:9:33 | call to new |
+| result 0 | main.go:51:2:51:14 | call to op | main.go:51:2:51:14 | call to op |
+| result 0 | main.go:53:2:53:22 | call to op2 | main.go:53:2:53:22 | call to op2 |
+| result 0 | main.go:53:14:53:21 | call to bump | main.go:53:14:53:21 | call to bump |
 | result 0 | main.go:54:10:54:15 | call to test | main.go:54:2:54:15 | ... := ...[0] |
 | result 0 | main.go:56:9:56:15 | call to test2 | main.go:56:2:56:15 | ... = ...[0] |
+| result 0 | tst.go:9:17:9:33 | call to new | tst.go:9:17:9:33 | call to new |
 | result 1 | main.go:54:10:54:15 | call to test | main.go:54:2:54:15 | ... := ...[1] |
 | result 1 | main.go:56:9:56:15 | call to test2 | main.go:56:2:56:15 | ... = ...[1] |
diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_isResult.expected b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_isResult.expected
@@ -0,0 +1,4 @@
+| main.go:51:2:51:14 | call to op | main.go:51:2:51:14 | call to op | result |
+| main.go:53:2:53:22 | call to op2 | main.go:53:2:53:22 | call to op2 | result |
+| main.go:53:14:53:21 | call to bump | main.go:53:14:53:21 | call to bump | result |
+| tst.go:9:17:9:33 | call to new | tst.go:9:17:9:33 | call to new | result |
diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_isResult.ql b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_isResult.ql
@@ -0,0 +1,5 @@
+import go
+
+from FunctionOutput outp, DataFlow::CallNode c, DataFlow::Node nodeTo
+where outp.isResult() and nodeTo = outp.getNode(c)
+select c, nodeTo, outp
diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_isResult_int.expected b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_isResult_int.expected
@@ -0,0 +1,12 @@
+| main.go:51:2:51:14 | call to op | main.go:51:2:51:14 | call to op | 0 | result |
+| main.go:51:2:51:14 | call to op | main.go:51:2:51:14 | call to op | 0 | result 0 |
+| main.go:53:2:53:22 | call to op2 | main.go:53:2:53:22 | call to op2 | 0 | result |
+| main.go:53:2:53:22 | call to op2 | main.go:53:2:53:22 | call to op2 | 0 | result 0 |
+| main.go:53:14:53:21 | call to bump | main.go:53:14:53:21 | call to bump | 0 | result |
+| main.go:53:14:53:21 | call to bump | main.go:53:14:53:21 | call to bump | 0 | result 0 |
+| main.go:54:10:54:15 | call to test | main.go:54:2:54:15 | ... := ...[0] | 0 | result 0 |
+| main.go:54:10:54:15 | call to test | main.go:54:2:54:15 | ... := ...[1] | 1 | result 1 |
+| main.go:56:9:56:15 | call to test2 | main.go:56:2:56:15 | ... = ...[0] | 0 | result 0 |
+| main.go:56:9:56:15 | call to test2 | main.go:56:2:56:15 | ... = ...[1] | 1 | result 1 |
+| tst.go:9:17:9:33 | call to new | tst.go:9:17:9:33 | call to new | 0 | result |
+| tst.go:9:17:9:33 | call to new | tst.go:9:17:9:33 | call to new | 0 | result 0 |
diff --git a/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_isResult_int.ql b/ql/test/library-tests/semmle/go/dataflow/FunctionInputsAndOutputs/FunctionOutput_isResult_int.ql
@@ -0,0 +1,5 @@
+import go
+
+from FunctionOutput outp, int i, DataFlow::CallNode c, DataFlow::Node nodeTo
+where outp.isResult(i) and nodeTo = outp.getNode(c)
+select c, nodeTo, i, outp
diff --git a/ql/test/library-tests/semmle/go/dataflow/Nodes/BinaryOperationNodes.expected b/ql/test/library-tests/semmle/go/dataflow/Nodes/BinaryOperationNodes.expected
@@ -1,3 +1,4 @@
 | main.go:7:14:7:24 | ...+... | + | main.go:7:14:7:14 | x | main.go:7:19:7:23 | ...+... |
 | main.go:7:19:7:23 | ...+... | + | main.go:7:19:7:19 | y | main.go:7:23:7:23 | z |
-| main.go:15:2:15:13 | ... += ... | + | main.go:15:2:15:6 | index expression | main.go:15:11:15:13 | "!" |
+| main.go:10:14:10:18 | ...+... | + | main.go:10:14:10:14 | x | main.go:10:18:10:18 | y |
+| main.go:17:2:17:13 | ... += ... | + | main.go:17:2:17:6 | index expression | main.go:17:11:17:13 | "!" |
diff --git a/ql/test/library-tests/semmle/go/dataflow/Nodes/CallNode.expected b/ql/test/library-tests/semmle/go/dataflow/Nodes/CallNode.expected
@@ -1,4 +1,6 @@
 | main.go:7:2:7:25 | call to Println |
 | main.go:8:5:8:7 | call to f |
-| main.go:12:8:12:24 | call to make |
-| main.go:14:2:14:26 | call to Println |
+| main.go:9:9:9:14 | call to test |
+| main.go:10:2:10:19 | call to Println |
+| main.go:14:8:14:24 | call to make |
+| main.go:16:2:16:26 | call to Println |
diff --git a/ql/test/library-tests/semmle/go/dataflow/Nodes/CallNode_getArgument.expected b/ql/test/library-tests/semmle/go/dataflow/Nodes/CallNode_getArgument.expected
@@ -1,5 +1,6 @@
 | main.go:7:2:7:25 | call to Println | 0 | main.go:7:14:7:24 | ...+... |
-| main.go:12:8:12:24 | call to make | 0 | main.go:12:23:12:23 | 1 |
-| main.go:14:2:14:26 | call to Println | 0 | main.go:14:14:14:15 | ss |
-| main.go:14:2:14:26 | call to Println | 1 | main.go:14:18:14:18 | 0 |
-| main.go:14:2:14:26 | call to Println | 2 | main.go:14:21:14:25 | index expression |
+| main.go:10:2:10:19 | call to Println | 0 | main.go:10:14:10:18 | ...+... |
+| main.go:14:8:14:24 | call to make | 0 | main.go:14:23:14:23 | 1 |
+| main.go:16:2:16:26 | call to Println | 0 | main.go:16:14:16:15 | ss |
+| main.go:16:2:16:26 | call to Println | 1 | main.go:16:18:16:18 | 0 |
+| main.go:16:2:16:26 | call to Println | 2 | main.go:16:21:16:25 | index expression |
diff --git a/ql/test/library-tests/semmle/go/dataflow/Nodes/CallNode_getResult.expected b/ql/test/library-tests/semmle/go/dataflow/Nodes/CallNode_getResult.expected
@@ -0,0 +1 @@
+| main.go:14:8:14:24 | call to make | main.go:14:8:14:24 | call to make |
diff --git a/ql/test/library-tests/semmle/go/dataflow/Nodes/CallNode_getResult.ql b/ql/test/library-tests/semmle/go/dataflow/Nodes/CallNode_getResult.ql
@@ -0,0 +1,5 @@
+import go
+
+from DataFlow::CallNode c, DataFlow::Node outp
+where outp = c.getResult()
+select c, outp
diff --git a/ql/test/library-tests/semmle/go/dataflow/Nodes/CallNode_getResult_int.expected b/ql/test/library-tests/semmle/go/dataflow/Nodes/CallNode_getResult_int.expected
@@ -0,0 +1,3 @@
+| main.go:9:9:9:14 | call to test | 0 | main.go:9:2:9:14 | ... = ...[0] |
+| main.go:9:9:9:14 | call to test | 1 | main.go:9:2:9:14 | ... = ...[1] |
+| main.go:14:8:14:24 | call to make | 0 | main.go:14:8:14:24 | call to make |
diff --git a/ql/test/library-tests/semmle/go/dataflow/Nodes/CallNode_getResult_int.ql b/ql/test/library-tests/semmle/go/dataflow/Nodes/CallNode_getResult_int.ql
@@ -0,0 +1,5 @@
+import go
+
+from DataFlow::CallNode c, int i, DataFlow::Node outp
+where outp = c.getResult(i)
+select c, i, outp
diff --git a/ql/test/library-tests/semmle/go/dataflow/Nodes/main.go b/ql/test/library-tests/semmle/go/dataflow/Nodes/main.go
@@ -6,6 +6,8 @@ func main() {
 	x, y, z := 1, 2, 3
 	fmt.Println(x + (y + z))
 	go f()
+	x, y = test()
+	fmt.Println(x + y)
 }
 
 func f() {
@@ -14,3 +16,7 @@ func f() {
 	fmt.Println(ss, 0, ss[0])
 	ss[0] += "!"
 }
+
+func test() (int, int) {
+	return 23, 42
+}

Original file line number	Diff line number	Diff line change
`@@ -72,8 +72,6 @@ private module Gin {`
`72`	`72`	`)`
`73`	`73`	`\|`
`74`	`74`	`this = call.getResult(0)`
`75`		`- or`
`76`		`- this = call.getResult()`
`77`	`75`	`)`
`78`	`76`	`or`
`79`	`77`	`// Field reads:`
`@@ -104,8 +102,6 @@ private module Gin {`
`104`	`102`	`call.getTarget().hasQualifiedName(packagePath, typeName, ["ByName", "Get"])`
`105`	`103`	`\|`
`106`	`104`	`this = call.getResult(0)`
`107`		`- or`
`108`		`- this = call.getResult()`
`109`	`105`	`)`
`110`	`106`	`)`
`111`	`107`	`}`
Original file line number	Diff line number	Diff line change
`@@ -62,7 +62,7 @@ module PathFilePath {`
`62`	`62`
`63`	`63`	`override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) {`
`64`	`64`	`inp.isParameter(_) and`
`65`		`- (outp.isResult() or outp.isResult(_))`
	`65`	`+ outp.isResult(_)`
`66`	`66`	`}`
`67`	`67`	`}`
`68`	`68`	`}`
`@@ -472,7 +472,7 @@ module Path {`
`472`	`472`
`473`	`473`	`override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) {`
`474`	`474`	`inp.isParameter(_) and`
`475`		`- (outp.isResult() or outp.isResult(_))`
	`475`	`+ outp.isResult(_)`
`476`	`476`	`}`
`477`	`477`	`}`
`478`	`478`	`}`
`@@ -684,8 +684,7 @@ module URL {`
`684`	`684`	`}`
`685`	`685`
`686`	`686`	`override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) {`
`687`		`- inp.isReceiver() and`
`688`		`- if getName() = "Password" then outp.isResult(0) else outp.isResult()`
	`687`	`+ inp.isReceiver() and outp.isResult(0)`
`689`	`688`	`}`
`690`	`689`	`}`
`691`	`690`
Original file line number	Diff line number	Diff line change
`@@ -75,7 +75,7 @@ module ReflectedXss {`
`75`	`75`	`pred.getStringValue().regexpMatch("^[^<].*")`
`76`	`76`	`or`
`77`	`77`	// json data cannot begin with `<`
`78`		`- pred = any(EncodingJson::MarshalFunction mf).getOutput().getExitNode(_)`
	`78`	`+ exists(EncodingJson::MarshalFunction mf \| pred = mf.getOutput().getNode(mf.getACall()))`
`79`	`79`	`)`
`80`	`80`	`)`
`81`	`81`	`}`