perf(fuzz): Improves code coverage of fuzzers #554
Conversation
Updates fuzz target to include; - A round trip writer - A slice reader - Pretty printing, debug and display code
|
I correctly understand, that the purpose of this PR is to call as much methods as we can for fuzzing them? Would it be valuable to test each method in it's own I also noted, that we do not run fuzzing in our CI. Could you add a job for that to the workflow? |
|
Yeah, that's right, this fuzz harness is just calling as much functionality as possible to find ways that it could crash. . I think that there could be some value in splitting this fuzz harness up into 1 or more logical units. But I think it'll be impractical to do it as 1 function per There are other ways of fuzzing, for example, you can do property-based fuzz testing. In the context of quickxml this might look like generate_arbitrary_xml->parse_xml then assert that the parsed xml is identical to the generated xml. I'd be happy to open another PR doing this, although this would required some more intrusive changes to quick-xml, namely implementing arbitrary for the quick-xml types. I'd be happy add this to the CI. As quick-xml is already integrated with google/oss-fuzz might I suggest adding an oss-fuzz specific github action instead? |
|
Ok. I just have very limited knowledge about best practices in fuzzing, so I want to hear suggestions from more advanced users. Then the current implementation is fine.
Seems reasonable. Do it.
I think it is reasonable change, just make new dependency optional and run fuzzing with a special feature enabled. |
Done |
| steps: | ||
| - name: Build Fuzzers | ||
| id: build | ||
| uses: google/oss-fuzz/infra/cifuzz/actions/build_fuzzers@master |
There was a problem hiding this comment.
Is there a normal practice to depend on master for that job? The tagged version would be better, as I think (but I maybe wrong)
There was a problem hiding this comment.
This is normal practice yeah. Although now that you mention it, it does seem a little strange that this is "normal practice".
There was a problem hiding this comment.
There are some docs here https://google.github.io/oss-fuzz/getting-started/continuous-integration/
| with: | ||
| name: artifacts | ||
| path: ./out/artifacts |
There was a problem hiding this comment.
That name / path has some concrete meaning for the job? Maybe better to name them crashes if the concrete name do not matter?
There was a problem hiding this comment.
Yeah it has a specific meaning in the context of fuzzing. Artifacts encompass more than just crashes. e.g. crashes, out-of-memory errors, timeouts (e.g. triggered by an infinite loop) etc. I think even if we could change the name of this directory (which oss-fuzz won't like), it wouldn't make sense to as some of the outputs here aren't necessarily just crashes. There is some documentation for artifacts here.
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #554 +/- ##
==========================================
- Coverage 61.25% 61.15% -0.11%
==========================================
Files 32 33 +1
Lines 15654 15670 +16
==========================================
- Hits 9589 9583 -6
- Misses 6065 6087 +22
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Updates fuzz target to include;