search

selmf / unarr

A decompression library for rar, tar, zip and 7z archives
GNU Lesser General Public License v3.0
70 stars 13 forks source link

add fuzzer test #4

Closed comicfans closed 2 years ago

comicfans commented 5 years ago 
create a FUZZER_TEST build option, which will turn on clang
fuzzer+address sanitizer and 7Z support. and add a fuzzer_test
test to fuzze unarr
selmf commented 5 years ago

Allright, time to tackle this. Sorry for letting you wait for so long. First of all, I really like this. I have been fuzzing unarr using the sample app and afl, leak checking using valgrind, but that obviously hasn't catched all of the bugs. So having a fuzzer which supports address and leak sanitizing is great.

I still have a couple of questions and I will need a few improvements to the code before I can merge this though.

The first thing is trivial, but important nonetheless … coding style. I use 2 spaces of indentation for CMakeLists.txt and most of unarr's code uses 4 spaces. New code (with the exemption of third party stuff) should use the same coding style.

The next point is CMake integration. I try to use "modern CMake" and setting stuff like CMAKE_C_FLAGS is considered bad practice. I'd also like the sanitizer flags to be independent of the fuzzer target, so I can, for example, build the sample app and use afl with asan and lsan enabled. Also, the fuzzer target should not overwrite any build options.

Third and last – code duplication and coverage. The fuzzer code is pretty close to the sample application, which makes me wonder if it could be a better idea to merge these two targets into a single file, but there probably was a reason for you to implement it this way. As for coverage, the sample app uses a bit more of unarr's api, but the fuzzer code is opening zip archives with the deflateonly argument set to "true" - this will effectively disable all zip compression methods but deflate, thus reducing the usefulness of zip fuzzing greatly. That should be changed.

comicfans commented 5 years ago

Yes this should be improved. libFuzzer also suggest per-format based fuzzer is better than all-in-one fuzzer

selmf commented 2 years ago

@comicfans , are you still interested in having this merged? I'm currently working on finishing the next unarr release and if possible, I would like to include your fuzzer target.

The problems I mentioned are not a huge issue. We would just need to fix the CMakeLists.txt to use modern per-target commands to add the fuzzing flags and the rest can be added/improved at a later time. I will help/guide you through the process.

Please let me know if you'd like to complete it.

comicfans commented 2 years ago

Hi @selmf , I'm afraid that recently I'm short of time and not able to fix the PR, you may improve this or close it depending on your need, sorry about this.

selmf commented 2 years ago

@comicfans , no problem. I have the same issue. I will finish the PR. Just tell me how I should add you to the list of authors so I can properly credit you once I merge it.

comicfans commented 2 years ago

@comicfans , no problem. I have the same issue. I will finish the PR. Just tell me how I should add you to the list of authors so I can properly credit you once I merge it.

Ok, you can call me Wang Xin-yu (王昕宇)

selmf commented 2 years ago

@comicfans An updated version of the PR is available at #24 . Feel free to take a look at it before I hit "merge".

During my test runs, this has already exposed a fair amount of minor bugs which will be fixed in the upcoming unarr release, so it is very helpful. Thank you for your contribution :)

comicfans commented 2 years ago

Great to see it being merged