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

Return an error instead of nil when parseline gets nil/empty input #5

Closed
wants to merge 1 commit into from

Conversation

ryancox
Copy link

@ryancox ryancox commented May 10, 2016

This is a result of my first pass at go-fuzz'ing the lexer

@@ -188,10 +189,15 @@ func (mr *metricReceiver) getAdditionalTags(addr string) types.Tags {

// ParseLine with lexer impl.
func (mr *metricReceiver) parseLine(line []byte) (*types.Metric, error) {
if line == nil {
return nil, errors.New("Error parsing line. Byte array is nil")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this check is necessary, is it?

@ash2k
Copy link
Contributor

ash2k commented May 10, 2016

Thanks for doing this! Can you share the input data corpus you used? Tooling/code you wrote? I'd like to include it in the build process.

@ryancox
Copy link
Author

ryancox commented May 11, 2016

I have a WIP fuzz branch that includes a few items in the corpus as well as a new 'fuzz' target in the Makefile. Regarding the above questions about defensive checking, you will see if you look at that branch that there is code that does round-tripping using a call to parseLine. Without those checks, go-fuzz reports a crasher.

@ash2k
Copy link
Contributor

ash2k commented May 25, 2016

I've merged my branch where this piece is refactored. Thanks for the effort! I'll take a look at your branch when I have time. Really keen to add fuzz testing to this project.

@ash2k ash2k closed this May 25, 2016
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