Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
GODRIVER-2550 Add fuzzer to bson packages #1077
GODRIVER-2550 Add fuzzer to bson packages #1077
Changes from 22 commits
daf51bc
bb4f333
00be7df
58a2afd
839da00
ee9ff08
0cfb457
7337330
41efed0
8a0d0e8
c4af531
f2398f3
86aae74
e4ab0f6
c6d97d5
55dac80
e210fb1
4a322c1
1820a5e
045dc76
aec25d1
0ee3576
26b3710
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why is cleaning the testcache and fuzzcache necessary?
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.
@matthewdale I don't think it's necessary. I will remove these lines, I typically clean them locally to see how code coverage compares between runs but it doesn't seem like something we'd care about in CI. I will remove these lines.
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.
Do we not care about the output of
go test
(see the|| true
)?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.
This will still pipe the output, but if the
go test
panics/errors due to a crash (which is good in the case) we don't want the bash script to terminate, rather continue the generated corpus processing.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.
Is there an indicator that will let us know the fuzz test caused a panic?
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.
Sounds good, thanks
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.
@matthewdale Yes, this should still output the results of the test. If I added a
panic
to the fuzz test and then added checkpoints to the script like this:The output would be
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.
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.
This is just a log notifying the user that generated data has been moved to the file which will be tarred and put on S3. It will read like
In this case "new" seems redundant.
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.
Fair, sounds good.
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.
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.
Why not loop over
[]interface{}
? Is there a reason I'm not seeing for using these constructors?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.
It's a practice I took from the Go team. I think the idea is to make the type instantiation more realistic, i.e. something like this:
And not like
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 interesting that we unmarshal, marshal and unmarshal again. What's the reasoning there?
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.
The first unmarshal is to check the validity of the extended JSON. We only want this to generate corpus data on a panic/crash. If the value is not valid BSON and we don't crash, then this subtest if over.
The first marshal is to check that we can encode valid data structures into BSON, it seems unnecessary to fuzz the encoding of invalid data structures.
The last unmarshal is where we check that we can decode valid BSON, if this fails/panics/crashes then we want to know about it.
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.
Are these
testdata/fuzz/FuzzDecode
files separate from thebson-corpus
seeds? How are these added into the seed corpus?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.
This data is different from testdata/bson-corpus. Three of these were interesting cases generated by running the fuzzer. One is BSON that encapsulates all types for an initial maximum code coverage in the style of the encoding/json fuzz test.
From the documentation:
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.
Great, thanks for the explanation.