Re: [xz-devel] Adding fuzz testing support to liblzma
Dear Lasse, On 11/2/18 9:20 PM, Lasse Collin wrote: > On 2018-10-31 Bhargava Shastry wrote: >> On 10/30/18 6:26 PM, Lasse Collin wrote: >>> On 2018-10-30 Bhargava Shastry wrote: - oss-fuzz requires a Google linked email address of the maintainer. Could you please provide me one? >>> >>> No, I'm sorry. This is the email address to use to contact me, and I >>> don't plan to link this address to a Google account. >> >> No need to apologize :) >> >> I didn't mean to be presumptuous. > > I didn't mean to imply anything like that. Sorry if there was a > misunderstanding. I understand OSS-Fuzz is a Google project so it makes > sense to use Google accounts for logins. (So far I only have a Google > account to access Play Store on Android; I don't use it otherwise.) No worries, all is good :) >> The thing is that oss-fuzz creates >> bug reports on Google infrastructure and hence the requirement. >> >> I will ask oss-fuzz folks if there is an alternative to Google-linked >> account for viewing bug reports. > > Thanks. I saw your discussion here: > > https://github.com/google/oss-fuzz/issues/1915 > > Just seeing bug reports (or getting some kind of notice that something > has been found) goes a long way. :-) Okay :) >> After running version 2 overnight (with the corpus generated from >> version 1), I see that v2 covers 1007 CFG edges (1% better coverage). >> >> I agree that version 1 is better :) > > Hmm OK, thanks. I was thinking if v2 with bigger buffers is worth > considering still but I don't want to think more, so let's go with v1. Thank you. Moving forward, we could create more fuzz targets for different buffer sizes, or have a single target but make the buffer size conditional on, say, the first byte of fuzzed input. We could pick up on this thread once the initial integration with oss-fuzz has been accepted. > I committed these four files: > > tests/ossfuzz/Makefile > tests/ossfuzz/config/fuzz.dict > tests/ossfuzz/config/fuzz.options > tests/ossfuzz/fuzz.c > > I hope they are OK. > > Is this all that I have to do for now? Other people will take care of > the rest (Dockerfile and such that were in pdknsk's commit), right? Right, I have sent a PR to this effect. https://github.com/google/oss-fuzz/pull/1919 Once this is merged, xz will be continuously fuzzed. Thank you once again for your feedback and help on this front :) I wish more software creators/maintainers show similar interest in fuzzing! Regards, Bhargava -- Bhargava Shastry Security in Telecommunications TU Berlin / Telekom Innovation Laboratories Ernst-Reuter-Platz 7, Sekr TEL 17 / D - 10587 Berlin, Germany phone: +49 30 8353 58235 Keybase: https://keybase.io/bshastry
Re: [xz-devel] Adding fuzz testing support to liblzma
On 2018-10-31 Bhargava Shastry wrote: > On 10/30/18 6:26 PM, Lasse Collin wrote: > > On 2018-10-30 Bhargava Shastry wrote: > >> - oss-fuzz requires a Google linked email address of the > >> maintainer. Could you please provide me one? > > > > No, I'm sorry. This is the email address to use to contact me, and I > > don't plan to link this address to a Google account. > > No need to apologize :) > > I didn't mean to be presumptuous. I didn't mean to imply anything like that. Sorry if there was a misunderstanding. I understand OSS-Fuzz is a Google project so it makes sense to use Google accounts for logins. (So far I only have a Google account to access Play Store on Android; I don't use it otherwise.) > The thing is that oss-fuzz creates > bug reports on Google infrastructure and hence the requirement. > > I will ask oss-fuzz folks if there is an alternative to Google-linked > account for viewing bug reports. Thanks. I saw your discussion here: https://github.com/google/oss-fuzz/issues/1915 Just seeing bug reports (or getting some kind of notice that something has been found) goes a long way. :-) > After running version 2 overnight (with the corpus generated from > version 1), I see that v2 covers 1007 CFG edges (1% better coverage). > > I agree that version 1 is better :) Hmm OK, thanks. I was thinking if v2 with bigger buffers is worth considering still but I don't want to think more, so let's go with v1. I committed these four files: tests/ossfuzz/Makefile tests/ossfuzz/config/fuzz.dict tests/ossfuzz/config/fuzz.options tests/ossfuzz/fuzz.c I hope they are OK. Is this all that I have to do for now? Other people will take care of the rest (Dockerfile and such that were in pdknsk's commit), right? -- Lasse Collin | IRC: Larhzu @ IRCnet & Freenode
Re: [xz-devel] Adding fuzz testing support to liblzma
Dear Lasse, On 10/30/18 6:26 PM, Lasse Collin wrote: > On 2018-10-30 Bhargava Shastry wrote: >> - oss-fuzz requires a Google linked email address of the maintainer. >> Could you please provide me one? > > No, I'm sorry. This is the email address to use to contact me, and I > don't plan to link this address to a Google account. No need to apologize :) I didn't mean to be presumptuous. The thing is that oss-fuzz creates bug reports on Google infrastructure and hence the requirement. I will ask oss-fuzz folks if there is an alternative to Google-linked account for viewing bug reports. >> - It is better that the test harness and related config (dictionary, >> other fuzzer options) reside in the xz source repo. Are you okay >> maintaining these in the long run? > > Including the files in the xz repo is fine. I can maintain them in sense > that fuzz.c compiles and I can merge fuzzing related patches that get > sent to me. I hope this is enough. Thank you, I believe this should be enough. >> As starting point, I used all files with the "xz" extension that I >> could find in the source repo (total of 63 files). > > I guess it's a good starting point. > > Most of them are under hundred bytes and only one is over thousand > bytes (good-1-delta-lzma2.tiff.xz is 51,316 bytes). The bad files are > based on certain good files but each bad file has something broken in > it, so perhaps the bad files aren't so great for fuzzing (if the damage > is at the beginning, the decoder might stop there and fuzzing bits past > that point is pointless). > >> I also did the following experiment >> >> - I ran version 1 overnight (over 16 hours in total) >> - The coverage saturated at about 996 CFG edges >> >> Then, I took the corpus that was generated for v1 fuzzing and fed it >> to v2. My hope is that this will quickly tell me how much better >> (coverage wise) v2 is were it to be run for as long as v1 >> >> - I found v2 covers 1004 CFG edges i.e., only 8 CFG edges more than v1 >> >> However, to be sure I need to keep v2 running for as long as v1, but >> my guess is that this saturation will prevail. > > The test method sounds good. :-) Only eight more edges sounds low since > there are more than eight places where the code can run out of input or > output and has to stop. Perhaps it needs better input files to hit more > of such situations. Or, like I said in the previous email, maybe the > small input/output buffers aren't as valuable for fuzzing as I thought > and we should just use the simple fast version. After running version 2 overnight (with the corpus generated from version 1), I see that v2 covers 1007 CFG edges (1% better coverage). I agree that version 1 is better :) Regards, Bhargava -- Bhargava Shastry Security in Telecommunications TU Berlin / Telekom Innovation Laboratories Ernst-Reuter-Platz 7, Sekr TEL 17 / D - 10587 Berlin, Germany phone: +49 30 8353 58235 Keybase: https://keybase.io/bshastry
Re: [xz-devel] Adding fuzz testing support to liblzma
On 2018-10-30 Bhargava Shastry wrote: > - oss-fuzz requires a Google linked email address of the maintainer. > Could you please provide me one? No, I'm sorry. This is the email address to use to contact me, and I don't plan to link this address to a Google account. > - It is better that the test harness and related config (dictionary, > other fuzzer options) reside in the xz source repo. Are you okay > maintaining these in the long run? Including the files in the xz repo is fine. I can maintain them in sense that fuzz.c compiles and I can merge fuzzing related patches that get sent to me. I hope this is enough. > As starting point, I used all files with the "xz" extension that I > could find in the source repo (total of 63 files). I guess it's a good starting point. Most of them are under hundred bytes and only one is over thousand bytes (good-1-delta-lzma2.tiff.xz is 51,316 bytes). The bad files are based on certain good files but each bad file has something broken in it, so perhaps the bad files aren't so great for fuzzing (if the damage is at the beginning, the decoder might stop there and fuzzing bits past that point is pointless). > I also did the following experiment > > - I ran version 1 overnight (over 16 hours in total) > - The coverage saturated at about 996 CFG edges > > Then, I took the corpus that was generated for v1 fuzzing and fed it > to v2. My hope is that this will quickly tell me how much better > (coverage wise) v2 is were it to be run for as long as v1 > > - I found v2 covers 1004 CFG edges i.e., only 8 CFG edges more than v1 > > However, to be sure I need to keep v2 running for as long as v1, but > my guess is that this saturation will prevail. The test method sounds good. :-) Only eight more edges sounds low since there are more than eight places where the code can run out of input or output and has to stop. Perhaps it needs better input files to hit more of such situations. Or, like I said in the previous email, maybe the small input/output buffers aren't as valuable for fuzzing as I thought and we should just use the simple fast version. -- Lasse Collin | IRC: Larhzu @ IRCnet & Freenode
Re: [xz-devel] Adding fuzz testing support to liblzma
Dear Lasse, Thanks for your feedback. My reply is inline. However, it is a good time to discuss oss-fuzz integration as we apply final touches on the test harness :) I have a few questions for you: - oss-fuzz requires a Google linked email address of the maintainer. Could you please provide me one? - It is better that the test harness and related config (dictionary, other fuzzer options) reside in the xz source repo. Are you okay maintaining these in the long run? Thank you :) On 10/29/18 9:27 PM, Lasse Collin wrote: > On 2018-10-29 Bhargava Shastry wrote: >> Thanks for providing two versions for me to test. Here are the >> results: >> >> - version 1 decompresses the whole of fuzzed (compressed) data >> - version 2 decompresses in chunks of size (input=13 bytes) >> >> ### Executions per second >> >> I ran both versions a total of 96 times (I have 16 cores :-)) >> >> - version 1 averaged 1757.20 executions per second >> - version 2 averaged 429.10 executions per second >> >> So, clearly version 1 is faster > > Yes, and the difference is bigger than I hoped. > >> Regarding coverage >> >> - version 1 covered 950.26 CFG edges on average >> - version 2 covered 941.11 CFG edges on average > > I assume you had the latest xz.git that supports > FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION. That's correct. > Did you run the same number of fuzzing rounds on both (so the second > version took over four times longer) or did you run them for the same > time (so the second version ran only 1/4 of rounds)? It's the latter, I ran for a fixed time duration of 2 minutes. In this time, - version 1 was fuzzed 212677 times on average i.e., the test was fuzzed with that many distinct inputs - version 2 was fuzzed 51986 times on average So, like you say, roughly v2 ran for 1/4 of rounds as v1. > If both version saw the same number of rounds, I would expect the > second version to have the same or better coverage. But if the > comparison was based on time, then it's no surprise if the first > version has better apparent coverage even if it is impossible for it to > hit certain code paths that are possible with the second version. It > might also depend on which input file is used as a starting point for > the fuzzer. As starting point, I used all files with the "xz" extension that I could find in the source repo (total of 63 files). I also did the following experiment - I ran version 1 overnight (over 16 hours in total) - The coverage saturated at about 996 CFG edges Then, I took the corpus that was generated for v1 fuzzing and fed it to v2. My hope is that this will quickly tell me how much better (coverage wise) v2 is were it to be run for as long as v1 - I found v2 covers 1004 CFG edges i.e., only 8 CFG edges more than v1 However, to be sure I need to keep v2 running for as long as v1, but my guess is that this saturation will prevail. >> Overall, version 1 is superior imho. > > I don't know yet. Increasing the input and output chunk sizes is > probably needed to make the second version faster. You could try > some odd values between 100 and 250, or maybe even up to 500. Okay, I can try this out once current experiment completes. Regards, Bhargava
Re: [xz-devel] Adding fuzz testing support to liblzma
On 2018-10-29 Bhargava Shastry wrote: > Thanks for providing two versions for me to test. Here are the > results: > > - version 1 decompresses the whole of fuzzed (compressed) data > - version 2 decompresses in chunks of size (input=13 bytes) > > ### Executions per second > > I ran both versions a total of 96 times (I have 16 cores :-)) > > - version 1 averaged 1757.20 executions per second > - version 2 averaged 429.10 executions per second > > So, clearly version 1 is faster Yes, and the difference is bigger than I hoped. > Regarding coverage > > - version 1 covered 950.26 CFG edges on average > - version 2 covered 941.11 CFG edges on average I assume you had the latest xz.git that supports FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION. Did you run the same number of fuzzing rounds on both (so the second version took over four times longer) or did you run them for the same time (so the second version ran only 1/4 of rounds)? If both version saw the same number of rounds, I would expect the second version to have the same or better coverage. But if the comparison was based on time, then it's no surprise if the first version has better apparent coverage even if it is impossible for it to hit certain code paths that are possible with the second version. It might also depend on which input file is used as a starting point for the fuzzer. > Overall, version 1 is superior imho. I don't know yet. Increasing the input and output chunk sizes is probably needed to make the second version faster. You could try some odd values between 100 and 250, or maybe even up to 500. On the other hand, it's possible that I'm putting too much weight on the importance of fuzzing the stop & continue code paths. Thanks again! -- Lasse Collin | IRC: Larhzu @ IRCnet & Freenode
Re: [xz-devel] Adding fuzz testing support to liblzma
Dear Lasse, Thanks for providing two versions for me to test. Here are the results: - version 1 decompresses the whole of fuzzed (compressed) data - version 2 decompresses in chunks of size (input=13 bytes) ### Executions per second I ran both versions a total of 96 times (I have 16 cores :-)) - version 1 averaged 1757.20 executions per second - version 2 averaged 429.10 executions per second So, clearly version 1 is faster Regarding coverage - version 1 covered 950.26 CFG edges on average - version 2 covered 941.11 CFG edges on average Since the coverage values were close, I noted the p-value between these two distributions to be 5.84e-10 So, although the average coverage is close, they are clearly different. Overall, version 1 is superior imho. I will send you the plots off-list. Regards, Bhargava
Re: [xz-devel] Adding fuzz testing support to liblzma
On 2018-10-26 Bhargava Shastry wrote:
> On 10/25/18 11:34 PM, Lasse Collin wrote:
> >> +// Decode BUFSIZ==8192 bytes of inbuf at a time
> >
> > The value of BUFSIZ depends on libc and isn't always 8192.
>
> Agreed. What would you like it to be? Afaiu, we are doing not only
> header decoding but also contents of the compressed block itself,
> right?
Yes, not only headers but the whole file.
The buffer size doesn't matter too much. For mem-to-mem operations, 4096
bytes is plenty (there's no syscall overhead for disk I/O). If it is
important to clear output buffer with memset(), 1024 bytes could be
enough to reduce the overhead from the memset() calls. Going much below
512 starts to slow things down due to the increased number of calls to
lzma_code().
> As a reference, fuzzers usually generate between 1 byte and a few
> hundred KB of compressed data.
OK, this is good to know.
> Test harnesses for other compression libs such as lzo [1] use an
> output buffer size equal to default block size (= 256 KB).
This is not comparable. With that LZO API you need to decompress the
whole block with a single call and thus the caller needs to know the
block size.
By the way, a few hopefully helpful comments about the linked [1]:
- If lzo_init() fails, 0 is returned and thus the error is silently
ignored. There is a printf if __DEBUG__ is defined but that's it.
I'm not familiar with the fuzzing framework so I don't know if it
matters, but without such knowledge, silently ignoring errors looks
very suspicious to me.
- out[bufSize] is a 256 KiB buffer that is allocated on stack. On
Linux/glibc the default stack size is 8 MiB, so maybe it's OK in
this particular situation, but in general 256 KiB buffer is a bit
big to be allocated on stack (it could be made static since it
doesn't need to be thread safe).
> > I guess the reason to split the input into BUFSIZ chunks is there to
> > emulate reading from a file, but to me that doesn't seem very useful
> > from point of view of fuzzing. Splitting the input into smaller
> > chunks is good in sense that it tests that liblzma behaves
> > correctly when liblzma doesn't get the whole file at once. However,
> > the BUFSIZ is far too big to do this for header decoding.
>
> It is my understanding that the harness is doing more than just header
> decoding, right? I notice (in the generated html coverage report) that
> calls to "block_decode" are also executed while fuzzing.
Yes. Testing with small buffer sizes might help in finding corner cases
outside header decoding too (the decoder must be able to stop and
continue processing input at any point in the file). Doing it one byte
at a time hurts fuzzing performance though. Depending on the input, it's
like 3-20 times slower than with normal buffer sizes (1 KiB or more).
Perhaps a compromise (a few bytes at a time) should be considered too.
> > It should *never* happen if the code calling the liblzma functions
> > is correct. It is better to use if() instead of assert() here to
> > catch LZMA_PROG_ERROR. It's not good to rely on that assertion
> > checks haven't been disabled with #define NDEBUG.
>
> The reason I chose assert() was because fuzzers pick up on signals
> such as SIGABRT, SIGSEGV etc. Also, libFuzzer interface reserves
> non-zero return codes, the LLVMFuzzerTestOneInput() API is required
> to always return 0.
OK.
> Would replacing "assert" with an "abort" make sense? This way, we
> don't need to use a debug build (which may slow down fuzzing).
Yes, abort() combined with a fprintf(stderr, ...) should be fine.
> >> +// Init decoder.
> >> +if (!init_decoder(&strm)) {
> >> +// Decoder initialization failed. There's no point
> >> +// retrying, so bail out.
> >> +return 0;
> >> +}
> >
> > Returning 0 indicates success, I guess. If initialization fails, the
> > actual fuzzing is never done. While it's unlikely that the
> > initialization fails, it's still essential to report the error to
> > the caller instead of silently skipping the whole fuzzing step.
>
> Would a call to "abort" make sense here?
Yes, here too.
> > - Is it enough to pass the whole input to liblzma at once? If not,
> > how small chunks to use? One byte could give most thorough
> > fuzzing but is slower too.
>
> One suggestion is to keep the current output buffer size I guess.
Yes, it is one suggestion. It's just that I'm thinking that small
buffers could improve the fuzzing coverage (more places where the
decoder has to be able to stop & continue the processing of input data).
I apologize that instead of trying to explain in detail what I think
could be tried, I thought it needed less energy from me to write some
code myself.
The first version is effectively very similar to the version you most
recently sent to the list, but I wrote it from scratch because I didn't
know the license of the version you sent and the code is very short
anyway.
The se
