-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
bam,sam: fix zero and negativish value handling #110
Conversation
I am leaving the fuzzer running on the fixed code. |
Identified by go-fuzz.
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
Identified by go-fuzz.
Found a couple more that can be protected against. There are more, but they appear to be just massive over allocation that only cause a problem in a low memory environment. I'll make a new issue for those. PTAL |
if b.len() < n { | ||
b.err = io.ErrUnexpectedEOF | ||
return nil | ||
} | ||
s := b.off |
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.
wouldn't it be better to check that b.len() < n + b.off
?
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.
b.len()
is the number of remaining bytes in the buffer (see here).
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.
ah. right.
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
I'm going to merge this though it does not fix all crashers. The remaining crashers seem to be only occurring when go-fuzz is running with multiple slaves, thus allowing memory exhaustion by concerted effort. I'm still not happy with the fact that small inputs can cause this, but it's reasonable I guess. I'll follow those at #111, and I'm continuing to run go-fuzz on the current state with a single slave to flush out any other cases here. |
@brentp Please take a look.