fix(cyclonedx): Avoid a StackOverflowError due to dependency cycles
#9591
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9591 +/- ##
=========================================
Coverage 68.03% 68.03%
Complexity 1285 1285
=========================================
Files 249 249
Lines 8826 8826
Branches 920 920
=========================================
Hits 6005 6005
Misses 2432 2432
Partials 389 389
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -64,7 +73,7 @@ internal fun Bom.addDependencies(input: ReporterInput, parentRef: String, ids: S | |||
|
|
|||
| ids.forEach { id -> | |||
| val directDependencies = input.ortResult.getDependencies(id, maxLevel = 1, omitExcluded = true) | |||
| addDependencies(input, id.toCoordinates(), directDependencies) | |||
| addDependencies(input, id.toCoordinates(), directDependencies, visited) | |||
There was a problem hiding this comment.
I lack a bit of understanding of the dependency graph, e.g. what a parentRef actually is.
However, can you help understand whether this really only removes cycles or does remove more?
In particular, I'm not sure if visiting a ref in one branch of the tree has effects on another independent e.g. sibling branch of the tree, roughly speaking. (There are recursive calls but all use the same mutable set instead of a copy of it).
There was a problem hiding this comment.
I lack a bit of understanding of the dependency graph, e.g. what a parentRef actually is.
Please check the CycloneDX docs. The "parent" prefix was my choice, but parentRef is what gets serialized as ref, i.e. it is the thing the following dependencies apply to.
In particular, I'm not sure if visiting a
refin one branch of the tree has effects on another independent e.g. sibling branch of the tree, roughly speaking.
The dependency information in CycloneDX is very simple: As it's recorded directly under the document root, it cannot handle cases where the same "parent" might have different dependencies depending on the branch in the graph, or on the scope of the same project.
So the condition simply avoids recording dependencies for the same parent twice, even if dependencies actually might be different (e.g. due to different version conflict resolution in different scopes of the same project), but there simply is no other way to deal with this in CycloneDX AFAICS.
There was a problem hiding this comment.
So the condition simply avoids recording dependencies for the same parent twice, even if dependencies actually might be different (e.g. due to different version conflict resolution in different scopes of the same project), but there simply is no other way to deal with this in CycloneDX AFAICS.
I think that deserves a code comment, I was also wondering why it is ok to use a mutable set here.
There was a problem hiding this comment.
I've extended the existing comment.
Fixes #9587. Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth
force-pushed
the
cdx-avoid-dep-cycles
branch
from
c494785 to
cdfd561
Compare
|
Merging as all relevant test have passed. |
Fixes #9587.