support/fuzz: Add xdr.ClaimPredicate JSON fuzzing #3181
Conversation
|
how did you generate the corpus? |
|
I created just one file ( |
There was a problem hiding this comment.
💯 adding some fuzzing. One suggestion (💡 ), and one question (❓ ) are inline. Regardless 👍 .
In regards to where this code lives I think it should live inside this monorepo. However, I'm not sure about support/fuzz being the place. It seems akin to putting tests in a separate package. Like tests I think fuzz tests should live close to the code they are testing. The README for go-fuzz points out it is designed for putting in the package it is testing.
| return *p.RelBefore >= 0 | ||
| } | ||
|
|
||
| panic("Invalid type") |
There was a problem hiding this comment.
❓ What's the motivation for panicing here? Should this return false rather than panicing given that it is not valid?
There was a problem hiding this comment.
This line should never be executed because Unmarshal called before validate should error on non-existent union arm and we check all available options in the switch above.
| // 1. Install go-fuzz: https://github.com/dvyukov/go-fuzz | ||
| // 2. go-fuzz-build | ||
| // 3. go-fuzz |
There was a problem hiding this comment.
💡 Can you place these commands in a gofuzz.sh file in the repo root in a similar fashion as govet.sh? The shell script can install the go-fuzz tool at a specific version, then run the commands. It'll mean easy for others to get going without having to stumble on this comment, and we can include the fuzz test in a CI run, which I think will be important because the build tag means we don't verify this code compiles anywhere in tests.
There was a problem hiding this comment.
This is a great idea but I'd like to do it in a separate PR. We won't run fuzzer in Circle-CI, we probably need to use our internal infra for this. There are multiple reasons: go-fuzz runs indefinitely, we need to update the corpus when new files are generated and it's better to run fuzzing on a powerful machine. I'll create a script but when we agree when we want to run fuzzing.
|
I moved the file to |
What
Add
support/fuzzpackage and the first fuzzer forxdr.ClaimPredicateJSON methods.Why
A bug in
xdr.ClaimPredicate.MarshalJSON(#3179) stopped ingestion in testnet. I wrote a fuzzer to ensure there are no more bugs. Added files to separatesupport/fuzzpackage to add more fuzzers in the future.Known limitations
Do we want to add fuzzers here or to separate repo?