Revert "Revert "[dataflowengineoss] Refactor isCallRetVal (joernio#4673…

…)"" This reverts commit af53280.
Privado-Inc · Dec 24, 2024 · a418b6f · a418b6f
1 parent f9f7b51
commit a418b6f
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 16 deletions.
diff --git a/...ngineoss/src/main/scala/io/joern/dataflowengineoss/passes/reachingdef/EdgeValidator.scala b/...ngineoss/src/main/scala/io/joern/dataflowengineoss/passes/reachingdef/EdgeValidator.scala
@@ -1,10 +1,17 @@
 package io.joern.dataflowengineoss.passes.reachingdef
 
-import io.joern.dataflowengineoss.language._
-import io.joern.dataflowengineoss.queryengine.Engine.isOutputArgOfInternalMethod
-import io.joern.dataflowengineoss.semanticsloader.{FlowMapping, ParameterNode, PassThroughMapping, Semantics}
+import io.joern.dataflowengineoss.language.*
+import io.joern.dataflowengineoss.queryengine.Engine.{isOutputArgOfInternalMethod, semanticsForCall}
+import io.joern.dataflowengineoss.semanticsloader.{
+  FlowMapping,
+  FlowPath,
+  FlowSemantic,
+  ParameterNode,
+  PassThroughMapping,
+  Semantics
+}
 import io.shiftleft.codepropertygraph.generated.nodes.{Call, CfgNode, Expression, StoredNode}
-import io.shiftleft.semanticcpg.language.*
+import io.shiftleft.semanticcpg.language._
 
 object EdgeValidator {
 
@@ -15,6 +22,11 @@ object EdgeValidator {
       case (childNode: Expression, parentNode)
           if isCallRetval(parentNode) || !isValidEdgeToExpression(parentNode, childNode) =>
         false
+      case (childNode: Call, parentNode: Expression)
+          if isCallRetval(childNode) && childNode.argument.contains(parentNode) =>
+        // e.g. foo(x), but there are semantics for `foo` that don't taint its return value
+        // in which case we don't want `x` to taint `foo(x)`.
+        false
       case (childNode: Expression, parentNode: Expression)
           if parentNode.isArgToSameCallWith(childNode) && childNode.isDefined && parentNode.isUsed =>
         parentNode.hasDefinedFlowTo(childNode)
@@ -34,17 +46,22 @@ object EdgeValidator {
         curNode.isUsed
     }
 
+  /** Is it a CALL for which semantics exist but don't taint its return value?
+    */
   private def isCallRetval(parentNode: StoredNode)(implicit semantics: Semantics): Boolean =
     parentNode match {
-      case call: Call =>
-        val sem = semantics.forMethod(call.methodFullName)
-        sem.isDefined && !sem.get.mappings.exists {
-          case FlowMapping(_, ParameterNode(dst, _)) => dst == -1
-          case PassThroughMapping                    => true
-          case _                                     => false
-        }
-      case _ =>
-        false
+      case call: Call => semanticsForCall(call).exists(!explicitlyFlowsToReturnValue(_))
+      case _          => false
     }
 
+  private def explicitlyFlowsToReturnValue(flowSemantic: FlowSemantic): Boolean =
+    flowSemantic.mappings.exists(explicitlyFlowsToReturnValue)
+
+  private def explicitlyFlowsToReturnValue(flowPath: FlowPath): Boolean = flowPath match {
+    // Some frontends (e.g. python) denote named arguments using `-1` as the argument index. As such
+    // `-1` denotes the return value only if there's no argument name.
+    case FlowMapping(_, ParameterNode(-1, None)) => true
+    case PassThroughMapping                      => true
+    case _                                       => false
+  }
 }
diff --git a/joern-cli/frontends/pysrc2cpg/src/test/scala/io/joern/pysrc2cpg/dataflow/DataFlowTests.scala b/joern-cli/frontends/pysrc2cpg/src/test/scala/io/joern/pysrc2cpg/dataflow/DataFlowTests.scala
@@ -681,7 +681,7 @@ class DataFlowTests extends PySrc2CpgFixture(withOssDataflow = true) {
         |x = {'x': 10}
         |print(1, x)
         |""".stripMargin)
-      .withExtraFlows(List(FlowSemantic(".*print", List(PassThroughMapping), true)))
+      .withSemantics(DefaultSemantics().plus(List(FlowSemantic(".*print", List(PassThroughMapping), true))))
 
     def source       = cpg.literal
     def sink         = cpg.call("print").argument.argumentIndex(2)
@@ -1243,8 +1243,8 @@ class RegexDefinedFlowsDataFlowTests
         |""".stripMargin)
     "be found" in {
       val src = cpg.identifier("Foo").l
-      val snk = cpg.call("print").l
-      snk.reachableByFlows(src).size shouldBe 2
+      val snk = cpg.call("print").argument(1).l
+      snk.reachableByFlows(src).size shouldBe 3
     }
   }
   "Import statement with method ref sample four" in {