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

Fix: repeatedly dereference pom variables #2781

Merged
merged 5 commits into from Apr 16, 2024

Conversation

willmurphyscode
Copy link
Contributor

Previously, if there was more than one layer of variable indirection in the pom property (propert A says it has the same value as property B, property B says it has the same value as property C), then Syft would only dereference one layer. Add a loop to dereference variables until either dereferencing fails, or until the variable is completely dereferenced back to a literal.

Fixes #2776

Manual testing on repro example from that issue:

❯ diff <(syft -q dir:spring-boot-empty-project | awk '{print $1 " "  $2 " " $3}') <(/tmp/syft -q dir:spring-boot-empty-project | awk '{print $1 " " $2 " " $3}')
9c9
< spring-boot-starter-actuator ${project.parent.version} java-archive
---
> spring-boot-starter-actuator 3.2.4 java-archive
11,15c11,15
< spring-boot-starter-data-mongodb ${project.parent.version} java-archive
< spring-boot-starter-security ${project.parent.version} java-archive
< spring-boot-starter-test ${project.parent.version} java-archive
< spring-boot-starter-validation ${project.parent.version} java-archive
< spring-boot-starter-web ${project.parent.version} java-archive
---
> spring-boot-starter-data-mongodb 3.2.4 java-archive
> spring-boot-starter-security 3.2.4 java-archive
> spring-boot-starter-test 3.2.4 java-archive
> spring-boot-starter-validation 3.2.4 java-archive
> spring-boot-starter-web 3.2.4 java-archive

Previously, if there was more than one layer of variable indirection in
the pom property (propert A says it has the same value as property B,
property B says it has the same value as property C), then Syft would
only dereference one layer. Add a loop to dereference variables until
either dereferencing fails, or until the variable is completely
dereferenced back to a literal.

Signed-off-by: Will Murphy <will.murphy@anchore.com>
if value, ok := entries[propertyName]; ok {
return value
}
for propertyMatcher.MatchString(propertyCase) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this diff looks larger than it is, because the change in control flow intends the replace function one more tab, but the body of the replace function is not changed.

Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

LGTM

propertyName := strings.TrimSpace(match[2 : len(match)-1]) // remove leading ${ and trailing }
entries := pomProperties(pom)
if value, ok := entries[propertyName]; ok {
return value
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having a loop, adding a recursive call here to resolveProperty could also be a solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That diff would potentially be a lot easier to read. Let me give that a shot. Thanks for the suggestion!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, in adding new tests for this implementation, I realized that there are two cases we need to make sure halt:

  1. Non-rooted deference, where following the path of repeated de-reference ends in a variable
  2. Cyclic dereference, where a variable refers to itself

Neither of these are well-formed pom.xml, we just need to defend against them hanging or overflowing the stack or something.

Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
@willmurphyscode
Copy link
Contributor Author

@kzantow I changed to a recursive implementation and added more test cases to guard against degenerate pom files that have cyclic or unending variable definitions. Looking for a re-review when you have a moment. Thanks!

syft/pkg/cataloger/java/parse_pom_xml_test.go Show resolved Hide resolved
syft/pkg/cataloger/java/parse_pom_xml_test.go Outdated Show resolved Hide resolved
Signed-off-by: Will Murphy <will.murphy@anchore.com>
@willmurphyscode willmurphyscode merged commit 3e71f46 into main Apr 16, 2024
11 checks passed
@willmurphyscode willmurphyscode deleted the fix-repeat-pom-var-dereference branch April 16, 2024 19:44
brian-ebarb pushed a commit to brian-ebarb/syft that referenced this pull request Apr 17, 2024
* Fix: repeatedly dereference pom variables

Previously, if there was more than one layer of variable indirection in
the pom property (propert A says it has the same value as property B,
property B says it has the same value as property C), then Syft would
only dereference one layer. Add a loop to dereference variables until
either dereferencing fails, or until the variable is completely
dereferenced back to a literal.

Signed-off-by: Will Murphy <will.murphy@anchore.com>

* switch to recursive implementation

Signed-off-by: Will Murphy <will.murphy@anchore.com>

* add test cases for degenerate poms

Signed-off-by: Will Murphy <will.murphy@anchore.com>

* switch to recursive implementation

Signed-off-by: Will Murphy <will.murphy@anchore.com>

* remove redundant pieces of test cases

Signed-off-by: Will Murphy <will.murphy@anchore.com>

---------

Signed-off-by: Will Murphy <will.murphy@anchore.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pom parser not resolving all dependency versions
2 participants