-
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
[DVT-812] Conformance Fuzzing Library #87
Conversation
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.
Headed in the right direction but i had a few questions.
cmd/rpcfuzz/argfuzz/argfuzz.go
Outdated
) | ||
|
||
func MutateExecutor(arg []byte, c fuzz.Continue) []byte { | ||
switch rand.Intn(10) { |
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 seems a little brittle. I.e. you need to keep this hard coded number and the number of cases in this switch statement in sync. It might be worth making an interface of the various mutators and putting them into a list at somepoint.
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.
updated
cmd/rpcfuzz/argfuzz/argfuzz.go
Outdated
} | ||
|
||
func ByteMutator(arg []byte, c fuzz.Continue) []byte { | ||
if arg == nil { |
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 interesting. Does this mean you can't mutate nil input? It seems like you'd still want that the be mutable.
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.
updated it so that it can take nil's now. see below comment!
cmd/rpcfuzz/argfuzz/argfuzz.go
Outdated
if d == nil { | ||
continue | ||
} |
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.
Same comment as above. Given that we're fuzzing, it seems like you would fuzz a nil
the same way that you would fuzz anything else especially since the fuzzer can produce nil outputs.
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.
true...i just updated it so that if it's nil, it will be set to a random string then go through the regular mutator. I wonder what would be a better approach here, i.e. set it to a random value (int, string, etc.) then run it through the mutators?
cmd/rpcfuzz/argfuzz/argfuzz.go
Outdated
if n == 0 { | ||
return arg | ||
} |
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 base case seems a little odd. Same potential consider as the other nil checks. If the input is empty you're not doing any mutation?
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.
good point! updated, this base case should only happen on delete mutators as there wouldn't be anything to delete in an empty byte slice
cmd/rpcfuzz/rpcfuzz.go
Outdated
currTestResult := testreporter.TestResult{ | ||
Name: currTest.GetName(), | ||
Method: currTest.GetMethod(), | ||
Args: make([][]interface{}, n), | ||
Result: make([]interface{}, n), | ||
Errors: make([]error, n), | ||
NumberOfTestsRan: n, | ||
} |
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.
I would say, if you're going to go through the trouble of creating a type of test results, you might want to have an actual constructor to avoid this constructions.
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.
added! 🧠 💡
cmd/rpcfuzz/rpcfuzz.go
Outdated
} | ||
|
||
if err != nil { | ||
currTestResult.NumberOfTestsFailed++ |
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.
Same thought with all this ++
. YOu might want something like
r := testreporter.New(curTest) // it should be able to get all of the information that it needs from the test interface
r.Passed()
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.
thanks for the suggestion, i updated it to incorporate that.
cmd/rpcfuzz/rpcfuzz.go
Outdated
wg sync.WaitGroup | ||
mutex sync.Mutex |
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.
These aren't helpful names.
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.
updated
- updated mutation support for nils, float32, and float64 - cleaned up testreporter for pass/fail tests - improved code readability and maintainability via refactors and docs
Description
Fuzzing functionality integrated into the arguments of the JSON RPC calls, by running with the
--fuzz
flag. The fuzzer's effectiveness becomes evident through multiple iterations of running fuzzed tests (default to 100 times), with the expected outcome being an error returned by the RPC.Jira / Linear Tickets
Testing
Test run:
What's next?