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
+      (
+        function.getNumResult() = 0 and resultIndex = -1
+        or
+        resultIndex in [0 .. function.getNumResult() - 1]
+      )
+    }
 
   /**
-   * A declared function or concrete method.
+   * A result of a declared function or concrete method, or its normal
+   * completion when it has no results.
    *
    * 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.getFunction().getNumResult() <= 1 and
+      resultExpr = call
+      or
+      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, this.getMethod())
+          not hasExplicitReceiverArgument(call) and
+          (
+            apos = -1 and
+            result = getDirectReceiverArgument(call, this.getMethod())
+            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 = this.getMethod().getNumParameter() - 1
         )
-        or
-        hasExplicitReceiverArgument(this) and
-        result = super.getArgument(apos + 1)
-      ) and
-      not (
-        super.hasImplicitVarargs() and
-        apos = this.getMethod().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/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"
+		}
+	}
+
 }