Repository: joernio/joern (scala)

Issue:
[Bug] Reaching def edges are missing for some global definitions
Consider the following C code:

```C
int a = 0;
int b = 1;
int c = 2;

void foo() {
  bar(a, b, c);
}
```

There are now 3 outgoing reaching def edges for global identifier `a`, but only 2 for `b` and `c`. `b` and `c` are missing the edges to the arguments to the `bar` call. This edge is generated for `a`.

Now it gets weird. 

```C
int b = 0;
int a = 1;
int c = 2;

void foo() {
  bar(a, b, c);
}
```

After simply switching global `a` and `b`, the reaching def edges for `b` are correct and `a` is missing the edge to the `bar` call argument.

The exact same thing happens for pysrc2cpg. So I guess thats DF engine related.

(Reported via Discord: https://discord.com/channels/832209896089976854/832214243230744626/1375144863716085894)

Pull request:
Fixed an overapproximation in `SourcesToStartingPoints` that propagates an identifier as a source in many more places than it should, ending the propagation if the identifier is an argument to some call or operation already.

Resolves https://github.com/joernio/joern/issues/5520
(see ticket for discussion)

Note: This is branched off of https://github.com/joernio/joern/pull/5522
Resolves: https://github.com/joernio/joern/issues/5511

Patch:
diff --git a/dataflowengineoss/src/main/scala/io/joern/dataflowengineoss/package.scala b/dataflowengineoss/src/main/scala/io/joern/dataflowengineoss/package.scala
index 7c7417a3506a..d8467bf73a6f 100644
--- a/dataflowengineoss/src/main/scala/io/joern/dataflowengineoss/package.scala
+++ b/dataflowengineoss/src/main/scala/io/joern/dataflowengineoss/package.scala
@@ -34,12 +34,17 @@ package object dataflowengineoss {
       .target
   }
 
-  def identifierToFirstUsages(node: Identifier): List[Identifier] = node.refsTo.flatMap(identifiersFromCapturedScopes).l
-
-  def identifiersFromCapturedScopes(i: Declaration): List[Identifier] =
-    i.capturedByMethodRef.referencedMethod.ast.isIdentifier
-      .nameExact(i.name)
-      .sortBy(x => (x.lineNumber, x.columnNumber))
+  def identifierToFirstUsages(node: Identifier): List[Identifier] =
+    node.refsTo.flatMap(firstIdentifierFromCapturedScopes).l
+
+  def firstIdentifierFromCapturedScopes(i: Declaration): List[Identifier] =
+    i.capturedByMethodRef.referencedMethod
+      .flatMap(m =>
+        m.ast.isIdentifier // this includes closures defined under method
+          .nameExact(i.name)
+          .sortBy(x => (x.lineNumber, x.columnNumber))
+          .headOption
+      )
       .l
 
 }
diff --git a/dataflowengineoss/src/main/scala/io/joern/dataflowengineoss/passes/reachingdef/DdgGenerator.scala b/dataflowengineoss/src/main/scala/io/joern/dataflowengineoss/passes/reachingdef/DdgGenerator.scala
index 7693f28757ff..155a1de7d598 100644
--- a/dataflowengineoss/src/main/scala/io/joern/dataflowengineoss/passes/reachingdef/DdgGenerator.scala
+++ b/dataflowengineoss/src/main/scala/io/joern/dataflowengineoss/passes/reachingdef/DdgGenerator.scala
@@ -169,12 +169,9 @@ class DdgGenerator(semantics: Semantics) {
 
     def addEdgesToCapturedIdentifiersAndParameters(): Unit = {
       val identifierDestPairs =
-        method._identifierViaContainsOut
-          .flatMap { identifier =>
-            identifierToFirstUsages(identifier).map(usage => (identifier, usage))
-          }
-          .l
-          .distinctBy(_._2.method)
+        method._identifierViaContainsOut.flatMap { identifier =>
+          identifierToFirstUsages(identifier).map(usage => (identifier, usage))
+        }.l
 
       identifierDestPairs
         .foreach { case (src, dst) =>
diff --git a/dataflowengineoss/src/main/scala/io/joern/dataflowengineoss/queryengine/SourcesToStartingPoints.scala b/dataflowengineoss/src/main/scala/io/joern/dataflowengineoss/queryengine/SourcesToStartingPoints.scala
index 74b651ef428a..fe8a9fa6c126 100644
--- a/dataflowengineoss/src/main/scala/io/joern/dataflowengineoss/queryengine/SourcesToStartingPoints.scala
+++ b/dataflowengineoss/src/main/scala/io/joern/dataflowengineoss/queryengine/SourcesToStartingPoints.scala
@@ -226,6 +226,7 @@ abstract class BaseSourceToStartingPoints extends Callable[Unit] {
 
   private def withFieldAndIndexAccesses(nodes: List[CfgNode]): List[CfgNode] =
     nodes.flatMap {
+      case identifier: Identifier if identifier.isArgument.nonEmpty => identifier :: Nil
       case moduleVar: Identifier if moduleVar.isModuleVariable =>
         moduleVar :: moduleVariableToFirstUsagesAcrossProgram(moduleVar)
       case identifier: Identifier => identifier :: fieldAndIndexAccesses(identifier)


Test patch:
diff --git a/joern-cli/frontends/c2cpg/src/test/scala/io/joern/c2cpg/dataflow/DataFlowTests.scala b/joern-cli/frontends/c2cpg/src/test/scala/io/joern/c2cpg/dataflow/DataFlowTests.scala
index 59148cf6b66b..117f7de7a17b 100644
--- a/joern-cli/frontends/c2cpg/src/test/scala/io/joern/c2cpg/dataflow/DataFlowTests.scala
+++ b/joern-cli/frontends/c2cpg/src/test/scala/io/joern/c2cpg/dataflow/DataFlowTests.scala
@@ -45,13 +45,13 @@ class DataFlowTests extends DataFlowCodeToCpgSuite {
       implicit val callResolver: NoResolve.type = NoResolve
       val source                                = cpg.identifier
       val sink                                  = cpg.method.name("free").parameter.argument
-      sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.size shouldBe 6
+      sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.size shouldBe 5
     }
 
     "find flows to `free`" in {
       val source = cpg.identifier
       val sink   = cpg.call.name("free").argument(1)
-      sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.size shouldBe 6
+      sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.size shouldBe 5
     }
 
     "find flows from identifiers to return values of `flow`" in {
@@ -464,7 +464,7 @@ class DataFlowTests extends DataFlowCodeToCpgSuite {
       val source = cpg.identifier
       val sink   = cpg.method.name("free").parameter.argument
       val flows  = sink.reachableByFlows(source).map(flowToResultPairs).l.distinct
-      flows.size shouldBe 6
+      flows.size shouldBe 5
     }
   }
 
@@ -1307,13 +1307,13 @@ class DataFlowTests extends DataFlowCodeToCpgSuite {
       implicit val callResolver: NoResolve.type = NoResolve
       val source                                = cpg.identifier
       val sink                                  = cpg.method.name("free").parameter.argument
-      sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.toSet.size shouldBe 6
+      sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.toSet.size shouldBe 5
     }
 
     "find flows to `free`" in {
       val source = cpg.identifier
       val sink   = cpg.call.name("free").argument(1)
-      sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.toSet.size shouldBe 6
+      sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.toSet.size shouldBe 5
     }
 
     "find flows from identifiers to return values of `flow`" in {
@@ -1638,6 +1638,24 @@ class DataFlowTests extends DataFlowCodeToCpgSuite {
     }
   }
 
+  "DataFlowTest71" should {
+    val cpg = code("""
+        |void abc() {
+        |  char *a;
+        |  fgets(a, 10, stdin);
+        |  a = "something";
+        |  a[5] = "\0";
+        |  system(a);
+        |}""".stripMargin)
+
+    "not find a flow from 'a' at 'fgets' to 'system'" in {
+      val src  = cpg.call("fgets").argument(1).l
+      val sink = cpg.call("system").argument(1).l
+      val flow = sink.reachableByFlows(src)
+      sink.reachableByFlows(src).size shouldBe 0
+    }
+  }
+
 }
 
 class DataFlowTestsWithCallDepth extends DataFlowCodeToCpgSuite {
@@ -2171,4 +2189,35 @@ class DataFlowTestsWithCallDepth extends DataFlowCodeToCpgSuite {
       )
     }
   }
+
+  "DataFlowTest81" should {
+    val cpg = code("""int a = 1;
+        |int b = 2;
+        |int c = 3;
+        |
+        |void foo() {
+        |  bar(a, b, c);
+        |}
+        |""".stripMargin)
+
+    "not find a flow from top-level identifiers to call inside function" in {
+      val one   = cpg.literal.code("1").l
+      val two   = cpg.literal.code("2").l
+      val three = cpg.literal.code("3").l
+      one.size shouldBe 1
+      two.size shouldBe 1
+      three.size shouldBe 1
+
+      val sink = cpg.call("bar").argument.argumentIndexGte(1).l
+      sink.size shouldBe 3
+
+      def flows(source: Iterator[CfgNode]): Set[List[(String, Integer)]] =
+        sink.reachableByFlows(source).map(flowToResultPairs).toSet
+
+      flows(one) shouldBe Set(("a = 1", 1) :: ("bar(a, b, c)", 6) :: Nil)
+      flows(two) shouldBe Set(("b = 2", 2) :: ("bar(a, b, c)", 6) :: Nil)
+      flows(three) shouldBe Set(("c = 3", 3) :: ("bar(a, b, c)", 6) :: Nil)
+    }
+  }
+
 }
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
index cf3812d311db..6a382ce21049 100644
--- 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
@@ -11,7 +11,7 @@ import io.joern.dataflowengineoss.semanticsloader.{
   PassThroughMapping
 }
 import io.joern.pysrc2cpg.testfixtures.PySrc2CpgFixture
-import io.shiftleft.codepropertygraph.generated.Cpg
+import io.shiftleft.codepropertygraph.generated.{Cpg, Operators}
 import io.shiftleft.codepropertygraph.generated.nodes.{Literal, Member, Method}
 import io.shiftleft.semanticcpg.language.*
 
@@ -422,8 +422,7 @@ class DataFlowTests extends PySrc2CpgFixture(withOssDataflow = true) {
         |    y['B'] = x['A']
         |    sink(y)
         |""".stripMargin)
-
-    val sources = cpg.identifier("x").l
+    val sources = cpg.assignment.target.isIdentifier.nameExact("y").l
     val sinks   = cpg.call("sink").argument.l
     val flows   = sinks.reachableByFlows(sources)
     flows.size shouldBe 1
@@ -442,8 +441,8 @@ class DataFlowTests extends PySrc2CpgFixture(withOssDataflow = true) {
         |       return sink(d)
         |""".stripMargin)
 
-    val sources = cpg.call("<operator>.indexAccess").argument.isIdentifier.l
-    val sinks   = cpg.call("sink").l
+    val sources = cpg.call(Operators.indexAccess).where(_.code(".*['(x|y|z)'].*")).l
+    val sinks   = cpg.call("sink").argument.l
     sinks.reachableByFlows(sources).size should not be 0
   }
 
@@ -1185,9 +1184,9 @@ class RegexDefinedFlowsDataFlowTests
         |print(Foo.func())
         |""".stripMargin)
     "be found" in {
-      val src = cpg.identifier("Foo").l
+      val src = cpg.call("func").receiver.l
       val snk = cpg.call("print").argument(1).l
-      snk.reachableByFlows(src).size shouldBe 3
+      snk.reachableByFlows(src).size shouldBe 1
     }
   }
   "Import statement with method ref sample four" in {
diff --git a/joern-cli/frontends/rubysrc2cpg/src/test/scala/io/joern/rubysrc2cpg/dataflow/ClassTests.scala b/joern-cli/frontends/rubysrc2cpg/src/test/scala/io/joern/rubysrc2cpg/dataflow/ClassTests.scala
index 6632c0aff827..9a0c00067d36 100644
--- a/joern-cli/frontends/rubysrc2cpg/src/test/scala/io/joern/rubysrc2cpg/dataflow/ClassTests.scala
+++ b/joern-cli/frontends/rubysrc2cpg/src/test/scala/io/joern/rubysrc2cpg/dataflow/ClassTests.scala
@@ -159,7 +159,7 @@ class ClassTests extends RubyCode2CpgFixture(withPostProcessing = true, withData
 
     val source = cpg.identifier.name("x").l
     val sink   = cpg.call.name("puts").l
-    sink.reachableByFlows(source).size shouldBe 2
+    sink.reachableByFlows(source).size shouldBe 1
   }
 
   // TODO:
diff --git a/joern-cli/frontends/swiftsrc2cpg/src/test/scala/io/joern/swiftsrc2cpg/dataflow/DataFlowTests.scala b/joern-cli/frontends/swiftsrc2cpg/src/test/scala/io/joern/swiftsrc2cpg/dataflow/DataFlowTests.scala
index e5980342ecf8..6bf37b80e4e3 100644
--- a/joern-cli/frontends/swiftsrc2cpg/src/test/scala/io/joern/swiftsrc2cpg/dataflow/DataFlowTests.scala
+++ b/joern-cli/frontends/swiftsrc2cpg/src/test/scala/io/joern/swiftsrc2cpg/dataflow/DataFlowTests.scala
@@ -46,13 +46,13 @@ class DataFlowTests extends DataFlowCodeToCpgSuite {
       implicit val callResolver: NoResolve.type = NoResolve
       val source                                = cpg.identifier
       val sink                                  = cpg.call.name("free").argumentExact("elem")
-      sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.size shouldBe 4
+      sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.size shouldBe 3
     }
 
     "find flows to `free`" in {
       val source = cpg.identifier
       val sink   = cpg.call.name("free")
-      sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.size shouldBe 4
+      sink.reachableByFlows(source).l.map(flowToResultPairs).distinct.size shouldBe 3
     }
 
     "find flows from identifiers to return values of `flow`" in {