Repository: joernio/joern (scala)

Issue:
[Bug] c2cpg: turn comma operator into block instead of <operator>.expressionList
```
joern> importCode.cpp.fromString("""
int something(void);
void a() {
    int b;
    int c;
    for (; b = something(), b > c;) {
    }
}
""") 
```


The comma operator in the condition is turned into a `<operator>.expressionList` here. Due to the varargs nature of that, we have a hard time of tracking which data is exactly flowing into the condition there (only the last `b > c`). The comma operator has strict left-to-right evaluation semantics and returns the right operand - so a block is a perfect match to encode those semantics.

Note that I'm not talking about [std::initializer_list](https://en.cppreference.com/w/cpp/utility/initializer_list). An operator call seems appropriate there, since all the values in that list will be seen by the callee, not just the last one.

Pull request:
Fixes: https://github.com/joernio/joern/issues/2563

Patch:
diff --git a/joern-cli/frontends/c2cpg/src/main/scala/io/joern/c2cpg/astcreation/AstForStatementsCreator.scala b/joern-cli/frontends/c2cpg/src/main/scala/io/joern/c2cpg/astcreation/AstForStatementsCreator.scala
index 850b02da1928..53c0597c204c 100644
--- a/joern-cli/frontends/c2cpg/src/main/scala/io/joern/c2cpg/astcreation/AstForStatementsCreator.scala
+++ b/joern-cli/frontends/c2cpg/src/main/scala/io/joern/c2cpg/astcreation/AstForStatementsCreator.scala
@@ -2,6 +2,7 @@ package io.joern.c2cpg.astcreation
 
 import io.shiftleft.codepropertygraph.generated.ControlStructureTypes
 import io.joern.x2cpg.Ast
+import io.shiftleft.codepropertygraph.generated.nodes.ExpressionNew
 import org.eclipse.cdt.core.dom.ast._
 import org.eclipse.cdt.core.dom.ast.cpp._
 import org.eclipse.cdt.core.dom.ast.gnu.IGNUASTGotoStatement
@@ -89,7 +90,7 @@ trait AstForStatementsCreator { this: AstCreator =>
   private def astForDoStatement(doStmt: IASTDoStatement): Ast = {
     val code         = nodeSignature(doStmt)
     val doNode       = controlStructureNode(doStmt, ControlStructureTypes.DO, code)
-    val conditionAst = nullSafeAst(doStmt.getCondition)
+    val conditionAst = astForConditionExpression(doStmt.getCondition)
     val bodyAst      = nullSafeAst(doStmt.getBody)
     controlStructureAst(doNode, Some(conditionAst), bodyAst, placeConditionLast = true)
   }
@@ -97,14 +98,14 @@ trait AstForStatementsCreator { this: AstCreator =>
   private def astForSwitchStatement(switchStmt: IASTSwitchStatement): Ast = {
     val code         = s"switch(${nullSafeCode(switchStmt.getControllerExpression)})"
     val switchNode   = controlStructureNode(switchStmt, ControlStructureTypes.SWITCH, code)
-    val conditionAst = nullSafeAst(switchStmt.getControllerExpression)
+    val conditionAst = astForConditionExpression(switchStmt.getControllerExpression)
     val stmtAsts     = nullSafeAst(switchStmt.getBody)
     controlStructureAst(switchNode, Some(conditionAst), stmtAsts)
   }
 
   private def astsForCaseStatement(caseStmt: IASTCaseStatement): Seq[Ast] = {
     val labelNode = newJumpTargetNode(caseStmt)
-    val stmt      = nullSafeAst(caseStmt.getExpression)
+    val stmt      = astForConditionExpression(caseStmt.getExpression)
     Seq(Ast(labelNode), stmt)
   }
 
@@ -149,6 +150,25 @@ trait AstForStatementsCreator { this: AstCreator =>
     r.map(x => asChildOfMacroCall(statement, x))
   }
 
+  private def astForConditionExpression(expr: IASTExpression, explicitArgumentIndex: Option[Int] = None): Ast = {
+    val ast = expr match {
+      case exprList: IASTExpressionList =>
+        val compareAstBlock = blockNode(expr, Defines.empty, registerType(Defines.voidTypeName))
+        scope.pushNewScope(compareAstBlock)
+        val compareBlockAstChildren = exprList.getExpressions.toList.map(nullSafeAst)
+        setArgumentIndices(compareBlockAstChildren)
+        val compareBlockAst = blockAst(compareAstBlock, compareBlockAstChildren)
+        scope.popScope()
+        compareBlockAst
+      case other =>
+        nullSafeAst(other)
+    }
+    explicitArgumentIndex.foreach { i =>
+      ast.root.foreach { case expr: ExpressionNew => expr.argumentIndex = i }
+    }
+    ast
+  }
+
   private def astForFor(forStmt: IASTForStatement): Ast = {
     val codeInit = nullSafeCode(forStmt.getInitializerStatement)
     val codeCond = nullSafeCode(forStmt.getConditionExpression)
@@ -162,7 +182,7 @@ trait AstForStatementsCreator { this: AstCreator =>
     val initAst = blockAst(initAstBlock, nullSafeAst(forStmt.getInitializerStatement, 1).toList)
     scope.popScope()
 
-    val compareAst = nullSafeAst(forStmt.getConditionExpression, 2)
+    val compareAst = astForConditionExpression(forStmt.getConditionExpression, Some(2))
     val updateAst  = nullSafeAst(forStmt.getIterationExpression, 3)
     val bodyAsts   = nullSafeAst(forStmt.getBody, 4)
     forAst(forNode, Seq(), Seq(initAst), Seq(compareAst), Seq(updateAst), bodyAsts)
@@ -182,22 +202,18 @@ trait AstForStatementsCreator { this: AstCreator =>
   }
 
   private def astForWhile(whileStmt: IASTWhileStatement): Ast = {
-    val code         = s"while (${nullSafeCode(whileStmt.getCondition)})"
-    val conditionAst = nullSafeAst(whileStmt.getCondition)
-    val bodyAst      = nullSafeAst(whileStmt.getBody)
-    whileAst(Some(conditionAst), bodyAst, Some(code))
+    val code       = s"while (${nullSafeCode(whileStmt.getCondition)})"
+    val compareAst = astForConditionExpression(whileStmt.getCondition)
+    val bodyAst    = nullSafeAst(whileStmt.getBody)
+    whileAst(Some(compareAst), bodyAst, Some(code))
   }
 
   private def astForIf(ifStmt: IASTIfStatement): Ast = {
     val (code, conditionAst) = ifStmt match {
-      case s: CASTIfStatement =>
-        val c = s"if (${nullSafeCode(s.getConditionExpression)})"
-        val a = nullSafeAst(ifStmt.getConditionExpression)
-        (c, a)
-      case s: CPPASTIfStatement if s.getConditionExpression != null =>
-        val c = s"if (${nullSafeCode(s.getConditionExpression)})"
-        val a = nullSafeAst(ifStmt.getConditionExpression)
-        (c, a)
+      case s @ (_: CASTIfStatement | _: CPPASTIfStatement) if s.getConditionExpression != null =>
+        val c          = s"if (${nullSafeCode(s.getConditionExpression)})"
+        val compareAst = astForConditionExpression(s.getConditionExpression)
+        (c, compareAst)
       case s: CPPASTIfStatement if s.getConditionExpression == null =>
         val c         = s"if (${nullSafeCode(s.getConditionDeclaration)})"
         val exprBlock = blockNode(s.getConditionDeclaration, Defines.empty, Defines.voidTypeName)


Test patch:
diff --git a/joern-cli/frontends/c2cpg/src/test/scala/io/joern/c2cpg/passes/ast/AstCreationPassTests.scala b/joern-cli/frontends/c2cpg/src/test/scala/io/joern/c2cpg/passes/ast/AstCreationPassTests.scala
index 8ca2c7ecbba7..cde931beeab7 100644
--- a/joern-cli/frontends/c2cpg/src/test/scala/io/joern/c2cpg/passes/ast/AstCreationPassTests.scala
+++ b/joern-cli/frontends/c2cpg/src/test/scala/io/joern/c2cpg/passes/ast/AstCreationPassTests.scala
@@ -763,6 +763,24 @@ class AstCreationPassTests extends AbstractPassTest {
       arg2.code shouldBe "foo(x)"
     }
 
+    "not create an expression list for comma operator" in AstFixture("""
+        |int something(void);
+        |void a() {
+        |  int b;
+        |  int c;
+        |  for (; b = something(), b > c;) {}
+        |}
+      """.stripMargin) { cpg =>
+      val List(forLoop)        = cpg.controlStructure.l
+      val List(conditionBlock) = forLoop.condition.collectAll[Block].l
+      conditionBlock.argumentIndex shouldBe 2
+      val List(assignmentCall, greaterCall) = conditionBlock.astChildren.collectAll[Call].l
+      assignmentCall.argumentIndex shouldBe 1
+      assignmentCall.code shouldBe "b = something()"
+      greaterCall.argumentIndex shouldBe 2
+      greaterCall.code shouldBe "b > c"
+    }
+
     "be correct for call expression" in AstFixture("""
         |void method(int x) {
         |  foo(x);