Data flow: Remove allowParameterReturnInSelf restriction

hvitved · hvitved · commit 1cd582c43bd4 · 2024-12-05T10:36:50.000+01:00
diff --git a/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlowPrivate.qll
@@ -296,15 +296,6 @@ predicate knownSinkModel(Node sink, string model) { none() }
 
 class DataFlowSecondLevelScope = Unit;
 
-/**
- * Holds if flow is allowed to pass from parameter `p` and back to itself as a
- * side-effect, resulting in a summary from `p` to itself.
- *
- * One example would be to allow flow like `p.foo = p.bar;`, which is disallowed
- * by default as a heuristic.
- */
-predicate allowParameterReturnInSelf(ParameterNode p) { none() }
-
 /** An approximated `Content`. */
 class ContentApprox = Unit;
 
diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll
@@ -1342,23 +1342,6 @@ predicate knownSourceModel(Node source, string model) { External::sourceNode(sou
 
 predicate knownSinkModel(Node sink, string model) { External::sinkNode(sink, _, model) }
 
-/**
- * Holds if flow is allowed to pass from parameter `p` and back to itself as a
- * side-effect, resulting in a summary from `p` to itself.
- *
- * One example would be to allow flow like `p.foo = p.bar;`, which is disallowed
- * by default as a heuristic.
- */
-predicate allowParameterReturnInSelf(ParameterNode p) {
-  p instanceof IndirectParameterNode
-  or
-  // models-as-data summarized flow
-  exists(DataFlowCallable c, ParameterPosition pos |
-    p.isParameterOf(c, pos) and
-    FlowSummaryImpl::Private::summaryAllowParameterReturnInSelf(c.asSummarizedCallable(), pos)
-  )
-}
-
 private predicate fieldHasApproxName(Field f, string s) {
   s = f.getName().charAt(0) and
   // Reads and writes of union fields are tracked using `UnionContent`.
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll
@@ -3084,23 +3084,6 @@ predicate knownSinkModel(Node sink, string model) { sinkNode(sink, _, model) }
 
 class DataFlowSecondLevelScope = Unit;
 
-/**
- * Holds if flow is allowed to pass from parameter `p` and back to itself as a
- * side-effect, resulting in a summary from `p` to itself.
- *
- * One example would be to allow flow like `p.foo = p.bar;`, which is disallowed
- * by default as a heuristic.
- */
-predicate allowParameterReturnInSelf(ParameterNode p) {
-  exists(DataFlowCallable c, ParameterPosition pos |
-    parameterNode(p, c, pos) and
-    FlowSummaryImpl::Private::summaryAllowParameterReturnInSelf(c.asSummarizedCallable(), pos)
-  )
-  or
-  VariableCapture::Flow::heuristicAllowInstanceParameterReturnInSelf(p.(DelegateSelfReferenceNode)
-        .getCallable())
-}
-
 /** An approximated `Content`. */
 class ContentApprox extends TContentApprox {
   /** Gets a textual representation of this approximated `Content`. */
diff --git a/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll b/go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll
@@ -438,20 +438,6 @@ predicate knownSinkModel(Node sink, string model) { sinkNode(sink, _, model) }
 
 class DataFlowSecondLevelScope = Unit;
 
-/**
- * Holds if flow is allowed to pass from parameter `p` and back to itself as a
- * side-effect, resulting in a summary from `p` to itself.
- *
- * One example would be to allow flow like `p.foo = p.bar;`, which is disallowed
- * by default as a heuristic.
- */
-predicate allowParameterReturnInSelf(ParameterNode p) {
-  exists(DataFlowCallable c, int pos |
-    p.isParameterOf(c, pos) and
-    FlowSummaryImpl::Private::summaryAllowParameterReturnInSelf(c.asSummarizedCallable(), pos)
-  )
-}
-
 /** An approximated `Content`. */
 class ContentApprox = Unit;
 
diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll
@@ -695,22 +695,6 @@ private Expr getRelatedExpr(Node n) {
 /** Gets the second-level scope containing the node `n`, if any. */
 DataFlowSecondLevelScope getSecondLevelScope(Node n) { result.getANode() = n }
 
-/**
- * Holds if flow is allowed to pass from parameter `p` and back to itself as a
- * side-effect, resulting in a summary from `p` to itself.
- *
- * One example would be to allow flow like `p.foo = p.bar;`, which is disallowed
- * by default as a heuristic.
- */
-predicate allowParameterReturnInSelf(ParameterNode p) {
-  exists(DataFlowCallable c, ParameterPosition pos |
-    parameterNode(p, c, pos) and
-    FlowSummaryImpl::Private::summaryAllowParameterReturnInSelf(c.asSummarizedCallable(), pos)
-  )
-  or
-  CaptureFlow::heuristicAllowInstanceParameterReturnInSelf(p.(InstanceParameterNode).getCallable())
-}
-
 /** An approximated `Content`. */
 class ContentApprox extends TContentApprox {
   /** Gets a textual representation of this approximated `Content`. */
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll b/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll
@@ -1096,25 +1096,6 @@ predicate knownSinkModel(Node sink, string model) {
 
 class DataFlowSecondLevelScope = Unit;
 
-/**
- * Holds if flow is allowed to pass from parameter `p` and back to itself as a
- * side-effect, resulting in a summary from `p` to itself.
- *
- * One example would be to allow flow like `p.foo = p.bar;`, which is disallowed
- * by default as a heuristic.
- */
-predicate allowParameterReturnInSelf(ParameterNode p) {
-  exists(DataFlowCallable c, ParameterPosition pos |
-    p.(ParameterNodeImpl).isParameterOf(c, pos) and
-    FlowSummaryImpl::Private::summaryAllowParameterReturnInSelf(c.asLibraryCallable(), pos)
-  )
-  or
-  exists(Function f |
-    VariableCapture::Flow::heuristicAllowInstanceParameterReturnInSelf(f) and
-    p = TSynthCapturedVariablesParameterNode(f)
-  )
-}
-
 /** An approximated `Content`. */
 class ContentApprox = Unit;
 
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
@@ -2107,23 +2107,6 @@ predicate knownSinkModel(Node sink, string model) {
 
 class DataFlowSecondLevelScope = Unit;
 
-/**
- * Holds if flow is allowed to pass from parameter `p` and back to itself as a
- * side-effect, resulting in a summary from `p` to itself.
- *
- * One example would be to allow flow like `p.foo = p.bar;`, which is disallowed
- * by default as a heuristic.
- */
-predicate allowParameterReturnInSelf(ParameterNodeImpl p) {
-  exists(DataFlowCallable c, ParameterPosition pos |
-    p.isParameterOf(c, pos) and
-    FlowSummaryImpl::Private::summaryAllowParameterReturnInSelf(c.asLibraryCallable(), pos)
-  )
-  or
-  VariableCapture::Flow::heuristicAllowInstanceParameterReturnInSelf(p.(LambdaSelfReferenceNode)
-        .getCallable())
-}
-
 /** An approximated `Content`. */
 class ContentApprox extends TContentApprox {
   string toString() {
diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll
@@ -990,20 +990,6 @@ module RustDataFlow implements InputSig<Location> {
    */
   predicate isUnreachableInCall(NodeRegion nr, DataFlowCall call) { none() }
 
-  /**
-   * Holds if flow is allowed to pass from parameter `p` and back to itself as a
-   * side-effect, resulting in a summary from `p` to itself.
-   *
-   * One example would be to allow flow like `p.foo = p.bar;`, which is disallowed
-   * by default as a heuristic.
-   */
-  predicate allowParameterReturnInSelf(ParameterNode p) {
-    exists(DataFlowCallable c, ParameterPosition pos |
-      p.isParameterOf(c, pos) and
-      FlowSummaryImpl::Private::summaryAllowParameterReturnInSelf(c.asLibraryCallable(), pos)
-    )
-  }
-
   /**
    * Holds if the value of `node2` is given by `node1`.
    *
diff --git a/shared/dataflow/codeql/dataflow/DataFlow.qll b/shared/dataflow/codeql/dataflow/DataFlow.qll
@@ -267,15 +267,6 @@ signature module InputSig<LocationSig Location> {
 
   default int accessPathLimit() { result = 5 }
 
-  /**
-   * Holds if flow is allowed to pass from parameter `p` and back to itself as a
-   * side-effect, resulting in a summary from `p` to itself.
-   *
-   * One example would be to allow flow like `p.foo = p.bar;`, which is disallowed
-   * by default as a heuristic.
-   */
-  predicate allowParameterReturnInSelf(ParameterNode p);
-
   /**
    * Holds if the value of `node2` is given by `node1`.
    *
diff --git a/shared/dataflow/codeql/dataflow/VariableCapture.qll b/shared/dataflow/codeql/dataflow/VariableCapture.qll
@@ -245,9 +245,6 @@ signature module OutputSig<LocationSig Location, InputSig<Location> I> {
   /** Holds if there is a read step from `node1` to `node2`. */
   predicate readStep(ClosureNode node1, I::CapturedVariable v, ClosureNode node2);
 
-  /** Holds if this-to-this summaries are expected for `c`. */
-  predicate heuristicAllowInstanceParameterReturnInSelf(I::Callable c);
-
   /** Holds if captured variable `v` is cleared at `node`. */
   predicate clearsContent(ClosureNode node, I::CapturedVariable v);
 }
@@ -579,18 +576,6 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
     exists(Callable c | ce.hasBody(c) and captureAccess(v, c))
   }
 
-  predicate heuristicAllowInstanceParameterReturnInSelf(Callable c) {
-    // If multiple variables are captured, then we should allow flow from one to
-    // another, which entails a this-to-this summary.
-    2 <= strictcount(CapturedVariable v | captureAccess(v, c))
-    or
-    // Constructors that capture a variable may assign it to a field, which also
-    // entails a this-to-this summary. If there are multiple constructors, then
-    // they might call each other, so if one constructor captures a variable we
-    // allow this-to-this summaries for all of them.
-    exists(ClosureExpr ce | ce.hasBody(c) and c.isConstructor() and hasConstructorCapture(ce, _))
-  }
-
   /** Holds if a constructor, if any, for the closure defined by `ce` captures `v`. */
   private predicate hasConstructorCapture(ClosureExpr ce, CapturedVariable v) {
     exists(Callable c | ce.hasBody(c) and c.isConstructor() and captureAccess(v, c))
diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll
@@ -498,21 +498,6 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
       )
     }
 
-    /**
-     * Holds if flow from `p` to a return node of kind `kind` is allowed.
-     *
-     * We don't expect a parameter to return stored in itself, unless
-     * explicitly allowed
-     */
-    bindingset[p, kind]
-    private predicate parameterFlowThroughAllowed(ParamNodeEx p, ReturnKindExt kind) {
-      exists(ParameterPosition pos | p.isParameterOf(_, pos) |
-        not kind.(ParamUpdateReturnKind).getPosition() = pos
-        or
-        allowParameterReturnInSelfEx(p)
-      )
-    }
-
     private module Stage1 implements StageSig {
       class Ap = Unit;
 
@@ -936,8 +921,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
           throughFlowNodeCand(p) and
           returnFlowCallableNodeCand(c, kind) and
           p.getEnclosingCallable() = c and
-          exists(ap) and
-          parameterFlowThroughAllowed(p, kind)
+          exists(ap)
         )
       }
 
@@ -2103,7 +2087,6 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
               TSummaryCtxSome(pragma[only_bind_into](p), _, _, pragma[only_bind_into](argAp), _) and
             not outBarrier(ret, state) and
             kind = ret.getKind() and
-            parameterFlowThroughAllowed(p, kind) and
             argApa = getApprox(argAp) and
             PrevStage::returnMayFlowThrough(ret, pragma[only_bind_into](argApa), apa, kind)
           )
@@ -2439,7 +2422,6 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
         ) {
           revFlow(pragma[only_bind_into](p), state, TReturnCtxMaybeFlowThrough(pos),
             apSome(returnAp), pragma[only_bind_into](ap)) and
-          parameterFlowThroughAllowed(p, pos.getKind()) and
           PrevStage::parameterMayFlowThrough(p, getApprox(ap))
         }
 
@@ -2525,8 +2507,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
         private predicate parameterFlowsThroughRev(
           ParamNodeEx p, Ap ap, ReturnPosition pos, Ap returnAp
         ) {
-          revFlow(p, _, TReturnCtxMaybeFlowThrough(pos), apSome(returnAp), ap) and
-          parameterFlowThroughAllowed(p, pos.getKind())
+          revFlow(p, _, TReturnCtxMaybeFlowThrough(pos), apSome(returnAp), ap)
         }
 
         pragma[nomagic]
diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll b/shared/dataflow/codeql/dataflow/internal/DataFlowImplCommon.qll
@@ -1600,7 +1600,8 @@ module MakeImplCommon<LocationSig Location, InputSig<Location> Lang> {
      * node `n`, in the same callable, using only value-preserving steps.
      */
     private predicate parameterValueFlowsToPreUpdate(ParamNode p, PostUpdateNode n) {
-      parameterValueFlow(p, n.getPreUpdateNode(), TReadStepTypesNone(), _, _)
+      parameterValueFlow(p, n.getPreUpdateNode(), TReadStepTypesNone(), _, _) and
+      p != n // needed for Golang
     }
 
     cached
@@ -1680,9 +1681,6 @@ module MakeImplCommon<LocationSig Location, InputSig<Location> Lang> {
       reverseStepThroughInputOutputAlias(node1, node2, model)
     }
 
-    cached
-    predicate allowParameterReturnInSelfEx(ParamNodeEx p) { allowParameterReturnInSelf(p.asNode()) }
-
     cached
     predicate paramMustFlow(ParamNode p, ArgNode arg) { localMustFlowStep+(p, arg) }
 
diff --git a/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll b/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll
@@ -1051,18 +1051,6 @@ module Make<
       )
     }
 
-    /**
-     * Holds if flow is allowed to pass from the parameter at position `pos` of `c`,
-     * to a return node, and back out to the parameter.
-     */
-    predicate summaryAllowParameterReturnInSelf(SummarizedCallable c, ParameterPosition ppos) {
-      exists(SummaryComponentStack inputContents, SummaryComponentStack outputContents |
-        summary(c, inputContents, outputContents, _, _) and
-        inputContents.bottom() = pragma[only_bind_into](TArgumentSummaryComponent(ppos)) and
-        outputContents.bottom() = pragma[only_bind_into](TArgumentSummaryComponent(ppos))
-      )
-    }
-
     signature module TypesInputSig {
       /** Gets the type of content `c`. */
       DataFlowType getContentType(ContentSet c);
diff --git a/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll b/swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll
@@ -1420,25 +1420,6 @@ predicate knownSinkModel(Node sink, string model) { sinkNode(sink, _, model) }
 
 class DataFlowSecondLevelScope = Unit;
 
-/**
- * Holds if flow is allowed to pass from parameter `p` and back to itself as a
- * side-effect, resulting in a summary from `p` to itself.
- *
- * One example would be to allow flow like `p.foo = p.bar;`, which is disallowed
- * by default as a heuristic.
- */
-predicate allowParameterReturnInSelf(ParameterNode p) {
-  exists(Callable c |
-    c = p.(ParameterNodeImpl).getEnclosingCallable().asSourceCallable() and
-    CaptureFlow::heuristicAllowInstanceParameterReturnInSelf(c)
-  )
-  or
-  exists(DataFlowCallable c, ParameterPosition pos |
-    p.(ParameterNodeImpl).isParameterOf(c, pos) and
-    FlowSummaryImpl::Private::summaryAllowParameterReturnInSelf(c.asSummarizedCallable(), pos)
-  )
-}
-
 /** An approximated `Content`. */
 class ContentApprox = Unit;
 
