Allow functions with multiple return values in Guards logic

Author: owen-mc

go/ql/lib/semmle/go/controlflow/Guards.qll

@@ -304,33 +304,80 @@ private module GuardsInput implements
   pragma[inline]
   predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos }
 
-  final private class FinalFunction = G::Function;
+  private newtype TNonOverridableMethod =
+    TMethod(G::Function function, int resultIndex) {
+      exists(function.getFuncDecl()) and
+      resultIndex in [-1 .. function.getNumResult() - 1]
+    }
 
   /**
-   * A declared function or concrete method.
+   * A result of a declared function or concrete method, or its normal completion.
    *
    * Calls are restricted separately to calls whose syntactic target is this
    * function or method, excluding interface dispatch.
    */
-  class NonOverridableMethod extends FinalFunction {
-    NonOverridableMethod() {
-      exists(super.getFuncDecl()) and
-      super.getNumResult() <= 1
+  class NonOverridableMethod extends TNonOverridableMethod {
+    G::Function getFunction() { this = TMethod(result, _) }
+
+    int getResultIndex() { this = TMethod(_, result) }
+
+    string toString() {
+      result = this.getFunction().toString() + " result " + this.getResultIndex().toString()
     }
 
-    Parameter getParameter(ParameterPosition ppos) { result = super.getParameter(ppos) }
+    int getNumParameter() { result = this.getFunction().getNumParameter() }
+
+    Parameter getParameter(ParameterPosition ppos) {
+      result = this.getFunction().getParameter(ppos)
+    }
+
+    /**
+     * Holds if every return maps one expression to each result position.
+     *
+     * Otherwise, the shared wrapper analysis would treat a partial set of
+     * return expressions as exhaustive.
+     */
+    private predicate hasOnlyPositionMappedReturns() {
+      forall(G::ReturnStmt ret | ret.getEnclosingFunction() = this.getFunction().getFuncDecl() |
+        ret.getNumExpr() = this.getFunction().getNumResult()
+      )
+    }
 
     /** Gets an expression being returned by this function. */
     Expr getAReturnExpr() {
       exists(G::ReturnStmt ret |
-        ret.getEnclosingFunction() = super.getFuncDecl() and
-        result = ret.getExpr()
+        this.getResultIndex() >= 0 and
+        this.hasOnlyPositionMappedReturns() and
+        ret.getEnclosingFunction() = this.getFunction().getFuncDecl() and
+        result = ret.getExpr(this.getResultIndex())
       )
     }
   }
 
-  private predicate nonOverridableCall(G::CallExpr call, NonOverridableMethod m) {
-    call.getTarget() = m
+  private predicate extractedCallResult(Expr use, G::CallExpr call, int resultIndex) {
+    exists(GoSsa::SsaDefinition def, IR::ExtractTupleElementInstruction extract |
+      use = def.getVariable().getAUse().(IR::EvalInstruction).getExpr() and
+      def.(GoSsa::SsaExplicitDefinition).getInstruction() = extract and
+      extract.extractsElement(IR::evalExprInstruction(call), resultIndex)
+    )
+  }
+
+  private predicate nonOverridableCall(
+    Expr resultExpr, G::CallExpr call, NonOverridableMethod method
+  ) {
+    call.getTarget() = method.getFunction() and
+    (
+      method.getResultIndex() = -1 and
+      resultExpr = call
+      or
+      method.getResultIndex() = 0 and
+      method.getFunction().getNumResult() = 1 and
+      resultExpr = call
+      or
+      method.getResultIndex() >= 0 and
+      method.getFunction().getNumResult() > 1 and
+      extractedCallResult(resultExpr, call, method.getResultIndex())
+    )
   }
 
   private predicate hasExplicitReceiverArgument(G::CallExpr call) {
@@ -352,28 +399,32 @@ private module GuardsInput implements
     )
   }
 
-  class NonOverridableMethodCall extends Expr instanceof G::CallExpr {
-    NonOverridableMethodCall() { nonOverridableCall(this, _) }
+  class NonOverridableMethodCall extends Expr {
+    NonOverridableMethodCall() { nonOverridableCall(this, _, _) }
+
+    private G::CallExpr getCall() { nonOverridableCall(this, result, _) }
 
-    NonOverridableMethod getMethod() { nonOverridableCall(this, result) }
+    NonOverridableMethod getMethod() { nonOverridableCall(this, _, result) }
 
     Expr getArgument(ArgumentPosition apos) {
-      (
-        not hasExplicitReceiverArgument(this) and
+      exists(G::CallExpr call | call = this.getCall() |
         (
-          apos = -1 and
-          result = getDirectReceiverArgument(this)
+          not hasExplicitReceiverArgument(call) and
+          (
+            apos = -1 and
+            result = getDirectReceiverArgument(call)
+            or
+            apos != -1 and
+            result = call.getArgument(apos)
+          )
           or
-          apos != -1 and
-          result = super.getArgument(apos)
+          hasExplicitReceiverArgument(call) and
+          result = call.getArgument(apos + 1)
+        ) and
+        not (
+          call.hasImplicitVarargs() and
+          apos = call.getTarget().getNumParameter() - 1
         )
-        or
-        hasExplicitReceiverArgument(this) and
-        result = super.getArgument(apos + 1)
-      ) and
-      not (
-        super.hasImplicitVarargs() and
-        apos = this.getTarget().getNumParameter() - 1
       )
     }
   }
@@ -413,8 +464,8 @@ private module LogicInput implements GuardsImpl::LogicInputSig {
 
   predicate implicitReturnDefinition(GuardsInput::NonOverridableMethod method, SsaDefinition def) {
     exists(IR::ReadResultInstruction read |
-      method.getNumResult() = 1 and
-      read.reads(method.getResult(0)) and
+      method.getResultIndex() >= 0 and
+      read.reads(method.getFunction().getResult(method.getResultIndex())) and
       def.getVariable().getAUse() = read
     )
   }

go/ql/test/library-tests/semmle/go/controlflow/Guards/Guards.expected

@@ -198,6 +198,10 @@ valueControlsResult
 | guards.go:309:13:309:20 | pointers | explicit variadic wrapper | not null |
 | guards.go:321:2:321:22 | call to ensureNotNil | after assertion | no exception |
 | guards.go:321:15:321:21 | pointer | after assertion | not null |
+| guards.go:333:2:333:32 | call to ensureNotNilWithResult | after single-result assertion | no exception |
+| guards.go:333:25:333:31 | pointer | after single-result assertion | not null |
+| guards.go:345:2:345:33 | call to ensureNotNilWithResults | after multi-result assertion | no exception |
+| guards.go:345:26:345:32 | pointer | after multi-result assertion | not null |
 ensuresEqResult
 | guards.go:14:7:14:16 | ...==... | 0 = value | true |
 | guards.go:14:7:14:16 | ...==... | value = 0 | true |
@@ -247,16 +251,20 @@ ensuresEqResult
 | guards.go:302:9:302:21 | ...!=... | values = nil | false |
 | guards.go:315:5:315:18 | ...==... | nil = pointer | true |
 | guards.go:315:5:315:18 | ...==... | pointer = nil | true |
-| guards.go:328:9:328:22 | ...!=... | nil = pointer | false |
-| guards.go:328:9:328:22 | ...!=... | pointer = nil | false |
-| guards.go:353:5:353:32 | ...==... | 0 = call to modelZeroGuard | true |
-| guards.go:353:5:353:32 | ...==... | call to modelZeroGuard = 0 | true |
-| guards.go:356:5:356:35 | ...!=... | 0 = call to modelNotZeroGuard | false |
-| guards.go:356:5:356:35 | ...!=... | call to modelNotZeroGuard = 0 | false |
-| guards.go:359:5:359:34 | ...==... | call to modelNullGuard = nil | true |
-| guards.go:359:5:359:34 | ...==... | nil = call to modelNullGuard | true |
-| guards.go:362:5:362:37 | ...!=... | call to modelNotNullGuard = nil | false |
-| guards.go:362:5:362:37 | ...!=... | nil = call to modelNotNullGuard | false |
+| guards.go:326:5:326:18 | ...==... | nil = pointer | true |
+| guards.go:326:5:326:18 | ...==... | pointer = nil | true |
+| guards.go:338:5:338:18 | ...==... | nil = pointer | true |
+| guards.go:338:5:338:18 | ...==... | pointer = nil | true |
+| guards.go:352:9:352:22 | ...!=... | nil = pointer | false |
+| guards.go:352:9:352:22 | ...!=... | pointer = nil | false |
+| guards.go:377:5:377:32 | ...==... | 0 = call to modelZeroGuard | true |
+| guards.go:377:5:377:32 | ...==... | call to modelZeroGuard = 0 | true |
+| guards.go:380:5:380:35 | ...!=... | 0 = call to modelNotZeroGuard | false |
+| guards.go:380:5:380:35 | ...!=... | call to modelNotZeroGuard = 0 | false |
+| guards.go:383:5:383:34 | ...==... | call to modelNullGuard = nil | true |
+| guards.go:383:5:383:34 | ...==... | nil = call to modelNullGuard | true |
+| guards.go:386:5:386:37 | ...!=... | call to modelNotNullGuard = nil | false |
+| guards.go:386:5:386:37 | ...!=... | nil = call to modelNotNullGuard | false |
 defaultCaseResult
 | guards.go:16:2:17:18 | case clause | positive | true |
 | guards.go:27:2:28:22 | case clause | tagged other | true |
@@ -265,11 +273,11 @@ defaultCaseResult
 | guards.go:155:2:156:22 | case clause | default only | true |
 | guards.go:164:2:165:32 | case clause | expressionless default | true |
 modelBarrierResult
-| guards.go:351:27:351:33 | pointer | model bool | model-bool |
-| guards.go:354:27:354:33 | pointer | model zero | model-zero |
-| guards.go:357:31:357:37 | pointer | model not zero | model-not-zero |
-| guards.go:360:27:360:33 | pointer | model null | model-null |
-| guards.go:363:31:363:37 | pointer | model not null | model-not-null |
-| guards.go:366:34:366:40 | pointer | model no exception | model-no-exception |
+| guards.go:375:27:375:33 | pointer | model bool | model-bool |
+| guards.go:378:27:378:33 | pointer | model zero | model-zero |
+| guards.go:381:31:381:37 | pointer | model not zero | model-not-zero |
+| guards.go:384:27:384:33 | pointer | model null | model-null |
+| guards.go:387:31:387:37 | pointer | model not null | model-not-null |
+| guards.go:390:34:390:40 | pointer | model no exception | model-no-exception |
 legacyConditionGuardCount
-| 88 |
+| 92 |

go/ql/test/library-tests/semmle/go/controlflow/Guards/guards.go

@@ -322,6 +322,30 @@ func exceptionGuard(pointer *int) {
 	sink("after assertion") // pointer is not null
 }
 
+func ensureNotNilWithResult(pointer *int) *int {
+	if pointer == nil {
+		panic("nil pointer")
+	}
+	return pointer
+}
+
+func exceptionGuardWithResult(pointer *int) {
+	ensureNotNilWithResult(pointer)
+	sink("after single-result assertion") // pointer is not null
+}
+
+func ensureNotNilWithResults(pointer *int) (*int, bool) {
+	if pointer == nil {
+		panic("nil pointer")
+	}
+	return pointer, true
+}
+
+func exceptionGuardWithResults(pointer *int) {
+	ensureNotNilWithResults(pointer)
+	sink("after multi-result assertion") // pointer is not null
+}
+
 func valueSink(string, any) {}
 
 func modelBoolGuard(pointer *int) bool {

go/ql/test/library-tests/semmle/go/dataflow/GuardingFunctions/test.go

@@ -334,6 +334,62 @@ func deeplyNestedConditionalRight(p string) bool {
 	return p[1] == 'b' && len(p)%2 == 1 && p[0] == 'a' && !isBad(p)
 }
 
+// Valid when the second result is nil
+func guardMultiError(p string) (string, error) {
+	if isBad(p) {
+		return "", errors.New("invalid")
+	}
+	return p, nil
+}
+
+// Valid when the first result is true; the second result is unrelated
+func guardMultiResultIsolation(p string) (bool, bool) {
+	return !isBad(p), true
+}
+
+// Valid when the named error result is nil
+func guardMultiNamed(p string) (value string, err error) {
+	if isBad(p) {
+		err = errors.New("invalid")
+		return
+	}
+	value = p
+	return
+}
+
+// Not a guard: the naked return can return false without validating p
+func mixedNamedResultGuard(p string, bypass bool) (invalid bool) {
+	if bypass {
+		return
+	}
+	return isBad(p)
+}
+
+func uncheckedMultiResult(p string) (string, error) {
+	return p, nil
+}
+
+// Not a guard: the tuple-forwarding return can return nil without validating p
+func mixedTupleReturnGuard(p string, bypass bool) (string, error) {
+	if bypass {
+		return uncheckedMultiResult(p)
+	}
+	if isBad(p) {
+		return "", errors.New("invalid")
+	}
+	return p, nil
+}
+
+type multiGuard struct{}
+
+// Validates p when the second result is nil
+func (multiGuard) validate(p string) (string, error) {
+	if isBad(p) {
+		return "", errors.New("invalid")
+	}
+	return p, nil
+}
+
 // Finally, actually test the functions -- try sinking a tainted value in the is-true/false
 // or is-nil/non-nil case for each candidate:
 
@@ -848,4 +904,81 @@ func test() {
 		}
 	}
 
+	{
+		s := source()
+		_, err := guardMultiError(s)
+		if err == nil {
+			sink(s)
+		} else {
+			sink(s) // $ hasValueFlow="s"
+		}
+	}
+
+	{
+		s := source()
+		valid, _ := guardMultiResultIsolation(s)
+		if valid {
+			sink(s)
+		} else {
+			sink(s) // $ hasValueFlow="s"
+		}
+	}
+
+	{
+		s := source()
+		_, unrelated := guardMultiResultIsolation(s)
+		if unrelated {
+			sink(s) // $ hasValueFlow="s"
+		} else {
+			sink(s) // $ hasValueFlow="s"
+		}
+	}
+
+	{
+		s := source()
+		_, err := guardMultiNamed(s)
+		if err == nil {
+			sink(s)
+		} else {
+			sink(s) // $ hasValueFlow="s"
+		}
+	}
+
+	{
+		s := source()
+		invalid := mixedNamedResultGuard(s, true)
+		if !invalid {
+			sink(s) // $ hasValueFlow="s"
+		}
+	}
+
+	{
+		s := source()
+		_, err := mixedTupleReturnGuard(s, true)
+		if err == nil {
+			sink(s) // $ hasValueFlow="s"
+		}
+	}
+
+	{
+		s := source()
+		_, err := guardMultiError(s)
+		copiedErr := err
+		if copiedErr == nil {
+			sink(s)
+		} else {
+			sink(s) // $ hasValueFlow="s"
+		}
+	}
+
+	{
+		s := source()
+		_, err := (multiGuard{}).validate(s)
+		if err == nil {
+			sink(s)
+		} else {
+			sink(s) // $ hasValueFlow="s"
+		}
+	}
+
 }