Errors during createAndApply of ParallelCpgPass not escalated about codepropertygraph

[mpollmeier]

This one slipped between the cracks - sorry. I'll take a look.

[mpollmeier]

I added a test for this in #1577, but didn't need to change anything in the implementation... as you would expect, createAndApply does throw an exception.

The companion repository above doesn't contain any code, maybe you can provide a working example there?

[simkoc]

[simon@roxanne tmp]$ git clone [email protected]:simkoc/cpg-bug-poc.git
Cloning into 'cpg-bug-poc'...
remote: Enumerating objects: 131, done.
remote: Counting objects: 100% (131/131), done.
remote: Compressing objects: 100% (58/58), done.
remote: Total 131 (delta 26), reused 116 (delta 19), pack-reused 0
Receiving objects: 100% (131/131), 16.10 KiB | 246.00 KiB/s, done.
Resolving deltas: 100% (26/26), done.
[simon@roxanne tmp]$ cd cpg-bug-poc/
[simon@roxanne cpg-bug-poc]$ git checkout issue2
Branch 'issue2' set up to track remote branch 'issue2' from 'origin'.
Switched to a new branch 'issue2'
[simon@roxanne cpg-bug-poc]$ sbt test
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by sbt.TrapExit$ (file:/home/simon/.sbt/boot/scala-2.12.14/org.scala-sbt/sbt/1.5.7/run_2.12-1.5.7.jar)
WARNING: Please consider reporting this to the maintainers of sbt.TrapExit$
WARNING: System::setSecurityManager will be removed in a future release
[info] welcome to sbt 1.5.7 (N/A Java 17.0.1)
[info] loading settings for project global-plugins from gpg.sbt,release.sbt,sonatype.sbt ...
[info] loading global plugins from /home/simon/.sbt/1.0/plugins
[info] loading settings for project cpg-bug-poc-build from plugins.sbt ...
[info] loading project definition from /home/simon/tmp/cpg-bug-poc/project
[info] loading settings for project cpg-bug-poc from build.sbt ...
[info] set current project to cpg bug poc (in build file:/home/simon/tmp/cpg-bug-poc/)
[info] compiling 1 Scala source to /home/simon/tmp/cpg-bug-poc/target/scala-2.13/classes ...
[info] compiling 1 Scala source to /home/simon/tmp/cpg-bug-poc/target/scala-2.13/test-classes ...
Exception in thread "Writer" java.lang.RuntimeException: Edge with type='POST_DOMINATE' with direction='OUT' not supported by nodeType='METHOD'
  | => cat overflowdb.NodeDb.storeAdjacentNode(NodeDb.java:637)
        at overflowdb.NodeDb.storeAdjacentNode(NodeDb.java:616)
	at overflowdb.NodeDb.addEdge(NodeDb.java:303)
	at overflowdb.NodeRef.addEdge(NodeRef.java:160)
	at overflowdb.SemiEdge.$minus$minus$greater(SyntacticSugar.scala:59)
	at io.shiftleft.passes.DiffGraph$Applier.odbAddEdge(DiffGraph.scala:411)
	at io.shiftleft.passes.DiffGraph$Applier.addEdge(DiffGraph.scala:403)
	at io.shiftleft.passes.DiffGraph$Applier.$anonfun$run$1(DiffGraph.scala:355)
	at io.shiftleft.passes.DiffGraph$Applier.$anonfun$run$1$adapted(DiffGraph.scala:351)
	at scala.collection.IterableOnceOps.foreach(IterableOnce.scala:553)
	at scala.collection.IterableOnceOps.foreach$(IterableOnce.scala:551)
	at scala.collection.AbstractIterator.foreach(Iterator.scala:1279)
	at io.shiftleft.passes.DiffGraph$Applier.run(DiffGraph.scala:351)
	at io.shiftleft.passes.DiffGraph$Applier$.applyDiff(DiffGraph.scala:438)
	at io.shiftleft.passes.ParallelCpgPass$Writer.run(ParallelCpgPass.scala:116)
	at java.base/java.lang.Thread.run(Thread.java:833)
[info] TestForBug:
[info] the cpg creation test
[info] - should fail
[info] Run completed in 443 milliseconds.
[info] Total number of tests run: 1
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 1, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 4 s, completed Jan 14, 2022, 2:56:05 PM

Did you checkout issue2 as linked? Above is the complete workflow with a passing unit test even though there is an error. This error will not be recognized during CI execution as the test suite says that every test succeeded. I would expect the unit test to fail.

[mpollmeier]

Ah, yes I didn't notice that this was in a branch, thanks.
I can confirm the issue now, and it's actually in the ParallelCpgPass.

[mpollmeier]

@bbrehm can I ask for your opinion on how to best handle (partial) failure in the ParallelCpgPass?

I added a test in https://github.com/ShiftLeftSecurity/codepropertygraph/tree/michael/parallel-cpg-pass-fail, but there are of course other options and questions.

E.g. should we continue running all other parts on failure? I guess so, but then if we want to throw an exception we need to wrap it in some kind of wrapper exception that contains all others.
Alternatively we could return a Seq[Try] or similar here, rather than Unit - that would be the cleaner and more composable way IMO, but you may have different ideas.

[simkoc]

If I may add my two cents. Simply returning a List[(Part,Error)] or in the spirit thereof by createAndApply would suffice. This way I would be able to check if the list is empty (complete success), whether an error occurred (non empty) and if so during which part. A method of the ParallelCpgPass doing this would be fine as well.

[bbrehm]

Exceptions during diffgraph creation:

Generally, I'd say that we shouldn't try to handle this at all -- it is the job of the pass author to deal with exceptions they raise, i.e. enclose their computation in a try-catch-finally block if required.

The legacy ParallelCpgPass interface and the newer ConcurrentWriterCpgPass are fundamentally incapable of sane exception handling: In these cases, changes to the graph are written concurrently with the generation of changes. Hence, the graph is already modified in the moment that the exception is thrown. The newer ConcurrentWriterCpgPass is at least deterministic, unless users introduce race conditions -- ParallelCpgPass is racy by design.

Whether the legacy CpgPass can handle exceptions depends on the exact class of Iterator (lazy or pre-constructed) that the pass-author decides to allocate. This makes it very hard to understand edge-case behavior of passes and is the reason that CpgPass and ParallelCpgPass are due for deprecation (and then for removal after some deprecation period).

The newer ForkJoinParallelCpgPass is always capable of handling exceptions in the passes: All changes are coalesced into a single diffgraph; if an uncaught exception occurs, then we log the error, run finish() to close file-descriptors etc and then re-throw the exception without committing any changes to the graph.

Exceptions during diffgraph application (like the one you showed):

We currently don't verify that the diffgraph conforms to the schema before applying it. Hence, the graph is always in an undefined broken unrecoverable state after such an error occurs.

I agree that we should rethrow the exception (and nuke the entire scan) 👍

Before merging such a change, we should verify in kibana that prod doesn't have any such warnings, to avoid breaking stuff that currently manages to limp on to a scan result.

[mpollmeier]

@simkoc this is fixed in cpg 1.3.492

[simkoc]

yes it works now, thank you