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

Tidy up SPDX parsing #3358

Merged
merged 3 commits into from
Dec 6, 2022
Merged

Tidy up SPDX parsing #3358

merged 3 commits into from
Dec 6, 2022

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Dec 5, 2022

🛠️ Fixes #3344.

This PR provides 3 upgrades to the SPDX parser:

  1. Defensively process the results returned by the SPDX parser, to prevent accidentally dereferencing a nil pointer as in Daemon panics when exporting sbom #3344.
  2. Use a newer version of the spdx/tools-golang library. No release since v0.3.0 is available, however, we can use the same commit as Syft currently vendors. This newer parser does less "magic" for JSON SPDX, and pretty much does a naive json.Unmarshal. This means we can drop our panic/recovery logic, and additionally prevents mishandling of a single file being owned by multiple packages, which is the underlying issue that causes Daemon panics when exporting sbom #3344.
  3. With the new SPDX parser, we can upgrade to v2.3 of the SPDX specification, which has been released since we started the SBOM work on buildkit.

It seems like the SPDX parser can produce reuslts that contain nils in
unexpected places. This patch introduces more defensive checks to skip
over nil values, or to explicitly populate them if they haven't been
provided exactly.

Signed-off-by: Justin Chadwell <me@jedevc.com>
The new SPDX JSON parsers are significantly simpler, and use a simple
json.Unmarshal, which reduces the ability of the tooling to panic.

Additionally, it easily handles the case of a single file being owned by
multiple packages, which otherwise resulted in nil values in the
Package.Files slices.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

It might be better to just parse out the field we need and leave else to json.RawMessage. This way when new fields are added we don't remove them on older buildkit releases.

@tonistiigi tonistiigi merged commit 98506ce into moby:master Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Daemon panics when exporting sbom
2 participants