Fraunhofer-AISEC / cpg

A library to extract Code Property Graphs from C/C++, Java, Go, Python, Ruby and every other language through LLVM-IR.
https://fraunhofer-aisec.github.io/cpg/
Apache License 2.0
248 stars 60 forks source link

DFG Hotfix #250

Closed vfsrfs closed 3 years ago

vfsrfs commented 3 years ago

Fixes multiple issues with incorrect DFG edges.

Graph Changes

None

Interface Changes

None

oxisto commented 3 years ago

Ah, codyze will fail for now because it is not yet on the 3.0.0. branch

vfsrfs commented 3 years ago

Ah, codyze will fail for now because it is not yet on the 3.0.0. branch

Why is this an issue? As far as I know the Master branch is 3.0.0 or am I wrong?

oxisto commented 3 years ago

Ah, codyze will fail for now because it is not yet on the 3.0.0. branch

Why is this an issue? As far as I know the Master branch is 3.0.0 or am I wrong?

I was referring to the fact that Codyze still uses pre-3.0.0 before the node structuring, that is why the external codyze check fails.

konradweiss commented 3 years ago

Ah, codyze will fail for now because it is not yet on the 3.0.0. branch

Why is this an issue? As far as I know the Master branch is 3.0.0 or am I wrong?

I was referring to the fact that Codyze still uses pre-3.0.0 before the node structuring, that is why the external codyze check fails.

Why do we have a check codyze test in our CI? The codyze is supposed to fail when we make bigger changes, such a test should not be done in CPG, adaptations to the CPG in Codyze should happen on releases and not before merging a PR in the CPG.

konradweiss commented 3 years ago

@vfsrfs Looks like most is fixed. We still have cyles when we use these self referencing RW references as a--. Currently the the Graph there looks like this:
cylceMinusA we have to remove these cycles, so either we directly draw the edge from such operators like '--' '/=' '+=' etc. to the Declaration instead of going over the referenz. These changes would have to be done in the AST construction and not the DFG-refinement. Lastly there is the Task to create a new Uninitialized node for the case where we have 'int a;' with the purpose to newer have to draw DFGs from the declaration itself, as the declaration also has incoming edges from every write and this would create cylces.

vfsrfs commented 3 years ago

@vfsrfs Looks like most is fixed. We still have cyles when we use these self referencing RW references as a--. Currently the the Graph there looks like this: cylceMinusA we have to remove these cycles, so either we directly draw the edge from such operators like '--' '/=' '+=' etc. to the Declaration instead of going over the referenz. These changes would have to be done in the AST construction and not the DFG-refinement. Lastly there is the Task to create a new Uninitialized node for the case where we have 'int a;' with the purpose to newer have to draw DFGs from the declaration itself, as the declaration also has incoming edges from every write and this would create cylces.

Thanks for the review 👍. I will fix the cycles caused by the unary and compound operator and add the uninitialized node.

vfsrfs commented 3 years ago

Ah, codyze will fail for now because it is not yet on the 3.0.0. branch

Why is this an issue? As far as I know the Master branch is 3.0.0 or am I wrong?

I was referring to the fact that Codyze still uses pre-3.0.0 before the node structuring, that is why the external codyze check fails.

Why do we have a check codyze test in our CI? The codyze is supposed to fail when we make bigger changes, such a test should not be done in CPG, adaptations to the CPG in Codyze should happen on releases and not before merging a PR in the CPG.

I agree with @konradweiss, every CPG PR will fail until codyze is updated, which means that we will have a delay for adding new features to the CPG.

oxisto commented 3 years ago

Ah, codyze will fail for now because it is not yet on the 3.0.0. branch

Why is this an issue? As far as I know the Master branch is 3.0.0 or am I wrong?

I was referring to the fact that Codyze still uses pre-3.0.0 before the node structuring, that is why the external codyze check fails.

Why do we have a check codyze test in our CI? The codyze is supposed to fail when we make bigger changes, such a test should not be done in CPG, adaptations to the CPG in Codyze should happen on releases and not before merging a PR in the CPG.

I agree with @konradweiss, every CPG PR will fail until codyze is updated, which means that we will have a delay for adding new features to the CPG.

It is not mandatory, you can include the check with a label. It is more a sanity check, if this changes anything on Codze. Maybe there is a way to not have it as an "error", rather a warning

It was introduced here: https://github.com/Fraunhofer-AISEC/cpg/pull/221

sonarcloud[bot] commented 3 years ago

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

96.5% 96.5% Coverage
0.0% 0.0% Duplication

vfsrfs commented 3 years ago

Adds an UninitialezedValue initializer to VariableDeclarations without initialised to allow for propagation in the dfg pass. Fixes multiple issues with incorrect DFG edges.

Graph Changes

Adds an UninitializedValue node to the graph with ingoing and outgoing EOG edges, an ingoing initializer edge and an outgoing dfg edge from/to the corresponding VariableDeclaration.

Interface Changes

None