Skip to content

Conversation

RTann
Copy link
Contributor

@RTann RTann commented Dec 14, 2023

After I thought #1180 would fix the panic I ran into, it turns out it's still there. I believe this is the culprit.

Next does the following loop:

if l.err != nil {
	return false
}

var vs []claircore.Vulnerability
for l.err = l.dec.Decode(&l.de); l.err == nil; l.err = l.dec.Decode(&l.de) { ... }
l.e = l.next
return true

After the loop, it is assumed l.err == nil, but that's not necessarily true. It's possible Decode returns a non-nil error, which should make Next return false.

@RTann RTann requested a review from a team as a code owner December 14, 2023 02:15
@RTann RTann requested review from crozzy and hdonnay and removed request for a team December 14, 2023 02:15
@RTann RTann force-pushed the ross/jsonblob-fix-panic branch from a55fd48 to ba2afe9 Compare December 14, 2023 02:20
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.19%. Comparing base (07ae3f0) to head (b55b68d).
⚠️ Report is 539 commits behind head on main.

Files with missing lines Patch % Lines
libvuln/jsonblob/jsonblob.go 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1181      +/-   ##
==========================================
- Coverage   52.23%   52.19%   -0.05%     
==========================================
  Files         221      221              
  Lines       16878    16881       +3     
==========================================
- Hits         8817     8811       -6     
- Misses       7252     7259       +7     
- Partials      809      811       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

}
}
}
if l.Err() != nil {
Copy link
Contributor Author

@RTann RTann Dec 14, 2023

Choose a reason for hiding this comment

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

this may not be completely correct. This allows EOF to still return true which is probably ok, but only if l.next != nil, I think. When l.next = nil and l.err == io.EOF, this returns true, but will still cause a panic because l.e will, therefore, be nil, which is never expected when Next returns true

Copy link
Member

Choose a reason for hiding this comment

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

By my thinking, the error check on the first entry should catch this?

@RTann RTann changed the title fix(jsonblob): check for error in Next jsonblob: check for error in Next Dec 15, 2023
Signed-off-by: RTann <rtannenb@redhat.com>
@RTann RTann force-pushed the ross/jsonblob-fix-panic branch from ba2afe9 to b55b68d Compare January 4, 2024 07:12
@RTann RTann closed this by deleting the head repository Oct 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants