From 335a14cad02c9d615d7d79c8853ee332708d714e Mon Sep 17 00:00:00 2001 From: Andreas Kellas Date: Wed, 15 Nov 2023 08:26:12 -0500 Subject: [PATCH] Remove excessive debug statements and format comments --- .../php2cpg/passes/PhpTypeRecovery.scala | 205 ++---------------- .../passes/PhpTypeRecoveryPassTests.scala | 18 +- 2 files changed, 30 insertions(+), 193 deletions(-) diff --git a/joern-cli/frontends/php2cpg/src/main/scala/io/joern/php2cpg/passes/PhpTypeRecovery.scala b/joern-cli/frontends/php2cpg/src/main/scala/io/joern/php2cpg/passes/PhpTypeRecovery.scala index 60be64698ebc..5171c3be2aa8 100644 --- a/joern-cli/frontends/php2cpg/src/main/scala/io/joern/php2cpg/passes/PhpTypeRecovery.scala +++ b/joern-cli/frontends/php2cpg/src/main/scala/io/joern/php2cpg/passes/PhpTypeRecovery.scala @@ -35,13 +35,6 @@ private class PhpTypeRecovery(cpg: Cpg, state: XTypeRecoveryState) extends XType private class RecoverForPhpFile(cpg: Cpg, cu: NamespaceBlock, builder: DiffGraphBuilder, state: XTypeRecoveryState) extends RecoverForXCompilationUnit[NamespaceBlock](cpg, cu, builder, state) { - - override protected def prepopulateSymbolTable(): Unit = { - logger.debug(s"prepopulating symbol table") - super.prepopulateSymbolTable() - logger.debug(s"symbol table: ${symbolTable.view.mkString(" ")}") - } - override protected def prepopulateSymbolTableEntry(x: AstNode): Unit = x match { case x: Call => x.methodFullName match { @@ -70,64 +63,21 @@ private class RecoverForPhpFile(cpg: Cpg, cu: NamespaceBlock, builder: DiffGraph .filter(_.dispatchType == DispatchTypes.DYNAMIC_DISPATCH) .filter(_.methodFullName.startsWith(Defines.UnresolvedNamespace)) - override def compute(): Boolean = { - logger.debug(s"compute() file: ${cu.file.name.l.mkString(" ")}") - // Note that we override postSetTypeInformation to iterate over and resolve - // the unresolvedDynamicCalls. - super.compute() - } - - override def visitAssignments(a: OpNodes.Assignment): Set[String] = { - logger.debug(s"""visiting assigment: ${a.name} - |- arguments: ${a.argumentOut.l.mkString(" ")}\n- iteration: ${state.currentIteration}""".stripMargin) - super.visitAssignments(a) - } - - override protected def visitIdentifierAssignedToBlock(i: Identifier, b: Block): Set[String] = { - logger.debug(s"visiting identifier ${i.name} assigned to block ${b.id}") - super.visitIdentifierAssignedToBlock(i, b) - } - - override protected def visitCallAssignedToBlock(c: Call, b: Block): Set[String] = { - logger.debug(s"visiting call ${c.name} assigned to block ${b.id}") - super.visitCallAssignedToBlock(c, b) - } - - override protected def visitStatementsInBlock(b: Block, assignmentTarget: Option[Identifier] = None): Set[String] = { - logger.debug(s"visiting statements in block ${b.id}") - super.visitStatementsInBlock(b, assignmentTarget) - } - - override protected def visitIdentifierAssignedToCall(i: Identifier, c: Call): Set[String] = { - logger.debug(s"visiting identifier ${i.name} assigned to call ${c.name}") - super.visitIdentifierAssignedToCall(i, c) - } - - override protected def visitIdentifierAssignedToIdentifier(x: Identifier, y: Identifier): Set[String] = { - logger.debug(s"visiting identifier ${x.name} assigned to identifier ${y.name}") - super.visitIdentifierAssignedToIdentifier(x, y) - } - - override protected def visitIdentifierAssignedToOperator(i: Identifier, c: Call, operation: String): Set[String] = { - logger.debug(s"visiting identifier ${i.name} assigned to operator ${operation}") - super.visitIdentifierAssignedToOperator(i, c, operation) + /* Register post-processing pass that executes in the super class */ + override protected def postSetTypeInformation(): Unit = { + unresolvedDynamicCalls.foreach(visitUnresolvedDynamicCall) } - override protected def visitIdentifierAssignedToConstructor(i: Identifier, c: Call): Set[String] = { - logger.debug(s"visiting identifier ${i.name} assigned to constructor ${c.name}") val constructorPaths = symbolTable.get(c).map(_.stripSuffix(s"${pathSep}")) associateTypes(i, constructorPaths) } override protected def visitIdentifierAssignedToCallRetVal(i: Identifier, c: Call): Set[String] = { - logger.debug(s"visiting identifier ${i.name} assigned to CallRetVal ${c.name}") if (symbolTable.contains(c)) { - logger.debug(s"- symbol table contains call") val callReturns = methodReturnValues(symbolTable.get(c).toSeq) associateTypes(i, callReturns) } else if (c.argument.exists(_.argumentIndex == 0)) { - logger.debug(s"- argument index exists") val callFullNames = (c.argument(0) match { case i: Identifier if symbolTable.contains(LocalVar(i.name)) => symbolTable.get(LocalVar(i.name)) case i: Identifier if symbolTable.contains(CallAlias(i.name)) => symbolTable.get(CallAlias(i.name)) @@ -136,66 +86,16 @@ private class RecoverForPhpFile(cpg: Cpg, cu: NamespaceBlock, builder: DiffGraph val callReturns = methodReturnValues(callFullNames) associateTypes(i, callReturns) } else { - // Check if the CPG already contains type info for this method, otherwise - // use dummy return value. - logger.debug(s"- checking CPG") + /* CPG may already contain type info for this method (globally, outside of compilation) + * unit. If not, use dummy return value. + */ val rs = methodReturnValues(Seq(c.methodFullName)) if (rs.isEmpty) associateTypes(i, Set(s"${c.name}$pathSep${XTypeRecovery.DummyReturnType}")) else associateTypes(i, rs) } } - override protected def visitIdentifierAssignedToLiteral(i: Identifier, l: Literal): Set[String] = { - logger.debug(s"visiting identifier ${i.name} assigned to literal ${l.code}") - super.visitIdentifierAssignedToLiteral(i, l) - } - - override protected def visitIdentifierAssignedToMethodRef( - i: Identifier, - m: MethodRef, - rec: Option[String] = None - ): Set[String] = { - logger.debug(s"visiting identifier ${i.name} assigned to MethodRef ${m.code}") - super.visitIdentifierAssignedToMethodRef(i, m, rec) - } - - override protected def visitIdentifierAssignedToTypeRef( - i: Identifier, - t: TypeRef, - rec: Option[String] = None - ): Set[String] = { - logger.debug(s"visiting identifier ${i.name} assigned to TypeRef ${t.code}") - super.visitIdentifierAssignedToTypeRef(i, t, rec) - } - - override protected def visitCallAssignedToIdentifier(c: Call, i: Identifier): Set[String] = { - logger.debug(s"visiting call ${c.name} assigned to identifier ${i.name}") - super.visitCallAssignedToIdentifier(c, i) - } - - override protected def visitCallAssignedToCall(x: Call, y: Call): Set[String] = { - logger.debug(s"""visiting call ${x.name} assigned to call ${y.name} - |- ${x.name}: [${getTypesFromCall(y).mkString(", ")}]""".stripMargin) - super.visitCallAssignedToCall(x, y) - } - - override protected def visitCallAssignedToLiteral(c: Call, l: Literal): Set[String] = { - logger.debug(s"visiting call ${c.name} assigned to literal ${l.code}") - super.visitCallAssignedToLiteral(c, l) - } - - override protected def visitCallAssignedToMethodRef(c: Call, m: MethodRef): Set[String] = { - logger.debug(s"visiting call ${c.name} assigned to MethodRef ${m.code}") - super.visitCallAssignedToMethodRef(c, m) - } - - override protected def visitIdentifierAssignedToFieldLoad(i: Identifier, fa: FieldAccess): Set[String] = { - logger.debug(s"visiting field identifier: ${getFieldName(fa)}") - super.visitIdentifierAssignedToFieldLoad(i, fa) - } - override protected def visitReturns(ret: Return): Unit = { - logger.debug(s"visiting return: ${ret.method.name}:(ID${ret.id})") /* A bug in XTypeRecovery mishandles functions that have multiple return * statements. We add a new "symbol table" (methodTypesTable) for method * return types as they get collected across the multiple return statements @@ -207,9 +107,7 @@ private class RecoverForPhpFile(cpg: Cpg, cu: NamespaceBlock, builder: DiffGraph .filterNot(_ == "ANY") .filterNot(_.startsWith(Defines.UnresolvedNamespace)) ) - logger.debug(s"- existing types: ${existingTypes.mkString(", ")}") existingTypes.addAll(methodTypesTable.getOrElse(m, mutable.HashSet())) - logger.debug(s"- existing types + methodTypesTable: ${existingTypes.mkString(", ")}") @tailrec def extractTypes(xs: List[CfgNode]): Set[String] = xs match { @@ -245,22 +143,13 @@ private class RecoverForPhpFile(cpg: Cpg, cu: NamespaceBlock, builder: DiffGraph case _ => Set.empty } val returnTypes = extractTypes(ret.argumentOut.l) - logger.debug(s"- extracted types: ${returnTypes.mkString(", ")}") existingTypes.addAll(returnTypes) - logger.debug(s"- new existing types: ${existingTypes.mkString(", ")}") - // Look up whether the function return is already known (either in the CPG - // or in the methodTypesTable) and if it is known and in our set, then - // remove it from the saveTypes set. + /* Check whether method return is already known, and if so, remove dummy value */ val saveTypes = existingTypes.filterNot(typeName => { if (typeName.startsWith(Defines.UnresolvedNamespace)) true else if (typeName.endsWith(s"${XTypeRecovery.DummyReturnType}")) - // 1. Get methodFullName from typeName - // 2. Check if types for methodFullName are known - // - if they are, check if they are in the existingTypes. - // - if they are, remove this method dummy from the set by returning true - // - else, keep method dummy in the set by returning false. typeName.split(pathSep).headOption match { case Some(methodName) => { val methodReturns = methodReturnValues(Seq(methodName)) @@ -273,15 +162,12 @@ private class RecoverForPhpFile(cpg: Cpg, cu: NamespaceBlock, builder: DiffGraph false }) methodTypesTable.update(m, saveTypes) - logger.debug(s"- saving types: ${saveTypes.mkString(", ")}") builder.setNodeProperty(ret.method.methodReturn, PropertyNames.DYNAMIC_TYPE_HINT_FULL_NAME, saveTypes) } - /** Necessary to change the filter regex from (this|self) to (\\$this|this), in order to account for $this PHP - * convention. - * - * (But need to leave the regular "this"? uncertain) - */ + /* Necessary to change the filter regex from (this|self) to (\\$this|this), in order to account for $this PHP + * convention. + */ override protected def associateTypes(symbol: LocalVar, fa: FieldAccess, types: Set[String]): Set[String] = { fa.astChildren.filterNot(_.code.matches("(\\$this|this|self)")).headOption.collect { case fi: FieldIdentifier => @@ -292,8 +178,7 @@ private class RecoverForPhpFile(cpg: Cpg, cu: NamespaceBlock, builder: DiffGraph symbolTable.append(symbol, types) } - /** Reference the PythonTypeRecovery implementation. The XTypeRecovery one seems incorrect. - */ + /* Reference the PythonTypeRecovery implementation. The XTypeRecovery one seems incorrect. */ override protected def getFieldParents(fa: FieldAccess): Set[String] = { if (fa.method.name == "") { Set(fa.method.fullName) @@ -306,25 +191,11 @@ private class RecoverForPhpFile(cpg: Cpg, cu: NamespaceBlock, builder: DiffGraph } } - override protected def persistMemberWithTypeDecl( - typeFullName: String, - memberName: String, - types: Set[String] - ): Unit = { - logger.debug( - s"persisting member with TypeDecl: typeFullName: ${typeFullName}, memberName: ${memberName}, types: [${types.mkString("; ")}]" - ) - super.persistMemberWithTypeDecl(typeFullName, memberName, types) - } - override protected def getTypesFromCall(c: Call): Set[String] = c.name match { case Operators.fieldAccess => symbolTable.get(LocalVar(getFieldName(new FieldAccess(c)))) case _ if symbolTable.contains(c) => symbolTable.get(c) case Operators.indexAccess => getIndexAccessTypes(c) - case n => { - logger.debug(s"Unknown RHS call type '$n' @ ${c.name}\n- looking up in CPG") - methodReturnValues(Seq(c.methodFullName)) - } + case n => methodReturnValues(Seq(c.methodFullName)) } override protected def indexAccessToCollectionVar(c: Call): Option[CollectionVar] = { @@ -337,8 +208,6 @@ private class RecoverForPhpFile(cpg: Cpg, cu: NamespaceBlock, builder: DiffGraph .getOrElse(XTypeRecovery.DummyIndexAccess) else x.name - logger.debug(s"Index access to collection var\n- call name: ${c.name}") - val collectionVar = Option(c.argumentOut.l match { case List(i: Identifier, idx: Literal) => CollectionVar(i.name, idx.code) case List(i: Identifier, idx: Identifier) => CollectionVar(i.name, idx.code) @@ -349,46 +218,25 @@ private class RecoverForPhpFile(cpg: Cpg, cu: NamespaceBlock, builder: DiffGraph logger.debug(s"Unhandled index access ${xs.map(x => (x.label, x.code)).mkString(",")} @ ${c.name}") null }) - logger.debug(s"- collection var: ${collectionVar.mkString}") collectionVar } - - override protected def storeCallTypeInfo(c: Call, types: Seq[String]): Unit = { - logger.debug(s"storing call type info: ${c.methodFullName}: [${types.mkString(", ")}]") - super.storeCallTypeInfo(c, types) - } - override protected def assignTypesToCall(x: Call, types: Set[String]): Set[String] = { - logger.debug(s"assigning types to call: ${x.name}: [${types.mkString(", ")}]") if (types.nonEmpty) { getSymbolFromCall(x) match { case (lhs, globalKeys) if globalKeys.nonEmpty => { - logger.debug(s"- globalKeys non-empty: lhs: ${lhs.toString()}") globalKeys.foreach { (fieldVar: FieldPath) => - logger.debug( - s"- persisting member with type decl: compUnitFullName: ${fieldVar.compUnitFullName}; identifier: ${fieldVar.identifier}; types: ${types - .mkString(", ")}" - ) persistMemberWithTypeDecl(fieldVar.compUnitFullName, fieldVar.identifier, types) } symbolTable.append(lhs, types) } - case (lhs, _) => { - logger.debug(s"- globalKeys empty: lhs: ${lhs.toString()}") - symbolTable.append(lhs, types) - } + case (lhs, _) => symbolTable.append(lhs, types) } } else Set.empty } - override protected def persistType(x: StoredNode, types: Set[String]): Unit = { - logger.debug(s"persisting type: ${x.label}: [${types.mkString(",")}]") - super.persistType(x, types) - } - override protected def methodReturnValues(methodFullNames: Seq[String]): Set[String] = { - // Look up methods in existing CPG + /* Look up methods in existing CPG */ val rs = cpg.method .fullNameExact(methodFullNames: _*) .methodReturn @@ -397,25 +245,24 @@ private class RecoverForPhpFile(cpg: Cpg, cu: NamespaceBlock, builder: DiffGraph .filterNot(_.endsWith("alloc.")) .filterNot(_.endsWith(s"${XTypeRecovery.DummyReturnType}")) .toSet - logger.debug(s"method return values: ${rs.mkString(",")}") if (rs.isEmpty) - // Return dummy return type if not found. + /* Return dummy return type if not found */ methodFullNames .flatMap(m => Set(m.concat(s"$pathSep${XTypeRecovery.DummyReturnType}"))) .toSet else rs } + /* If we know the type of the method's first parameter, use that to determine the method scope. + * + * TODO: Are there methods / instances where this doesn't work? Static methods? + * TODO: What if the first parameter could take multiple types? + * TODO: Test on nested dynamic calls, e.g. foo->bar->baz() + */ protected def visitUnresolvedDynamicCall(c: Call): Unit = { - // If we know the type of the method's first parameter, use that to - // determine the method scope. - // TODO: Are there methods / instances where this doesn't work? Static methods? - // TODO: What if the first parameter could take multiple types? - // TODO: Test on nested dynamic calls, e.g. foo->bar->baz() if (c.argument.exists(_.argumentIndex == 0)) { - val p = c.argument(0) - p match { + c.argument(0) match { case p: Identifier => { val ts = (p.typeFullName +: p.dynamicTypeHintFullName) .filterNot(_.equals("ANY")) @@ -423,8 +270,6 @@ private class RecoverForPhpFile(cpg: Cpg, cu: NamespaceBlock, builder: DiffGraph ts match { case Seq() => case Seq(t) => { - // Need to update the call node method full name and dynamic type hint full name - // Or maybe just the dynamic type hint full name? val newFullName = t + "->" + c.name builder.setNodeProperty(c, PropertyNames.METHOD_FULL_NAME, newFullName) builder.setNodeProperty( @@ -434,15 +279,11 @@ private class RecoverForPhpFile(cpg: Cpg, cu: NamespaceBlock, builder: DiffGraph ) builder.setNodeProperty(c, PropertyNames.DYNAMIC_TYPE_HINT_FULL_NAME, Seq.empty) } - case _ => { /* TODO */ } + case _ => { /* TODO: case where multiple possible types are identified */ } } } case _ => } } } - - override protected def postSetTypeInformation(): Unit = { - unresolvedDynamicCalls.foreach(visitUnresolvedDynamicCall) - } } diff --git a/joern-cli/frontends/php2cpg/src/test/scala/io/joern/php2cpg/passes/PhpTypeRecoveryPassTests.scala b/joern-cli/frontends/php2cpg/src/test/scala/io/joern/php2cpg/passes/PhpTypeRecoveryPassTests.scala index 108a3f4302c9..c37e25f6e16c 100644 --- a/joern-cli/frontends/php2cpg/src/test/scala/io/joern/php2cpg/passes/PhpTypeRecoveryPassTests.scala +++ b/joern-cli/frontends/php2cpg/src/test/scala/io/joern/php2cpg/passes/PhpTypeRecoveryPassTests.scala @@ -5,6 +5,12 @@ import io.shiftleft.semanticcpg.language._ class PhpTypeRecoveryPassTests extends PhpCode2CpgFixture() { + /* TODO: Future tests to specify correct type recovery behaviors: + * - Method call inherited from a super class should be recovered + * - A type hint on a parameter should be sufficient to resolve method full names at calls + * - Parameter types on builtins with variadic parameters + */ + "literals declared from built-in types" should { lazy val cpg = code(""" |foo" @@ -419,11 +422,4 @@ class PhpTypeRecoveryPassTests extends PhpCode2CpgFixture() { barMethod.methodReturn.dynamicTypeHintFullName shouldBe Seq("int") } } - - /* TODO: remaining tests: - * - Method call inherited from a super class should be recovered - * - A type hint on a parameter should be sufficient to resolve method full names at calls - * - Parameter types on builtins with variadic parameters - */ - -} +} \ No newline at end of file