-
Notifications
You must be signed in to change notification settings - Fork 567
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 panic in APK version specifier handling #1494
Conversation
Having trouble making sense of the failure logs: https://github.com/anchore/syft/actions/runs/3963383936/jobs/6791170964#step:6:159 Help appreciated 😃 |
@luhring we've updated this in main now where this CLI test is fixed - if you're able to merge from upstream you should no longer see this failure |
I basically submitted the same fix for #1484, just in a different way. Don't mind in which way this is fixed, though this also applies to "dependencies", not just "provides" from what I remember correctly. So you would also have to add if depSpecifier == "" {
continue
} later since that can also be empty by the packages you provide with apko (at least if you want to keep the same logic as for "provides" - you could also remove the if-checks and due to the workaround in the stripVersionSpecifier function it would still not crash, but not sure whether what would end up with garbage later - I'am not that familiar with the Syft code). But instead of doing these if-checks, maybe merging a part of this PR and my PR could make sense? So keeping your modification in Otherwise having empty strings in the Metadata struct seems like a way to shoot yourself in the foot later on anyways since it will be enumerated over in for loops. Unless of course you somehow want to know if the field existed in the APK definition but was left empty deliberately. |
@Nirusu Ah! I hadn't seen your PR, sorry — I'm glad you caught this. I actually like your solution better! I say we merge it, and I can close mine. My PR was just trying to stop the bleeding. Yours might need a rebase, and I like the idea of incorporating the changes to |
Sounds good, thanks @wagoodman! |
Signed-off-by: Dan Luhring <dluhring@chainguard.dev>
Signed-off-by: Dan Luhring <dluhring@chainguard.dev>
Signed-off-by: Dan Luhring <dluhring@chainguard.dev>
…ue check Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
6767b68
to
f7f22c6
Compare
note: the force push was to rebase on the latest in main (fixes the failing CLI test) changes made:
|
* Add failing test for strip version specifiers panic Signed-off-by: Dan Luhring <dluhring@chainguard.dev> * Fix test Signed-off-by: Dan Luhring <dluhring@chainguard.dev> * Prevent panic scenario in helper func Signed-off-by: Dan Luhring <dluhring@chainguard.dev> * Fix lint issue Signed-off-by: Dan Luhring <dluhring@chainguard.dev> * add tests for apk stripVersionSpecifier() and remove caller empty value check Signed-off-by: Alex Goodman <alex.goodman@anchore.com> Signed-off-by: Dan Luhring <dluhring@chainguard.dev> Signed-off-by: Alex Goodman <alex.goodman@anchore.com> Co-authored-by: Alex Goodman <alex.goodman@anchore.com>
A bug was introduced in #1063 where Syft would panic if a package listed in the installed DB doesn't "provide" anything.
This PR
reworks the logic so that Syft allows for empty "provides" values(edit: no longer part of this PR, see #1494 (comment)). It also adjusts thestripVersionSpecifier
helper function to remove the possibility of panicking (index out of range).