Fix CDG for ShortCircuitOperators

cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/graph/builder/Fluent.kt

@@ -1300,6 +1300,36 @@ operator fun Expression.minus(rhs: Expression): BinaryOperator {
     return node
 }
 
+/**
+ * Creates a new [BinaryOperator] with a `&&` [BinaryOperator.operatorCode] in the Fluent Node DSL
+ * and invokes [ArgumentHolder.addArgument] of the nearest enclosing [ArgumentHolder].
+ */
+context(frontend: LanguageFrontend<*, *>, holder: StatementHolder)
+infix fun Expression.logicAnd(rhs: Expression): BinaryOperator {
+    val node = frontend.newBinaryOperator("&&")
+    node.lhs = this
+    node.rhs = rhs
+
+    holder += node
+
+    return node
+}
+
+/**
+ * Creates a new [BinaryOperator] with a `||` [BinaryOperator.operatorCode] in the Fluent Node DSL
+ * and invokes [ArgumentHolder.addArgument] of the nearest enclosing [ArgumentHolder].
+ */
+context(frontend: LanguageFrontend<*, *>, holder: StatementHolder)
+infix fun Expression.logicOr(rhs: Expression): BinaryOperator {
+    val node = frontend.newBinaryOperator("||")
+    node.lhs = this
+    node.rhs = rhs
+
+    holder += node
+
+    return node
+}
+
 /**
  * Creates a new [UnaryOperator] with a `&` [UnaryOperator.operatorCode] in the Fluent Node DSL and
  * invokes [ArgumentHolder.addArgument] of the nearest enclosing [ArgumentHolder].

cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/graph/overlays/BasicBlock.kt

@@ -26,13 +26,10 @@
 package de.fraunhofer.aisec.cpg.graph.overlays
 
 import de.fraunhofer.aisec.cpg.PopulatedByPass
-import de.fraunhofer.aisec.cpg.graph.BranchingNode
 import de.fraunhofer.aisec.cpg.graph.Node
 import de.fraunhofer.aisec.cpg.graph.OverlayNode
 import de.fraunhofer.aisec.cpg.graph.edges.overlay.BasicBlockEdgeList
 import de.fraunhofer.aisec.cpg.graph.edges.unwrapping
-import de.fraunhofer.aisec.cpg.graph.statements.LoopStatement
-import de.fraunhofer.aisec.cpg.graph.statements.expressions.ComprehensionExpression
 import de.fraunhofer.aisec.cpg.helpers.neo4j.LocationConverter
 import de.fraunhofer.aisec.cpg.passes.BasicBlockCollectorPass
 import de.fraunhofer.aisec.cpg.sarif.PhysicalLocation
@@ -74,15 +71,12 @@ class BasicBlock() : OverlayNode() {
     val endNode: Node?
         get() = nodes.lastOrNull()
 
+    /**
+     * If this basic block ends in a branching point (i.e., has multiple outgoing EOG edges), this
+     * returns the [endNode] of this basic blocks EOG. Otherwise, null.
+     */
     val branchingNode: Node?
-        get() =
-            if (
-                endNode is BranchingNode ||
-                    endNode is LoopStatement ||
-                    endNode is ComprehensionExpression
-            ) {
-                endNode as Node
-            } else null
+        get() = if (this.nextEOG.size > 1) endNode else null
 
     @Convert(LocationConverter::class)
     override var location: PhysicalLocation? = null

cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/BasicBlockCollectorPass.kt

@@ -26,12 +26,15 @@
 package de.fraunhofer.aisec.cpg.passes
 
 import de.fraunhofer.aisec.cpg.TranslationContext
+import de.fraunhofer.aisec.cpg.frontends.HasShortCircuitOperators
 import de.fraunhofer.aisec.cpg.graph.EOGStarterHolder
 import de.fraunhofer.aisec.cpg.graph.Node
 import de.fraunhofer.aisec.cpg.graph.declarations.FunctionDeclaration
 import de.fraunhofer.aisec.cpg.graph.edges.flows.EvaluationOrder
 import de.fraunhofer.aisec.cpg.graph.overlays.BasicBlock
+import de.fraunhofer.aisec.cpg.graph.statements.expressions.ShortCircuitOperator
 import de.fraunhofer.aisec.cpg.passes.configuration.DependsOn
+import java.util.*
 
 @DependsOn(EvaluationOrderGraphPass::class)
 class BasicBlockCollectorPass(ctx: TranslationContext) : EOGStarterPass(ctx) {
@@ -41,20 +44,26 @@ class BasicBlockCollectorPass(ctx: TranslationContext) : EOGStarterPass(ctx) {
     }
 
     override fun accept(t: Node) {
-        val firstBasicBlock = collectBasicBlocks(t)
+        val firstBasicBlock = collectBasicBlocks(t, t.language is HasShortCircuitOperators)
         (t as? EOGStarterHolder)?.firstBasicBlock = firstBasicBlock
     }
 
     /**
      * Collects all basic blocks starting at the given [startNode]. We identify basic blocks by
      * iterating through the EOG in O(n) and collecting all nodes which are reachable from the
-     * [startNode].
+     * [startNode]. We identify basic blocks by merges and branches in the EOG but may not consider
+     * [ShortCircuitOperator]s as EOG-splitting nodes if we set [splitOnShortCircuitOperator] to
+     * `false`.
      *
      * It returns the first basic block, which is the one starting at the [startNode].
      *
      * @param startNode The node to start the collection from, usually a [FunctionDeclaration].
+     * @param splitOnShortCircuitOperator If true, the basic blocks will be split on short-circuit
+     *   operators (e.g., `&&` or `||`). If false, the short-circuit operators will not be
+     *   considered, and the basic blocks will be collected as if they were not splitting up the
+     *   EOG.
      */
-    fun collectBasicBlocks(startNode: Node): BasicBlock {
+    fun collectBasicBlocks(startNode: Node, splitOnShortCircuitOperator: Boolean): BasicBlock {
         val firstBB = BasicBlock()
         val worklist =
             mutableListOf<Triple<Node, EvaluationOrder?, BasicBlock>>(
@@ -71,7 +80,6 @@ class BasicBlockCollectorPass(ctx: TranslationContext) : EOGStarterPass(ctx) {
                 oldBB.prevEOG += basicBlock
                 continue
             }
-            // We have not seen this node yet, so we can continue.
 
             if (
                 currentStartNode.prevEOG.size > 1 &&
@@ -95,9 +103,30 @@ class BasicBlockCollectorPass(ctx: TranslationContext) : EOGStarterPass(ctx) {
                         }
                     }
             }
+
             basicBlock.nodes.add(currentStartNode)
 
-            val nextRelevantEOGEdges = currentStartNode.nextEOGEdges
+            // Note: This may not identify all cases correctly, e.g., if a ShortCircuitOperator is
+            // not the direct astParent.
+            val shortCircuit = currentStartNode.astParent as? ShortCircuitOperator
+            val language = shortCircuit?.language
+            val nextRelevantEOGEdges =
+                if (
+                    shortCircuit != null &&
+                        language is HasShortCircuitOperators &&
+                        !splitOnShortCircuitOperator &&
+                        currentStartNode.nextEOGEdges.size > 1
+                ) {
+                    // For ShortCircuitOperators, we select only the branch which is not a shortcut
+                    // because it's not really a CDG-relevant node, and we want to save the branches
+                    // it introduces.
+                    currentStartNode.nextEOGEdges.filter {
+                        shortCircuit.operatorCode in language.conjunctiveOperators == it.branch ||
+                            it.branch == null
+                    }
+                } else {
+                    currentStartNode.nextEOGEdges
+                }
 
             if (nextRelevantEOGEdges.size > 1) {
                 // If the currentStartNode splits up into multiple paths, the next nodes start a new
@@ -137,6 +166,7 @@ class BasicBlockCollectorPass(ctx: TranslationContext) : EOGStarterPass(ctx) {
                 // List is empty, nothing to do.
             }
         }
+
         return firstBB
     }
 }

cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/ControlDependenceGraphPass.kt

@@ -26,6 +26,7 @@
 package de.fraunhofer.aisec.cpg.passes
 
 import de.fraunhofer.aisec.cpg.TranslationContext
+import de.fraunhofer.aisec.cpg.frontends.HasShortCircuitOperators
 import de.fraunhofer.aisec.cpg.graph.BranchingNode
 import de.fraunhofer.aisec.cpg.graph.EOGStarterHolder
 import de.fraunhofer.aisec.cpg.graph.Node
@@ -44,8 +45,6 @@ import de.fraunhofer.aisec.cpg.helpers.functional.Lattice
 import de.fraunhofer.aisec.cpg.helpers.functional.MapLattice
 import de.fraunhofer.aisec.cpg.helpers.functional.PowersetLattice
 import de.fraunhofer.aisec.cpg.passes.configuration.DependsOn
-import kotlin.collections.component1
-import kotlin.collections.component2
 
 /** This pass builds the Control Dependence Graph (CDG) by iterating through the EOG. */
 @DependsOn(EvaluationOrderGraphPass::class)
@@ -102,7 +101,8 @@ open class ControlDependenceGraphPass(ctx: TranslationContext) : EOGStarterPass(
 
         val firstBasicBlock =
             (startNode as? EOGStarterHolder)?.firstBasicBlock
-                ?: BasicBlockCollectorPass(ctx).collectBasicBlocks(startNode)
+                ?: BasicBlockCollectorPass(ctx)
+                    .collectBasicBlocks(startNode, startNode.language is HasShortCircuitOperators)
 
         log.trace("Retrieved network of BBs for {}", startNode.name)
 
@@ -181,7 +181,7 @@ open class ControlDependenceGraphPass(ctx: TranslationContext) : EOGStarterPass(
                             }
                     }
                     .flatten()
-            finalDominators = finalDominators.minus(transitiveDominators).toMutableList()
+            finalDominators = finalDominators.minus(transitiveDominators.toSet()).toMutableList()
 
             // After deleting a bunch of stuff, we have two options: 1) there are no dominators
             // left, and we assign the function declaration, or 2) there is one or multiple
@@ -349,8 +349,14 @@ private fun EvaluationOrder.isConditionalBranch(): Boolean {
             startNode is ComprehensionExpression ||
             (startNode.astParent is ComprehensionExpression &&
                 startNode == (startNode.astParent as ComprehensionExpression).iterable) ||
-            startNode is ConditionalExpression ||
-            startNode is ShortCircuitOperator) && branch == false ||
+            startNode is ConditionalExpression) && branch == false ||
+            /*
+             * Code like `foo() && bar()` requires us to look-ahead for a [ShortCircuitOperator].
+             * The execution of the rhs of the [ShortCircuitOperator] always depends on the lhs:
+             * `foo() && bar()` -> `bar()` will only be called if `foo()` evaluates to `true`
+             * `foo() || bar()` -> `bar()` will only be called if `foo() evaluates to `false`
+             */
+            startNode.nextEOG.filterIsInstance<ShortCircuitOperator>().isNotEmpty() ||
             (startNode is IfStatement &&
                 !startNode.allBranchesFromMyThenBranchGoThrough(startNode.nextUnconditionalNode))
 }

cpg-core/src/test/kotlin/de/fraunhofer/aisec/cpg/passes/ControlDependenceGraphPassTest.kt

@@ -27,19 +27,19 @@ package de.fraunhofer.aisec.cpg.passes
 
 import de.fraunhofer.aisec.cpg.TranslationConfiguration
 import de.fraunhofer.aisec.cpg.frontends.TestLanguageWithColon
+import de.fraunhofer.aisec.cpg.frontends.TestLanguageWithShortCircuit
 import de.fraunhofer.aisec.cpg.frontends.testFrontend
 import de.fraunhofer.aisec.cpg.graph.*
 import de.fraunhofer.aisec.cpg.graph.builder.*
 import de.fraunhofer.aisec.cpg.graph.statements.expressions.Block
 import de.fraunhofer.aisec.cpg.graph.statements.expressions.Literal
 import kotlin.test.Test
 import kotlin.test.assertEquals
-import kotlin.test.assertNotNull
 import kotlin.test.assertTrue
 import org.junit.jupiter.api.Assertions.assertFalse
+import org.junit.jupiter.api.assertNotNull
 
 class ControlDependenceGraphPassTest {
-
     @Test
     fun testIfStatements() {
         val result = getIfTest()
@@ -112,12 +112,73 @@ class ControlDependenceGraphPassTest {
     }
 
     @Test
-    fun testTimoutEffective() {
+    fun testTimeoutEffective() {
         val result = getTimeoutTest()
         assertTrue { result.allChildren<Node>().flatMap { it.nextCDG }.isEmpty() }
     }
 
+    @Test
+    fun testShortCircuit() {
+        val result = getShortCircuitTest()
+        assertNotNull(result)
+
+        val barCall = result.calls("bar").singleOrNull()
+        assertNotNull(barCall)
+        val fooCall = result.calls("foo").singleOrNull()
+        assertNotNull(fooCall)
+        val bazCall = result.calls("baz").singleOrNull()
+        assertNotNull(bazCall)
+        val quuxCall = result.calls("quux").singleOrNull()
+        assertNotNull(quuxCall)
+        assertTrue(
+            barCall.prevCDG.contains(fooCall),
+            "Expected 'bar()' to be control dependent on 'foo()'",
+        ) // TODO: Once we update the ShortCircuitOperator to a better EOG description, this should
+        // test against the operator instead of foo().
+
+        assertTrue(
+            quuxCall.prevCDG.contains(bazCall),
+            "Expected 'quux()' to be control dependent on 'baz()'",
+        ) // TODO: Once we update the ShortCircuitOperator to a better EOG description, this should
+        // test against the operator instead of baz().
+    }
+
     companion object {
+
+        /**
+         * Test language with [HasShortCircuitOperators] to test short-circuit behavior in CDG.
+         *
+         * ```c
+         * int main() {
+         *   foo() && bar();
+         *   baz() || quux();
+         *   return 1;
+         * }
+         * ```
+         */
+        fun getShortCircuitTest() =
+            testFrontend(
+                    TranslationConfiguration.builder()
+                        .registerLanguage<TestLanguageWithShortCircuit>()
+                        .defaultPasses()
+                        .registerPass<ControlDependenceGraphPass>()
+                        .build()
+                )
+                .build {
+                    translationResult {
+                        translationUnit("if.cpp") {
+                            // The main method
+                            function("main", t("int")) {
+                                body {
+                                    call("foo") logicAnd call("bar")
+                                    call("baz") logicOr call("quux")
+                                    returnStmt { literal(1, t("int")) }
+                                }
+                            }
+                        }
+                    }
+                }
+
         fun getIfTest() =
             testFrontend(
                     TranslationConfiguration.builder()

cpg-core/src/testFixtures/kotlin/de/fraunhofer/aisec/cpg/frontends/TestLanguage.kt

@@ -25,7 +25,8 @@
  */
 package de.fraunhofer.aisec.cpg.frontends
 
-import de.fraunhofer.aisec.cpg.*
+import de.fraunhofer.aisec.cpg.TranslationConfiguration
+import de.fraunhofer.aisec.cpg.TranslationContext
 import de.fraunhofer.aisec.cpg.graph.Node
 import de.fraunhofer.aisec.cpg.graph.declarations.ProblemDeclaration
 import de.fraunhofer.aisec.cpg.graph.declarations.TranslationUnitDeclaration
@@ -36,6 +37,12 @@ import java.io.File
 import kotlin.reflect.KClass
 import kotlin.test.assertNotNull
 
+/** This is a variant of the test language with [HasShortCircuitOperators]. */
+open class TestLanguageWithShortCircuit() : TestLanguage(), HasShortCircuitOperators {
+    override val conjunctiveOperators: List<String> = listOf("&&")
+    override val disjunctiveOperators: List<String> = listOf("||")
+}
+
 /**
  * This is a variant of the test language with `::` as a [namespaceDelimiter] to simulate languages
  * like C++.