From b8538e705d4a98ef40c93d40bebffaa60f37878d Mon Sep 17 00:00:00 2001 From: Johannes Coetzee Date: Thu, 30 May 2024 13:51:22 +0200 Subject: [PATCH] [javasrc2cpg] Don't add unknown nodes to cpg for unhandled captured variables (#4617) * Add reproducing test * Add tests for new lambdas * Never add Unknown node for captured variable * Add todos to comments about lambda parameter capturing --- .../AstForNameExpressionsCreator.scala | 27 +++----- .../javasrc2cpg/querying/LambdaTests.scala | 68 +++++++++++++++++++ 2 files changed, 76 insertions(+), 19 deletions(-) diff --git a/joern-cli/frontends/javasrc2cpg/src/main/scala/io/joern/javasrc2cpg/astcreation/expressions/AstForNameExpressionsCreator.scala b/joern-cli/frontends/javasrc2cpg/src/main/scala/io/joern/javasrc2cpg/astcreation/expressions/AstForNameExpressionsCreator.scala index d3f17a2ad8fc..8b8ad10c6a34 100644 --- a/joern-cli/frontends/javasrc2cpg/src/main/scala/io/joern/javasrc2cpg/astcreation/expressions/AstForNameExpressionsCreator.scala +++ b/joern-cli/frontends/javasrc2cpg/src/main/scala/io/joern/javasrc2cpg/astcreation/expressions/AstForNameExpressionsCreator.scala @@ -97,22 +97,17 @@ trait AstForNameExpressionsCreator { this: AstCreator => val variable = capturedVariable.variable val typeDeclChain = capturedVariable.typeDeclChain - scope.enclosingMethod.map(_.lookupVariable("this")) match { - case None | Some(NotInScope) | Some(CapturedVariable(_, _)) => + scope.lookupVariable("this") match { + case NotInScope | CapturedVariable(_, _) => logger.warn( s"Attempted to create AST for captured variable ${variable.name}, but could not find `this` param in direct scope." ) - Ast(NewUnknown().code(variable.name).lineNumber(line(nameExpr)).columnNumber(column(nameExpr))) - - case Some(SimpleVariable(ScopeParameter(thisNode: NewMethodParameterIn))) => - val thisIdentifier = identifierNode( - nameExpr, - thisNode.name, - thisNode.code, - thisNode.typeFullName, - thisNode.dynamicTypeHintFullName - ) - val thisAst = Ast(thisIdentifier).withRefEdge(thisIdentifier, thisNode) + Ast(identifierNode(nameExpr, variable.name, variable.name, variable.typeFullName)) + + case SimpleVariable(scopeVariable) => + val thisIdentifier = + identifierNode(nameExpr, scopeVariable.name, scopeVariable.name, scopeVariable.typeFullName) + val thisAst = Ast(thisIdentifier).withRefEdge(thisIdentifier, scopeVariable.node) val lineNumber = line(nameExpr) val columnNumber = column(nameExpr) @@ -139,12 +134,6 @@ trait AstForNameExpressionsCreator { this: AstCreator => val captureFieldIdentifier = fieldIdentifierNode(nameExpr, variable.name, variable.name) callAst(finalFieldAccess, List(outerClassChain, Ast(captureFieldIdentifier))) - - case Some(SimpleVariable(thisNode)) => - logger.warn( - s"Attempted to create AST for captured variable ${variable.name}, but found non-parameter `this`: ${thisNode}." - ) - Ast(NewUnknown().code(variable.name).lineNumber(line(nameExpr)).columnNumber(column(nameExpr))) } } } diff --git a/joern-cli/frontends/javasrc2cpg/src/test/scala/io/joern/javasrc2cpg/querying/LambdaTests.scala b/joern-cli/frontends/javasrc2cpg/src/test/scala/io/joern/javasrc2cpg/querying/LambdaTests.scala index 7a67dd20ef00..aecb56aa7fd2 100644 --- a/joern-cli/frontends/javasrc2cpg/src/test/scala/io/joern/javasrc2cpg/querying/LambdaTests.scala +++ b/joern-cli/frontends/javasrc2cpg/src/test/scala/io/joern/javasrc2cpg/querying/LambdaTests.scala @@ -742,4 +742,72 @@ class LambdaTests extends JavaSrcCode2CpgFixture { cpg.call.nameExact("").count(_.argument.isEmpty) shouldBe 0 } } + + "calls on captured variables in lambdas contained in anonymous classes" should { + val cpg = code(""" + | + |public class Foo { + | + | public static void sink(String s) {}; + | + | public static Object test(Bar captured) { + | Visitor v = new Visitor() { + | public void visit(Visited visited) { + | visited.getList().forEach(lambdaParam -> captured.remove(lambdaParam)); + | } + | }; + | } + |} + |""".stripMargin) + + // TODO: This behaviour isn't exactly correct, but is on par with how we currently handle field captures in lambdas. + "have the correct receiver ast" in { + inside(cpg.call.name("remove").receiver.l) { case List(fieldAccessCall: Call) => + fieldAccessCall.name shouldBe Operators.fieldAccess + + inside(fieldAccessCall.argument.l) { case List(identifier: Identifier, fieldIdentifier: FieldIdentifier) => + identifier.name shouldBe "this" + identifier.typeFullName shouldBe "Foo.test.Visitor$0" + + fieldIdentifier.canonicalName shouldBe "captured" + + fieldAccessCall.typeFullName shouldBe ".Bar" + } + } + } + } + + // TODO: These tests exist to document current behaviour, but the current behaviour is wrong. + "lambdas capturing parameters" should { + val cpg = code(""" + |import java.util.function.Consumer; + | + |public class Foo { + | public String capturedField; + | + | public void foo() { + | Consumer consumer = lambdaParam -> System.out.println(capturedField); + | } + |} + |""".stripMargin) + + "represent the captured field as a field access" in { + inside(cpg.method.name(".*lambda.*").call.name("println").argument.l) { case List(_, fieldAccessCall: Call) => + fieldAccessCall.name shouldBe Operators.fieldAccess + + inside(fieldAccessCall.argument.l) { case List(identifier: Identifier, fieldIdentifier: FieldIdentifier) => + identifier.name shouldBe "this" + identifier.typeFullName shouldBe "Foo" + + fieldIdentifier.canonicalName shouldBe "capturedField" + } + } + } + + // TODO: It should, but it doesn't. + "have a captured local for the enclosing class" in { + // There should be an `outerClass` local which captures the outer method `this`. + cpg.method.name(".*lambda.*").local.name("this").typeFullName(".*Foo.*").isEmpty shouldBe true + } + } }