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

safer and fail-faster reading of bogus frames #197

Merged

Conversation

imkira
Copy link
Contributor

@imkira imkira commented Mar 28, 2016

When I created #196 I noticed that when enabling -race for travis, the build would just "explode" with a silent:

=== RUN   TestGoFuzzCrashers
signal: killed
FAIL    github.com/streadway/amqp   94.516s

When I analysed it locally, I discovered that TestGoFuzzCrashers was actually taking up several GBs of ram just to run 3 fuzzed strings against ReadFrame, because one of the test cases were actually telling it to allocate like 4GB+ of data.

This is my attempt to change all calls to ReadFull (that could be manipulated by a malicious user) with buffered-reading into a bytes.Buffer-backed buffer (which will grow for every successful read). This has the advantage of failing faster if a frame has a fake length in it. I just placed a 1MB threshold, since I believe most real-world cases will fit that size.

Also, there were some cases with negative lengths that were not being covered and were detected using go-fuzz.

Please let me know what you think.

@michaelklishin
Copy link
Collaborator

Sadly I had to revert this as it breaks CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants