Skip to content

Commit

Permalink
[jssrc2cpg] Fix race condition for registering types (joernio#3900)
Browse files Browse the repository at this point in the history
This is for: joernio#3869

(Also applied the same for swiftsrc2cpg with this PR as it uses mostly the same mechanics)
  • Loading branch information
max-leuthaeuser authored Dec 6, 2023
1 parent 0b7376e commit 7682b74
Show file tree
Hide file tree
Showing 29 changed files with 156 additions and 190 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package io.joern.jssrc2cpg
import better.files.File
import io.joern.dataflowengineoss.layers.dataflows.{OssDataFlow, OssDataFlowOptions}
import io.joern.jssrc2cpg.JsSrc2Cpg.postProcessingPasses
import io.joern.jssrc2cpg.datastructures.JsGlobal
import io.joern.jssrc2cpg.passes.*
import io.joern.jssrc2cpg.utils.AstGenRunner
import io.joern.x2cpg.X2Cpg.withNewEmptyCpg
Expand All @@ -29,9 +30,8 @@ class JsSrc2Cpg extends X2CpgFrontend[Config] {
val astCreationPass = new AstCreationPass(cpg, astGenResult, config, report)(config.schemaValidation)
astCreationPass.createAndApply()

new TypeNodePass(astCreationPass.allUsedTypes(), cpg).createAndApply()
new JsMetaDataPass(cpg, hash, config.inputPath).createAndApply()
new BuiltinTypesPass(cpg).createAndApply()
JavaScriptTypeNodePass.withRegisteredTypes(JsGlobal.typesSeen(), cpg).createAndApply()
new JavaScriptMetaDataPass(cpg, hash, config.inputPath).createAndApply()
new DependenciesPass(cpg, config).createAndApply()
new ConfigPass(cpg, config, report).createAndApply()
new PrivateKeyFilePass(cpg, config, report).createAndApply()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,9 @@ import org.slf4j.{Logger, LoggerFactory}
import overflowdb.BatchedUpdate.DiffGraphBuilder
import ujson.Value

import java.util.concurrent.ConcurrentHashMap
import scala.collection.mutable

class AstCreator(
val config: Config,
val parserResult: ParseResult,
val usedTypes: ConcurrentHashMap[(String, String), Boolean]
)(implicit withSchemaValidation: ValidationMode)
class AstCreator(val config: Config, val parserResult: ParseResult)(implicit withSchemaValidation: ValidationMode)
extends AstCreatorBase(parserResult.filename)
with AstForExpressionsCreator
with AstForPrimitivesCreator
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import io.shiftleft.codepropertygraph.generated.{EdgeTypes, EvaluationStrategies
import ujson.Value

import scala.collection.{SortedMap, mutable}
import scala.jdk.CollectionConverters.EnumerationHasAsScala
import scala.util.{Success, Try}

trait AstCreatorHelper(implicit withSchemaValidation: ValidationMode) { this: AstCreator =>
Expand All @@ -38,13 +37,8 @@ trait AstCreatorHelper(implicit withSchemaValidation: ValidationMode) { this: As
Ast(unknownNode(node, node.code))
}

protected def registerType(typeName: String, typeFullName: String): Unit = {
if (usedTypes.containsKey((typeName, typeName)) && typeName != typeFullName) {
usedTypes.put((typeName, typeFullName), true)
usedTypes.remove((typeName, typeName))
} else if (!usedTypes.keys().asScala.exists { case (tpn, _) => tpn == typeName }) {
usedTypes.putIfAbsent((typeName, typeFullName), true)
}
protected def registerType(typeFullName: String): Unit = {
JsGlobal.usedTypes.putIfAbsent(typeFullName, true)
}

private def nodeType(node: Value): BabelNode = fromString(node("type").str)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ trait AstForTypesCreator(implicit withSchemaValidation: ValidationMode) { this:
} else {
typeFor(createBabelNodeInfo(alias.json))
}
registerType(aliasName, aliasFullName)
registerType(aliasFullName)

val astParentType = methodAstParentStack.head.label
val astParentFullName = methodAstParentStack.head.properties("FULL_NAME").toString
Expand All @@ -43,7 +43,7 @@ trait AstForTypesCreator(implicit withSchemaValidation: ValidationMode) { this:
astParentFullName,
alias = Option(aliasFullName)
)
registerType(typeName, typeFullName)
registerType(typeFullName)
diffGraph.addEdge(methodAstParentStack.head, typeDeclNode_, EdgeTypes.AST)
} else {
seenAliasTypes.find(t => t.name == name).foreach(_.aliasTypeFullName(aliasFullName))
Expand Down Expand Up @@ -217,7 +217,7 @@ trait AstForTypesCreator(implicit withSchemaValidation: ValidationMode) { this:

protected def astForEnum(tsEnum: BabelNodeInfo): Ast = {
val (typeName, typeFullName) = calcTypeNameAndFullName(tsEnum)
registerType(typeName, typeFullName)
registerType(typeFullName)

val astParentType = methodAstParentStack.head.label
val astParentFullName = methodAstParentStack.head.properties("FULL_NAME").toString
Expand Down Expand Up @@ -299,7 +299,7 @@ trait AstForTypesCreator(implicit withSchemaValidation: ValidationMode) { this:

protected def astForClass(clazz: BabelNodeInfo, shouldCreateAssignmentCall: Boolean = false): Ast = {
val (typeName, typeFullName) = calcTypeNameAndFullName(clazz)
registerType(typeName, typeFullName)
registerType(typeFullName)

val astParentType = methodAstParentStack.head.label
val astParentFullName = methodAstParentStack.head.properties("FULL_NAME").toString
Expand Down Expand Up @@ -452,7 +452,7 @@ trait AstForTypesCreator(implicit withSchemaValidation: ValidationMode) { this:

protected def astForInterface(tsInterface: BabelNodeInfo): Ast = {
val (typeName, typeFullName) = calcTypeNameAndFullName(tsInterface)
registerType(typeName, typeFullName)
registerType(typeFullName)

val astParentType = methodAstParentStack.head.label
val astParentFullName = methodAstParentStack.head.properties("FULL_NAME").toString
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ trait AstNodeBuilder(implicit withSchemaValidation: ValidationMode) { this: AstC
methodFullName: String,
filename: String
): Ast = {
registerType(methodName, methodFullName)
registerType(methodFullName)

val astParentType = parentNode.label
val astParentFullName = parentNode.properties("FULL_NAME").toString
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ trait TypeHelper { this: AstCreator =>
case Some(key) => typeForTypeAnnotation(createBabelNodeInfo(node.json(key)))
case None => typeFromTypeMap(node)
}
registerType(tpe, tpe)
registerType(tpe)
tpe
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package io.joern.jssrc2cpg.datastructures

import io.joern.jssrc2cpg.passes.Defines
import io.joern.x2cpg.datastructures.Global

import scala.jdk.CollectionConverters._

object JsGlobal extends Global {

def typesSeen(): List[String] = {
val types = usedTypes.keys().asScala.filterNot(_ == Defines.Any).toList
usedTypes.clear()
types
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ import io.shiftleft.utils.IOUtils
import org.slf4j.{Logger, LoggerFactory}

import java.nio.file.Paths
import java.util.concurrent.ConcurrentHashMap
import scala.jdk.CollectionConverters.EnumerationHasAsScala
import scala.util.{Failure, Success, Try}

class AstCreationPass(cpg: Cpg, astGenRunnerResult: AstGenRunnerResult, config: Config, report: Report = new Report())(
Expand All @@ -22,13 +20,8 @@ class AstCreationPass(cpg: Cpg, astGenRunnerResult: AstGenRunnerResult, config:

private val logger: Logger = LoggerFactory.getLogger(classOf[AstCreationPass])

private val usedTypes: ConcurrentHashMap[(String, String), Boolean] = new ConcurrentHashMap()

override def generateParts(): Array[(String, String)] = astGenRunnerResult.parsedFiles.toArray

def allUsedTypes(): List[(String, String)] =
usedTypes.keys().asScala.filterNot { case (typeName, _) => typeName == Defines.Any }.toList

override def finish(): Unit = {
astGenRunnerResult.skippedFiles.foreach { skippedFile =>
val (rootPath, fileName) = skippedFile
Expand All @@ -45,7 +38,7 @@ class AstCreationPass(cpg: Cpg, astGenRunnerResult: AstGenRunnerResult, config:
val fileLOC = IOUtils.readLinesInFile(Paths.get(parseResult.fullPath)).size
report.addReportInfo(parseResult.filename, fileLOC, parsed = true)
Try {
val localDiff = new AstCreator(config, parseResult, usedTypes).createAst()
val localDiff = new AstCreator(config, parseResult).createAst()
diffGraph.absorb(localDiff)
} match {
case Failure(exception) =>
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import io.shiftleft.codepropertygraph.generated.nodes.NewMetaData
import io.shiftleft.codepropertygraph.generated.Languages
import io.shiftleft.passes.CpgPass

class JsMetaDataPass(cpg: Cpg, hash: String, inputPath: String) extends CpgPass(cpg) {
class JavaScriptMetaDataPass(cpg: Cpg, hash: String, inputPath: String) extends CpgPass(cpg) {

override def run(diffGraph: DiffGraphBuilder): Unit = {
val absolutePathToRoot = File(inputPath).path.toAbsolutePath.toString
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package io.joern.jssrc2cpg.passes

import io.joern.x2cpg.passes.frontend.TypeNodePass
import io.shiftleft.codepropertygraph.Cpg
import io.shiftleft.semanticcpg.language._
import io.shiftleft.passes.KeyPool

import scala.collection.mutable

object JavaScriptTypeNodePass {

def withRegisteredTypes(registeredTypes: List[String], cpg: Cpg, keyPool: Option[KeyPool] = None): TypeNodePass = {
new TypeNodePass(registeredTypes, cpg, keyPool, getTypesFromCpg = false) {

override protected def typeDeclTypes: mutable.Set[String] = {
// The only difference to the default implementation in TypeNodePass.typeDeclTypes is the following:
// We do not want to add types for types being inherited as this is already handled by the JavaScriptInheritanceNamePass.
val typeDeclTypes = mutable.Set[String]()
cpg.typeDecl.foreach { typeDecl =>
typeDeclTypes += typeDecl.fullName
}
typeDeclTypes
}

}
}

}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package io.joern.jssrc2cpg.passes
import better.files.File
import io.joern.jssrc2cpg.utils.AstGenRunner
import io.joern.jssrc2cpg.Config
import io.joern.jssrc2cpg.datastructures.JsGlobal
import io.joern.x2cpg.ValidationMode
import io.joern.x2cpg.X2Cpg.newEmptyCpg
import io.shiftleft.codepropertygraph.Cpg
Expand All @@ -25,6 +26,7 @@ abstract class AbstractPassTest extends AnyWordSpec with Matchers with Inside {
val config = Config(tsTypes = tsTypes).withInputPath(dir.toString).withOutputPath(dir.toString)
val astGenResult = new AstGenRunner(config).execute(dir)
new AstCreationPass(cpg, astGenResult, config).createAndApply()
JavaScriptTypeNodePass.withRegisteredTypes(JsGlobal.typesSeen(), cpg).createAndApply()
f(cpg)
file.delete()
}
Expand All @@ -41,8 +43,7 @@ abstract class AbstractPassTest extends AnyWordSpec with Matchers with Inside {
val astGenResult = new AstGenRunner(config).execute(dir)
val astCreationPass = new AstCreationPass(cpg, astGenResult, config)
astCreationPass.createAndApply()
new TypeNodePass(astCreationPass.allUsedTypes(), cpg).createAndApply()
new BuiltinTypesPass(cpg).createAndApply()
JavaScriptTypeNodePass.withRegisteredTypes(JsGlobal.typesSeen(), cpg).createAndApply()
f(cpg)
file.delete()
}
Expand All @@ -61,8 +62,7 @@ abstract class AbstractPassTest extends AnyWordSpec with Matchers with Inside {
val astGenResult = new AstGenRunner(config).execute(dir)
val astCreationPass = new AstCreationPass(cpg, astGenResult, config)
astCreationPass.createAndApply()
new TypeNodePass(astCreationPass.allUsedTypes(), cpg).createAndApply()
new BuiltinTypesPass(cpg).createAndApply()
JavaScriptTypeNodePass.withRegisteredTypes(JsGlobal.typesSeen(), cpg).createAndApply()
f(cpg)
file1.delete()
file2.delete()
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ class JsMetaDataPassTest extends AbstractPassTest {

"MetaDataPass" should {
val cpg = Cpg.emptyCpg
new JsMetaDataPass(cpg, "somehash", "").createAndApply()
new JavaScriptMetaDataPass(cpg, "somehash", "").createAndApply()

"create exactly 1 node" in {
cpg.graph.V.asScala.size shouldBe 1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,26 @@ class TSTypesTest extends AbstractPassTest {
p2.typeFullName shouldBe Defines.String
val List(barRet) = cpg.method("bar").methodReturn.l
barRet.typeFullName shouldBe "Foo"
cpg.typ.name.sorted.l shouldBe (List(
cpg.typ.name.sorted.l shouldBe List(
":program",
io.joern.x2cpg.Defines.ConstructorMethodName,
"ANY",
"Foo",
"Foo",
"Number",
"String",
"bar"
) ++ Defines.JsTypes).sorted
).sorted
cpg.typ.fullName.sorted.l shouldBe List(
"ANY",
"Foo",
"__ecma.Number",
"__ecma.String",
"code.ts::program",
"code.ts::program:Foo",
"code.ts::program:Foo:<init>",
"code.ts::program:bar"
).sorted
}

"have correct types for variables" in TsAstFixture(
Expand Down Expand Up @@ -248,7 +262,7 @@ class TSTypesTest extends AbstractPassTest {
tsTypes = true
) { cpg =>
cpg.typeDecl("string").l shouldBe empty
cpg.typeDecl(Defines.String).size shouldBe 1
cpg.typ.fullName(Defines.String).size shouldBe 1
inside(cpg.typeDecl("Alias").l) { case List(alias) =>
alias.fullName shouldBe "code.ts::program:Alias"
alias.code shouldBe "type Alias = string"
Expand Down
Loading

0 comments on commit 7682b74

Please sign in to comment.