More fixes for OSS data flow (#1083)

Author: fabsx00

dataflowengineoss/src/main/scala/io/shiftleft/dataflowengineoss/language/nodemethods/ExpressionMethods.scala

@@ -22,7 +22,12 @@ class ExpressionMethods[NodeType <: nodes.Expression](val node: NodeType) extend
     * */
   def isDefined(implicit semantics: Semantics): Boolean = {
     val s = semanticsForCallByArg
-    s.isEmpty || s.exists(_.mappings.exists { case (_, dstIndex) => dstIndex == node.order })
+    s.isEmpty || s.exists { semantic =>
+      semantic.mappings.exists {
+        case (_, dstIndex) =>
+          dstIndex == node.order
+      }
+    }
   }
 
   /**

dataflowengineoss/src/main/scala/io/shiftleft/dataflowengineoss/passes/reachingdef/ReachingDefPass.scala

@@ -4,8 +4,7 @@ import io.shiftleft.codepropertygraph.Cpg
 import io.shiftleft.codepropertygraph.generated.{nodes, _}
 import io.shiftleft.passes.{DiffGraph, ParallelCpgPass}
 import io.shiftleft.semanticcpg.language._
-
-import scala.collection.mutable
+import overflowdb.traversal.Traversal
 
 /**
   * A pass that calculates reaching definitions ("data dependencies").
@@ -29,14 +28,12 @@ class ReachingDefPass(cpg: Cpg) extends ParallelCpgPass[nodes.Method](cpg) {
   private def addReachingDefEdges(method: nodes.Method, solution: Solution[Set[Definition]]): DiffGraph.Builder = {
 
     val dstGraph = DiffGraph.newBuilder
-    val nodesWithOutgoingEdges: mutable.Set[nodes.StoredNode] = mutable.Set()
 
     def addEdge(fromNode: nodes.StoredNode, toNode: nodes.StoredNode, variable: String = ""): Unit = {
       val properties = List((EdgeKeyNames.VARIABLE, variable))
       if (fromNode.isInstanceOf[nodes.Unknown] || toNode
             .isInstanceOf[nodes.Unknown])
         return
-      nodesWithOutgoingEdges += fromNode
       dstGraph.addEdgeInOriginal(fromNode, toNode, EdgeTypes.REACHING_DEF, properties)
     }
 
@@ -61,9 +58,9 @@ class ReachingDefPass(cpg: Cpg) extends ParallelCpgPass[nodes.Method](cpg) {
           // For all calls, assume that input arguments
           // taint corresponding output arguments
           // and the return value
-          usageAnalyzer.uses(call).foreach { use =>
-            gen(call).foreach { g =>
-              if (use != g.node) {
+          usageAnalyzer.uses(node).foreach { use =>
+            gen(node).foreach { g =>
+              if (use != g.node && nodeMayBeSource(use)) {
                 addEdge(use, g.node, nodeToEdgeLabel(use))
               }
             }
@@ -82,39 +79,33 @@ class ReachingDefPass(cpg: Cpg) extends ParallelCpgPass[nodes.Method](cpg) {
           }
           addEdge(ret, method.methodReturn, "<RET>")
 
+        case exitNode: nodes.MethodReturn =>
+          in(exitNode).foreach { i =>
+            addEdge(i.node, exitNode, nodeToEdgeLabel(i.node))
+          }
         case _ =>
       }
     }
 
     // Add edges from the entry node
     allNodes
-      .filterNot(
-        x =>
-          x.isInstanceOf[nodes.Method] || x
-            .isInstanceOf[nodes.ControlStructure] || x.isInstanceOf[nodes.FieldIdentifier] || x
-            .isInstanceOf[nodes.JumpTarget] || x.isInstanceOf[nodes.MethodReturn] || x.isInstanceOf[nodes.Block])
+      .filter(nodeMayBeSource)
       .foreach { node =>
         if (usageAnalyzer.usedIncomingDefs(node).isEmpty) {
           addEdge(method, node)
         }
       }
-
-    // Add edges to exit node
-    val exitNode = method.methodReturn
-    (allNodes.toSet -- nodesWithOutgoingEdges).toList
-      .filter(_ != exitNode)
-      .filter(_ != method)
-      .filterNot(
-        x =>
-          x.isInstanceOf[nodes.Method] || x
-            .isInstanceOf[nodes.ControlStructure] || x.isInstanceOf[nodes.FieldIdentifier] || x
-            .isInstanceOf[nodes.JumpTarget] || x.isInstanceOf[nodes.MethodReturn])
-      .foreach { from =>
-        addEdge(from, exitNode)
-      }
     dstGraph
   }
 
+  private def nodeMayBeSource(x: nodes.StoredNode): Boolean = {
+    !(
+      x.isInstanceOf[nodes.Method] || x
+        .isInstanceOf[nodes.ControlStructure] || x.isInstanceOf[nodes.FieldIdentifier] || x
+        .isInstanceOf[nodes.JumpTarget] || x.isInstanceOf[nodes.MethodReturn] || x.isInstanceOf[nodes.Block]
+    )
+  }
+
   private def nodeToEdgeLabel(node: nodes.StoredNode): String = {
     node match {
       case n: nodes.CfgNode           => n.code

dataflowengineoss/src/main/scala/io/shiftleft/dataflowengineoss/passes/reachingdef/ReachingDefProblem.scala

@@ -27,6 +27,7 @@ object ReachingDefProblem {
     val init = new ReachingDefInit(transfer.gen)
     def meet: (Set[Definition], Set[Definition]) => Set[Definition] =
       (x: Set[Definition], y: Set[Definition]) => { x.union(y) }
+
     new DataFlowProblem[Set[Definition]](flowGraph, transfer, meet, init, true, Set[Definition]())
   }
 

dataflowengineoss/src/main/scala/io/shiftleft/dataflowengineoss/passes/reachingdef/UsageAnalyzer.scala

@@ -40,7 +40,6 @@ class UsageAnalyzer(in: Map[nodes.StoredNode, Set[Definition]]) {
 
   def uses(node: nodes.StoredNode): Set[nodes.StoredNode] = {
     val n = node match {
-      // case methodRet: nodes.MethodReturn => methodRet.start.method.cfgNode.toSet
       case ret: nodes.Return =>
         ret.astChildren.toSet
       case call: nodes.Call =>

dataflowengineoss/src/main/scala/io/shiftleft/dataflowengineoss/queryengine/Engine.scala

@@ -1,7 +1,8 @@
 package io.shiftleft.dataflowengineoss.queryengine
 
-import java.util.concurrent.{Callable, ExecutorCompletionService, ExecutorService, Executors}
+import io.shiftleft.codepropertygraph.generated.nodes.Call
 
+import java.util.concurrent.{Callable, ExecutorCompletionService, ExecutorService, Executors}
 import io.shiftleft.codepropertygraph.generated.{EdgeKeys, EdgeTypes, nodes}
 import io.shiftleft.dataflowengineoss.semanticsloader.{FlowSemantic, Semantics}
 import io.shiftleft.semanticcpg.language._
@@ -132,14 +133,22 @@ object Engine {
       implicit semantics: Semantics): Vector[PathElement] = {
     curNode match {
       case argument: nodes.Expression =>
-        val (arguments, nonArguments) = ddgInE(curNode, path).partition(_.outNode().isInstanceOf[nodes.Expression])
+        val (arguments, nonArguments) = ddgInE(curNode, path)
+          .filter { edge =>
+            !isCallRetvalThatShouldNotPropagate(edge.outNode().asInstanceOf[nodes.StoredNode])
+          }
+          .partition(_.outNode().isInstanceOf[nodes.Expression])
         val elemsForArguments = arguments.flatMap { e =>
           elemForArgument(e, argument)
         }
         val elems = elemsForArguments ++ nonArguments.map(edgeToPathElement)
         elems
       case _ =>
-        ddgInE(curNode, path).map(edgeToPathElement)
+        ddgInE(curNode, path)
+          .filter { edge =>
+            !isCallRetvalThatShouldNotPropagate(edge.outNode().asInstanceOf[nodes.StoredNode])
+          }
+          .map(edgeToPathElement)
     }
   }
 
@@ -158,6 +167,16 @@ object Engine {
       .toVector
   }
 
+  def isCallRetvalThatShouldNotPropagate(parentNode: nodes.StoredNode)(implicit semantics: Semantics): Boolean = {
+    parentNode match {
+      case call: Call =>
+        val sem = semantics.forMethod(call.methodFullName)
+        (sem.isDefined && !(sem.get.mappings.map(_._2).contains(-1)))
+      case _ =>
+        false
+    }
+  }
+
   /**
     * For a given `(parentNode, curNode)` pair, determine whether to expand into
     * `parentNode`. If so, return a corresponding path element or None if

dataflowengineoss/src/test/resources/default.semantics

@@ -30,3 +30,4 @@
 "<operator>.addition" 1->-1 2->-1
 "<operator>.conditional" 2->-1 3->-1
 "woo" 1->-1
+"free" 1->1

fuzzyc2cpg-tests/src/test/scala/io/shiftleft/fuzzyc2cpg/dotgenerator/DotDdgGeneratorTests.scala

@@ -19,11 +19,11 @@ class DotDdgGeneratorTests extends DataFlowCodeToCpgSuite {
       |""".stripMargin
 
   "A DdgDotGenerator" should {
-    "create a dot graph with 30 edges" in {
+    "create a dot graph with 31 edges" in {
       implicit val s = semantics
       val lines = cpg.method.name("foo").dotDdg.l.head.split("\n")
       lines.head.startsWith("digraph foo") shouldBe true
-      lines.count(x => x.contains("->")) shouldBe 30
+      lines.count(x => x.contains("->")) shouldBe 31
       lines.last.startsWith("}") shouldBe true
     }
   }
@@ -42,7 +42,7 @@ class DotDdgGeneratorTests2 extends DataFlowCodeToCpgSuite {
   "create correct dot graph" in {
     implicit val s = semantics
     val lines = cpg.method.name("foo").dotDdg.l.head.split("\n")
-    lines.count(x => x.contains("->") && x.contains("\"x\"")) shouldBe 2
+    lines.count(x => x.contains("->") && x.contains("\"x\"")) shouldBe 3
   }
 
 }

fuzzyc2cpg-tests/src/test/scala/io/shiftleft/fuzzyc2cpg/querying/CDataFlowTests.scala

@@ -26,14 +26,10 @@ class CDataFlowTests1 extends DataFlowCodeToCpgSuite {
 
   "Test 1: flow from function call read to multiple versions of the same variable" in {
 
-    def source = cpg.identifier.name("sz")
-
-    def sink = cpg.call.name("read")
-
+    val source = cpg.identifier.name("sz").l
+    val sink = cpg.call.name("read").l
     def flows = sink.reachableByFlows(source)
 
-    flows.map(flowToResultPairs).toSet.size shouldBe 6
-
     flows.map(flowToResultPairs).toSet shouldBe
       Set(
         List[(String, Option[Integer])](
@@ -872,7 +868,7 @@ class CDataFlowTests27 extends DataFlowCodeToCpgSuite {
       |""".stripMargin
 
   "Test 27: find flows of last statements to METHOD_RETURN" in {
-    val source = cpg.call("free")
+    val source = cpg.call("free").argument(1)
     val sink = cpg.method("foo").methodReturn
     implicit val s: Semantics = semantics
     val flows = sink.reachableByFlows(source).l
@@ -883,3 +879,44 @@ class CDataFlowTests27 extends DataFlowCodeToCpgSuite {
     )
   }
 }
+
+class CDataFlowTests28 extends DataFlowCodeToCpgSuite {
+  override val code =
+    """
+      |int foo() {
+      |   int y = 1;
+      |   y = something_else;
+      |   y = 10;
+      |}
+      |
+      |""".stripMargin
+
+  "Test 28: find that there is no flow from `y = 1` to exit node" in {
+    val source = cpg.literal("1").l
+    val sink = cpg.method("foo").methodReturn
+
+    val flows = sink.reachableByFlows(source).l
+    flows.size shouldBe 0
+  }
+}
+
+class CDataFlowTests29 extends DataFlowCodeToCpgSuite {
+  override val code =
+    """
+      |int foo() {
+      |   char * y = malloc(10);
+      |   free(y);
+      |   y = 10;
+      |}
+      |
+      |""".stripMargin
+
+  "Test 29: find that there is no flow from free(y) to exit node" in {
+    val source = cpg.call("free").argument(1).l
+    val sink = cpg.method("foo").methodReturn.l
+
+    implicit val s: Semantics = semantics
+    val flows = sink.reachableByFlows(source).l
+    flows.size shouldBe 0
+  }
+}

fuzzyc2cpg-tests/src/test/scala/io/shiftleft/fuzzyc2cpg/querying/DataFlowTests.scala

@@ -50,7 +50,7 @@ class DataFlowTests extends DataFlowCodeToCpgSuite {
   "should find flows from identifiers to return values of `flow`" in {
     val source = cpg.identifier
     val sink = cpg.method.name("flow").methodReturn
-    sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.size shouldBe 7
+    sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.size shouldBe 8
   }
 
   "find flows from z to method returns of flow" in {

fuzzyc2cpg-tests/src/test/scala/io/shiftleft/fuzzyc2cpg/querying/FreeListDataFlowTests.scala

@@ -50,7 +50,7 @@ class FreeListDataFlowTests extends DataFlowCodeToCpgSuite {
   "should find flows from identifiers to return values of `flow`" in {
     val source = cpg.identifier
     val sink = cpg.method.name("flow").methodReturn
-    sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.toSet.size shouldBe 7
+    sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.toSet.size shouldBe 8
   }
 
   "find flows from z to method returns of flow" in {