Merge pull request github#11732 from owen-mc/go/fix/model-data-flow-through-varargs

Go: Allow data flow through varargs parameters

Author: owen-mc

go/ql/lib/change-notes/2023-04-25-data-flow-varargs-parameters.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Fixed data flow through variadic function parameters. The arguments corresponding to a variadic parameter are no longer returned by `CallNode.getArgument(int i)` and `CallNode.getAnArgument()`, and hence aren't `ArgumentNode`s. They now have one result, which is an `ImplicitVarargsSlice` node. For example, a call `f(a, b, c)` to a function `f(T...)` is treated like `f([]T{a, b, c})`. The old behaviour is preserved by `CallNode.getSyntacticArgument(int i)` and `CallNode.getASyntacticArgument()`. `CallExpr.getArgument(int i)` and `CallExpr.getAnArgument()` are unchanged, and will still have three results in the example given.

go/ql/lib/semmle/go/Expr.qll

@@ -857,6 +857,24 @@ class CallExpr extends CallOrConversionExpr {
   /** Gets the number of argument expressions of this call. */
   int getNumArgument() { result = count(this.getAnArgument()) }
 
+  /** Holds if this call has implicit variadic arguments. */
+  predicate hasImplicitVarargs() {
+    this.getCalleeType().isVariadic() and
+    not this.hasEllipsis()
+  }
+
+  /**
+   * Gets an argument with an ellipsis after it which is passed to a varargs
+   * parameter, as in `f(x...)`.
+   *
+   * Note that if the varargs parameter is `...T` then the type of the argument
+   * must be assignable to the slice type `[]T`.
+   */
+  Expr getExplicitVarargsArgument() {
+    this.hasEllipsis() and
+    result = this.getArgument(this.getNumArgument() - 1)
+  }
+
   /**
    * Gets the name of the invoked function, method or variable if it can be
    * determined syntactically.
@@ -873,6 +891,15 @@ class CallExpr extends CallOrConversionExpr {
     )
   }
 
+  /**
+   * Gets the signature type of the invoked function.
+   *
+   * Note that it avoids calling `getTarget()` so that it works even when that
+   * predicate isn't defined, for example when calling a variable with function
+   * type.
+   */
+  SignatureType getCalleeType() { result = this.getCalleeExpr().getType() }
+
   /** Gets the declared target of this call. */
   Function getTarget() { this.getCalleeExpr() = result.getAReference() }
 

go/ql/lib/semmle/go/StringOps.qll

@@ -219,7 +219,7 @@ module StringOps {
        * replaced.
        */
       DataFlow::Node getAReplacedArgument() {
-        exists(int n | n % 2 = 0 and result = this.getArgument(n))
+        exists(int n | n % 2 = 0 and result = this.getSyntacticArgument(n))
       }
     }
 
@@ -304,11 +304,6 @@ module StringOps {
        * Gets the parameter index of the format string.
        */
       abstract int getFormatStringIndex();
-
-      /**
-       * Gets the parameter index of the first parameter to be formatted.
-       */
-      abstract int getFirstFormattedParameterIndex();
     }
 
     /**
@@ -336,7 +331,7 @@ module StringOps {
         formatDirective = this.getComponent(n) and
         formatDirective.charAt(0) = "%" and
         formatDirective.charAt(1) != "%" and
-        result = this.getArgument((n / 2) + f.getFirstFormattedParameterIndex())
+        result = this.getImplicitVarargsArgument(n / 2)
       }
     }
   }

go/ql/lib/semmle/go/dataflow/FunctionInputsAndOutputs.qll

@@ -74,7 +74,9 @@ private class ParameterInput extends FunctionInput, TInParameter {
 
   override predicate isParameter(int i) { i = index }
 
-  override DataFlow::Node getEntryNode(DataFlow::CallNode c) { result = c.getArgument(index) }
+  override DataFlow::Node getEntryNode(DataFlow::CallNode c) {
+    result = c.getSyntacticArgument(index)
+  }
 
   override DataFlow::Node getExitNode(FuncDef f) {
     result = DataFlow::parameterNode(f.getParameter(index))
@@ -280,7 +282,7 @@ private class OutReceiver extends FunctionOutput, TOutReceiver {
 /**
  * A parameter of a function, viewed as an output.
  *
- * Note that slices passed to varargs parameters using `...` are not included, since in this
+ * Note that slices passed to variadic parameters using `...` are not included, since in this
  * case it is ambiguous whether the output should be the slice itself or one of its elements.
  */
 private class OutParameter extends FunctionOutput, TOutParameter {
@@ -298,9 +300,12 @@ private class OutParameter extends FunctionOutput, TOutParameter {
 
   override DataFlow::Node getExitNode(DataFlow::CallNode c) {
     exists(DataFlow::Node arg |
-      arg = getArgument(c, index) and
-      // exclude slices passed to varargs parameters using `...` calls
+      arg = c.getSyntacticArgument(index) and
+      // exclude slices followed by `...` passed to variadic parameters
       not (c.hasEllipsis() and index = c.getNumArgument() - 1)
+      or
+      arg = c.(DataFlow::MethodCallNode).getReceiver() and
+      index = -1
     |
       result.(DataFlow::PostUpdateNode).getPreUpdateNode() = arg
     )

go/ql/lib/semmle/go/dataflow/internal/ContainerFlow.qll

@@ -10,7 +10,7 @@ private import semmle.go.dataflow.ExternalFlow
  * Holds if the step from `node1` to `node2` stores a value in an array, a
  * slice, a collection or a map. Thus, `node2` references an object with a
  * content `c` that contains the value of `node1`. This covers array
- * assignments and initializers as well as implicit array creations for
+ * assignments and initializers as well as implicit slice creations for
  * varargs.
  */
 predicate containerStoreStep(Node node1, Node node2, Content c) {
@@ -20,7 +20,11 @@ predicate containerStoreStep(Node node1, Node node2, Content c) {
       node2.getType() instanceof ArrayType or
       node2.getType() instanceof SliceType
     ) and
-    exists(Write w | w.writesElement(node2, _, node1))
+    (
+      exists(Write w | w.writesElement(node2, _, node1))
+      or
+      node1 = node2.(ImplicitVarargsSlice).getCallNode().getAnImplicitVarargsArgument()
+    )
   )
   or
   c instanceof CollectionContent and

go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll

@@ -10,6 +10,7 @@ private newtype TNode =
   MkInstructionNode(IR::Instruction insn) or
   MkSsaNode(SsaDefinition ssa) or
   MkGlobalFunctionNode(Function f) or
+  MkImplicitVarargsSlice(CallExpr c) { c.hasImplicitVarargs() } or
   MkSummarizedParameterNode(SummarizedCallable c, int i) {
     FlowSummaryImpl::Private::summaryParameterNodeRange(c, i)
   } or
@@ -426,6 +427,41 @@ module Public {
     override ResultNode getAResult() { result.getRoot() = this.getExpr() }
   }
 
+  /**
+   * An implicit varargs slice creation expression.
+   *
+   * A variadic function like `f(t1 T1, ..., Tm tm, A... x)` actually sees the
+   * varargs parameter as a slice `[]A`. A call `f(t1, ..., tm, x1, ..., xn)`
+   * desugars to `f(t1, ..., tm, []A{x1, ..., xn})`, and this node corresponds
+   * to this implicit slice creation.
+   */
+  class ImplicitVarargsSlice extends Node, MkImplicitVarargsSlice {
+    CallNode call;
+
+    ImplicitVarargsSlice() { this = MkImplicitVarargsSlice(call.getCall()) }
+
+    override ControlFlow::Root getRoot() { result = call.getRoot() }
+
+    /** Gets the call containing this varargs slice creation argument. */
+    CallNode getCallNode() { result = call }
+
+    override Type getType() {
+      exists(Function f | f = call.getTarget() |
+        result = f.getParameterType(f.getNumParameter() - 1)
+      )
+    }
+
+    override string getNodeKind() { result = "implicit varargs slice" }
+
+    override string toString() { result = "[]type{args}" }
+
+    override predicate hasLocationInfo(
+      string filepath, int startline, int startcolumn, int endline, int endcolumn
+    ) {
+      call.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    }
+  }
+
   /**
    * Gets a possible target of call `cn`.class
    *
@@ -498,16 +534,11 @@ module Public {
     CallExpr getCall() { result = this.getExpr() }
 
     /**
-     * Gets the data flow node corresponding to the `i`th argument of this call.
-     *
-     * Note that the first argument in calls to the built-in function `make` is a type, which is
-     * not a data-flow node. It is skipped for the purposes of this predicate, so the (syntactically)
-     * second argument becomes the first argument in terms of data flow.
-     *
-     * For calls of the form `f(g())` where `g` has multiple results, the arguments of the call to
-     * `i` are the (implicit) element extraction nodes for the call to `g`.
+     * Gets the `i`th argument of this call, where tuple extraction has been
+     * done but arguments corresponding to a variadic parameter are still
+     * considered separate.
      */
-    Node getArgument(int i) {
+    Node getSyntacticArgument(int i) {
       if expr.getArgument(0).getType() instanceof TupleType
       then result = DataFlow::extractTupleElement(DataFlow::exprNode(expr.getArgument(0)), i)
       else
@@ -519,12 +550,60 @@ module Public {
           )
     }
 
+    /**
+     * Gets a data flow node corresponding to an argument of this call, where
+     * tuple extraction has been done but arguments corresponding to a variadic
+     * parameter are still considered separate.
+     */
+    Node getASyntacticArgument() { result = this.getSyntacticArgument(_) }
+
+    /**
+     * Gets the data flow node corresponding to the `i`th argument of this call.
+     *
+     * Note that the first argument in calls to the built-in function `make` is a type, which is
+     * not a data-flow node. It is skipped for the purposes of this predicate, so the (syntactically)
+     * second argument becomes the first argument in terms of data flow.
+     *
+     * For calls of the form `f(g())` where `g` has multiple results, the arguments of the call to
+     * `i` are the (implicit) element extraction nodes for the call to `g`.
+     *
+     * Returns a single `Node` corresponding to a variadic parameter. If there is no corresponding
+     * argument with an ellipsis (`...`), then it is a `ImplicitVarargsSlice`. This is in contrast
+     * to `getArgument` on `CallExpr`, which gets the syntactic arguments. Use
+     * `getSyntacticArgument` to get that behavior.
+     */
+    Node getArgument(int i) {
+      result = this.getSyntacticArgument(i) and
+      not (expr.hasImplicitVarargs() and i >= expr.getCalleeType().getNumParameter() - 1)
+      or
+      i = expr.getCalleeType().getNumParameter() - 1 and
+      result.(ImplicitVarargsSlice).getCallNode() = this
+    }
+
     /** Gets the data flow node corresponding to an argument of this call. */
     Node getAnArgument() { result = this.getArgument(_) }
 
     /** Gets the number of arguments of this call, if it can be determined. */
     int getNumArgument() { result = count(this.getAnArgument()) }
 
+    /**
+     * Gets the 'i'th argument without an ellipsis after it which is passed to
+     * the varargs parameter of the target of this call (if there is one).
+     */
+    Node getImplicitVarargsArgument(int i) {
+      i >= 0 and
+      expr.hasImplicitVarargs() and
+      exists(int lastParamIndex | lastParamIndex = expr.getCalleeType().getNumParameter() - 1 |
+        result = this.getSyntacticArgument(lastParamIndex + i)
+      )
+    }
+
+    /**
+     * Gets an argument without an ellipsis after it which is passed to
+     * the varargs parameter of the target of this call (if there is one).
+     */
+    Node getAnImplicitVarargsArgument() { result = this.getImplicitVarargsArgument(_) }
+
     /** Gets a function passed as the `i`th argument of this call. */
     FunctionNode getCallback(int i) { result.getASuccessor*() = this.getArgument(i) }
 
@@ -696,7 +775,11 @@ module Public {
         or
         preupd = getAWrittenNode()
         or
-        preupd instanceof ArgumentNode and
+        (
+          preupd instanceof ArgumentNode and not preupd instanceof ImplicitVarargsSlice
+          or
+          preupd = any(CallNode c).getAnImplicitVarargsArgument()
+        ) and
         mutableType(preupd.getType())
       ) and
       (

go/ql/lib/semmle/go/frameworks/Beego.qll

@@ -253,21 +253,21 @@ module Beego {
       this.getTarget().hasQualifiedName([packagePath(), logsPackagePath()], getALogFunctionName())
     }
 
-    override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
+    override DataFlow::Node getAMessageComponent() { result = this.getASyntacticArgument() }
   }
 
   private class BeegoLoggerMethods extends LoggerCall::Range, DataFlow::MethodCallNode {
     BeegoLoggerMethods() {
       this.getTarget().hasQualifiedName(logsPackagePath(), "BeeLogger", getALogFunctionName())
     }
 
-    override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
+    override DataFlow::Node getAMessageComponent() { result = this.getASyntacticArgument() }
   }
 
   private class UtilLoggers extends LoggerCall::Range, DataFlow::CallNode {
     UtilLoggers() { this.getTarget().hasQualifiedName(utilsPackagePath(), "Display") }
 
-    override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
+    override DataFlow::Node getAMessageComponent() { result = this.getASyntacticArgument() }
   }
 
   private class HtmlQuoteSanitizer extends SharedXss::Sanitizer {

go/ql/lib/semmle/go/frameworks/BeegoOrm.qll

@@ -33,7 +33,7 @@ module BeegoOrm {
     // Note this class doesn't do any escaping, unlike the true ORM part of the package
     QueryBuilderSink() {
       exists(Method impl | impl.implements(packagePath(), "QueryBuilder", _) |
-        this = impl.getACall().getAnArgument()
+        this = impl.getACall().getASyntacticArgument()
       ) and
       this.getType().getUnderlyingType() instanceof StringType
     }

go/ql/lib/semmle/go/frameworks/ElazarlGoproxy.qll

@@ -90,7 +90,7 @@ module ElazarlGoproxy {
         onreqcall.getTarget().hasQualifiedName(packagePath(), "ProxyHttpServer", "OnRequest")
       |
         handlerReg.getReceiver() = onreqcall.getASuccessor*() and
-        check = onreqcall.getArgument(0)
+        check = onreqcall.getSyntacticArgument(0)
       )
     }
   }
@@ -112,13 +112,11 @@ module ElazarlGoproxy {
     ProxyLogFunction() { this.hasQualifiedName(packagePath(), "ProxyCtx", ["Logf", "Warnf"]) }
 
     override int getFormatStringIndex() { result = 0 }
-
-    override int getFirstFormattedParameterIndex() { result = 1 }
   }
 
   private class ProxyLog extends LoggerCall::Range, DataFlow::MethodCallNode {
     ProxyLog() { this.getTarget() instanceof ProxyLogFunction }
 
-    override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
+    override DataFlow::Node getAMessageComponent() { result = this.getASyntacticArgument() }
   }
 }

go/ql/lib/semmle/go/frameworks/Email.qll

@@ -56,13 +56,13 @@ module EmailData {
       // func NewV3MailInit(from *Email, subject string, to *Email, content ...*Content) *SGMailV3
       exists(Function newv3MailInit |
         newv3MailInit.hasQualifiedName(sendgridMail(), "NewV3MailInit") and
-        this = newv3MailInit.getACall().getArgument(any(int i | i = 1 or i >= 3))
+        this = newv3MailInit.getACall().getSyntacticArgument(any(int i | i = 1 or i >= 3))
       )
       or
       // func (s *SGMailV3) AddContent(c ...*Content) *SGMailV3
       exists(Method addContent |
         addContent.hasQualifiedName(sendgridMail(), "SGMailV3", "AddContent") and
-        this = addContent.getACall().getAnArgument()
+        this = addContent.getACall().getASyntacticArgument()
       )
     }
   }