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
Conversation
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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:
- Non-rooted deference, where following the path of repeated de-reference ends in a variable
- 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>
@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! |
Signed-off-by: Will Murphy <will.murphy@anchore.com>
* 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>
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: