trailofbits / sienna-locomotive

A user-friendly fuzzing and crash triage tool for Windows
https://blog.trailofbits.com/user-friendly-fuzzing-with-sienna-locomotive
GNU Affero General Public License v3.0
131 stars 24 forks source link

Code coverage should encompass more than the main module #333

Closed woodruffw closed 5 years ago

woodruffw commented 5 years ago

Right now, we only instrument basic blocks in the main module. That means that if test.exe links to foo.dll and bar.dll, we'll only record a change in coverage caused by a new code path in bar.dll if it also results in a new code path in test.exe.

That's been true most of the time, but SumatraPDF provides a counterexample: pretty much all document parsing happens in libmupdf.dll with the main module (sumatrapdf.exe) only providing the rendering thread, so we're probably missing important coverage changes.

woodruffw commented 5 years ago

(A good middle ground between just the main module and instrumenting every module would probably be just instrumenting those modules that correspond to functions selected by the user.)

ehennenfent commented 5 years ago

https://github.com/trailofbits/sienna-locomotive/commit/22bc85359b4b334531747fcabffe65b1707b8ae4#diff-532f2cabe3dad1a76c49c9ab7974b983R71 should work for the middle-ground solution if we comment out the first check in the if statement.

ehennenfent commented 5 years ago

This is not, unfortunately, as simple as the commit linked.

The first issue is that we want the callers, but we're copying the callees. I think the right way to do this is to copy all the module_data_t *'s into one set and keep a separate set with pointers to the ones that call targeted functions. When a function hook fires, we find the module it lives in and add it to the second set if the function is targeted.

The second problem is that our arenas now depend on the set of functions selected instead of just the target binary and arguments. If you change the targeted functions, and it changes which modules we measure, you'll (rightfully) get a wildly different coverage score.

woodruffw commented 5 years ago

336 begins work on this.