Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle InitializerListExpressions as variables of ComprehensionExpressions in the DFG #2016

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Original file line number Diff line number Diff line change
Expand Up @@ -124,22 +124,22 @@ interface HasInitializer : HasScope, HasType, ArgumentHolder, AssignmentHolder {

/**
* Some nodes have aliases, i.e., it potentially references another variable. This means that
* writing to this node, also writes to its [aliases] and vice-versa.
* writing to this node, also writes to its [aliases] and vice versa.
*/
interface HasAliases : HasScope {
/** The aliases which this node has. */
var aliases: MutableSet<HasAliases>
}

/**
* Specifies that this node (e.g. a [BinaryOperator] contains an operation that can be overloaded by
* an [OperatorDeclaration].
* Specifies that this node (e.g. a [BinaryOperator]) contains an operation that can be overloaded
* by an [OperatorDeclaration].
*/
interface HasOverloadedOperation : HasOperatorCode {

/**
* Arguments forwarded to the operator. This might not necessarily be all of the regular
* "arguments", since often the the first argument is part of the [operatorBase].
* Arguments forwarded to the operator. These might not necessarily be all regular "arguments",
* since often, the first argument is part of the [operatorBase].
*/
val operatorArguments: List<Expression>

Expand All @@ -149,3 +149,14 @@ interface HasOverloadedOperation : HasOperatorCode {
*/
val operatorBase: Expression
}

/**
* Specifies that this node (e.g., a [Reference]) can be used to read data from or write data to it.
* It adds the property [access] which allows to determine the direction of data flow edges.
*/
interface HasAccess {
KuechA marked this conversation as resolved.
Show resolved Hide resolved
/**
* Is this node used for writing data instead of just reading it? Determines dataflow direction
*/
var access: AccessValues
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought. Wouldn't it make sense to just implements this on all Expression nodes? The memory overhead is probably negligible and we would remove a lot of ugly as? HasAccess casts. Basically this should apply to all expressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided against this because many Expressions cannot receive any data (i.e., AccessValues.WRITE never makes sense for them and for some, also READ might be confusing), so the access value is not useful there. Are the casts are a major concern? Any opinions on this @konradweiss ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But all of them (most?) are part of the DFG and it is interesting to have a harmonized way to access this information even for a lot of them it will statically stay with READ.

}
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import de.fraunhofer.aisec.cpg.graph.edges.ast.AstEdges
import de.fraunhofer.aisec.cpg.graph.edges.ast.astEdgesOf
import de.fraunhofer.aisec.cpg.graph.edges.ast.astOptionalEdgeOf
import de.fraunhofer.aisec.cpg.graph.edges.unwrapping
import de.fraunhofer.aisec.cpg.graph.statements.expressions.Reference
import java.util.Objects
import org.apache.commons.lang3.builder.ToStringBuilder
import org.neo4j.ogm.annotation.Relationship
Expand All @@ -45,12 +44,7 @@ class ForEachStatement : LoopStatement(), BranchingNode, StatementHolder {
@Relationship("VARIABLE")
var variableEdge =
astOptionalEdgeOf<Statement>(
onChanged = { _, new ->
val end = new?.end
if (end is Reference) {
end.access = AccessValues.WRITE
}
}
onChanged = { _, new -> (new?.end as? HasAccess)?.access = AccessValues.WRITE }
)

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,12 @@ class AssignExpression :
base = base.base as? MemberExpression
}

if (isSimpleAssignment) {
(end as? Reference)?.access = AccessValues.WRITE
} else {
(end as? Reference)?.access = AccessValues.READWRITE
}
(end as? HasAccess)?.access =
if (isSimpleAssignment) {
AccessValues.WRITE
} else {
AccessValues.READWRITE
}
}
)
var lhs by unwrapping(AssignExpression::lhsEdges)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import java.util.*
import org.neo4j.ogm.annotation.Relationship
import org.slf4j.LoggerFactory

class CastExpression : Expression(), ArgumentHolder, HasType.TypeObserver {
class CastExpression : Expression(), ArgumentHolder, HasType.TypeObserver, HasAccess {
/**
* The [Expression] that is cast to [castType].
*
Expand Down Expand Up @@ -76,11 +76,13 @@ class CastExpression : Expression(), ArgumentHolder, HasType.TypeObserver {

override fun addArgument(expression: Expression) {
this.expression = expression
(this.expression as? HasAccess)?.access = access
}

override fun replaceArgument(old: Expression, new: Expression): Boolean {
if (this.expression == old) {
this.expression = new
(this.expression as? HasAccess)?.access = access
return true
}

Expand Down Expand Up @@ -114,6 +116,12 @@ class CastExpression : Expression(), ArgumentHolder, HasType.TypeObserver {

override fun hashCode() = Objects.hash(super.hashCode(), expression, castType)

override var access = AccessValues.READ
set(value) {
field = value
(this.expression as? HasAccess)?.access = value
}

companion object {
private val log = LoggerFactory.getLogger(CastExpression::class.java)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ package de.fraunhofer.aisec.cpg.graph.statements.expressions

import de.fraunhofer.aisec.cpg.graph.AccessValues
import de.fraunhofer.aisec.cpg.graph.ArgumentHolder
import de.fraunhofer.aisec.cpg.graph.HasAccess
import de.fraunhofer.aisec.cpg.graph.edges.ast.astEdgeOf
import de.fraunhofer.aisec.cpg.graph.edges.ast.astOptionalEdgeOf
import de.fraunhofer.aisec.cpg.graph.edges.unwrapping
Expand All @@ -41,12 +42,7 @@ class ComprehensionExpression : Expression(), ArgumentHolder {
var variableEdge =
astEdgeOf<Statement>(
of = ProblemExpression("Missing variableEdge in ${this::class}"),
onChanged = { _, new ->
val end = new?.end
if (end is Reference) {
end.access = AccessValues.WRITE
}
},
onChanged = { _, new -> (new?.end as? HasAccess)?.access = AccessValues.WRITE },
)

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,17 @@ import org.neo4j.ogm.annotation.Relationship
* be set before adding any values to [InitializerListExpression.initializers].
*/
// TODO Merge and/or refactor
class InitializerListExpression : Expression(), ArgumentHolder, HasType.TypeObserver {
class InitializerListExpression : Expression(), ArgumentHolder, HasType.TypeObserver, HasAccess {

/** The list of initializers. */
@Relationship(value = "INITIALIZERS", direction = Relationship.Direction.OUTGOING)
var initializerEdges =
astEdgesOf<Expression>(onAdd = { it.end.registerTypeObserver(this) }) {
astEdgesOf<Expression>(
onAdd = {
it.end.registerTypeObserver(this)
(it.end as? HasAccess)?.access = this.access
}
) {
it.end.unregisterTypeObserver(this)
}

Expand All @@ -63,6 +69,7 @@ class InitializerListExpression : Expression(), ArgumentHolder, HasType.TypeObse

override fun addArgument(expression: Expression) {
this.initializers += expression
(expression as? HasAccess)?.access = this.access
}

override fun replaceArgument(old: Expression, new: Expression): Boolean {
Expand All @@ -73,6 +80,7 @@ class InitializerListExpression : Expression(), ArgumentHolder, HasType.TypeObse
new.registerTypeObserver(this)
return true
}
(new as? HasAccess)?.access = this.access

return false
}
Expand Down Expand Up @@ -120,4 +128,10 @@ class InitializerListExpression : Expression(), ArgumentHolder, HasType.TypeObse
// unique to avoid too many hash collisions.
return Objects.hash(super.hashCode(), initializerEdges.size)
}

override var access = AccessValues.READ
set(value) {
field = value
initializers.forEach { (it as? HasAccess)?.access = value }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ package de.fraunhofer.aisec.cpg.graph.statements.expressions

import de.fraunhofer.aisec.cpg.PopulatedByPass
import de.fraunhofer.aisec.cpg.graph.AccessValues
import de.fraunhofer.aisec.cpg.graph.HasAccess
import de.fraunhofer.aisec.cpg.graph.HasAliases
import de.fraunhofer.aisec.cpg.graph.Node
import de.fraunhofer.aisec.cpg.graph.declarations.Declaration
Expand All @@ -44,7 +45,7 @@ import org.neo4j.ogm.annotation.Relationship
* expression `a = b`, which itself is an [AssignExpression], contains two [Reference]s, one for the
* variable `a` and one for variable `b`, which have been previously been declared.
*/
open class Reference : Expression(), HasType.TypeObserver, HasAliases {
open class Reference : Expression(), HasType.TypeObserver, HasAliases, HasAccess {
/**
* The [Declaration]s this expression might refer to. This will influence the [type] of this
* expression.
Expand Down Expand Up @@ -89,11 +90,7 @@ open class Reference : Expression(), HasType.TypeObserver, HasAliases {

override var aliases = mutableSetOf<HasAliases>()

/**
* Is this reference used for writing data instead of just reading it? Determines dataflow
* direction
*/
var access = AccessValues.READ
override var access = AccessValues.READ
var isStaticAccess = false

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,14 @@ import org.neo4j.ogm.annotation.Relationship
* ([arrayExpression]) and `index` ([subscriptExpression]) are of type [Expression]. CPP can
* overload operators thus changing semantics of array access.
*/
class SubscriptExpression : Expression(), HasBase, HasType.TypeObserver, ArgumentHolder {
class SubscriptExpression : Expression(), HasBase, HasType.TypeObserver, ArgumentHolder, HasAccess {
override var access = AccessValues.READ
set(value) {
field = value
// Propagate the access value to the array expression
(arrayExpression as? HasAccess)?.access = value
}

@Relationship("ARRAY_EXPRESSION")
var arrayExpressionEdge =
astEdgeOf<Expression>(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ package de.fraunhofer.aisec.cpg.graph.statements.expressions

import de.fraunhofer.aisec.cpg.graph.AccessValues
import de.fraunhofer.aisec.cpg.graph.ArgumentHolder
import de.fraunhofer.aisec.cpg.graph.HasAccess
import de.fraunhofer.aisec.cpg.graph.HasOverloadedOperation
import de.fraunhofer.aisec.cpg.graph.edges.ast.astEdgeOf
import de.fraunhofer.aisec.cpg.graph.edges.unwrapping
Expand All @@ -37,7 +38,8 @@ import org.apache.commons.lang3.builder.ToStringBuilder
import org.neo4j.ogm.annotation.Relationship

/** A unary operator expression, involving one expression and an operator, such as `a++`. */
class UnaryOperator : Expression(), HasOverloadedOperation, ArgumentHolder, HasType.TypeObserver {
class UnaryOperator :
Expression(), HasOverloadedOperation, ArgumentHolder, HasType.TypeObserver, HasAccess {
@Relationship("INPUT")
var inputEdge =
astEdgeOf<Expression>(
Expand Down Expand Up @@ -74,13 +76,13 @@ class UnaryOperator : Expression(), HasOverloadedOperation, ArgumentHolder, HasT
var isPrefix = false

private fun changeExpressionAccess() {
var access = AccessValues.READ
if (operatorCode == "++" || operatorCode == "--") {
access = AccessValues.READWRITE
}
if (input is Reference) {
(input as? Reference)?.access = access
}
var access =
if (operatorCode == "++" || operatorCode == "--") {
AccessValues.READWRITE
} else {
this.access
}
(input as? HasAccess)?.access = access
}

override fun toString(): String {
Expand Down Expand Up @@ -130,11 +132,13 @@ class UnaryOperator : Expression(), HasOverloadedOperation, ArgumentHolder, HasT

override fun addArgument(expression: Expression) {
this.input = expression
(this.input as? HasAccess)?.access = access
}

override fun replaceArgument(old: Expression, new: Expression): Boolean {
if (this.input == old) {
this.input = new
(this.input as? HasAccess)?.access = access
return true
}

Expand All @@ -161,6 +165,12 @@ class UnaryOperator : Expression(), HasOverloadedOperation, ArgumentHolder, HasT

override fun hashCode() = super.hashCode()

override var access = AccessValues.READ
set(value) {
field = value
(this.input as? HasAccess)?.access = value
}

companion object {
const val OPERATOR_POSTFIX_INCREMENT = "++"
const val OPERATOR_POSTFIX_DECREMENT = "--"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
package de.fraunhofer.aisec.cpg.graph.types

import de.fraunhofer.aisec.cpg.graph.ContextProvider
import de.fraunhofer.aisec.cpg.graph.HasAccess
import de.fraunhofer.aisec.cpg.graph.LanguageProvider
import de.fraunhofer.aisec.cpg.graph.Node
import de.fraunhofer.aisec.cpg.graph.declarations.ValueDeclaration
Expand Down Expand Up @@ -140,6 +141,9 @@ interface HasType : ContextProvider, LanguageProvider {
) {
(old?.end as? HasType)?.unregisterTypeObserver(this)
(new?.end as? HasType)?.registerTypeObserver(this)
if (this is HasAccess) {
(new?.end as? HasAccess)?.access = this.access
}
}
}

Expand Down
Loading
Loading