Fuzz tests for Arrow/Parquet

[emkornfield]

I'm generally new to the code base but it looks like existing fuzz tests might be generating random data and making sure it can be read back, but we don't have any fuzz tests for malformed data. I think in the context of Rust the goal would be to avoid panics?

If this is accurate, I'd propose creating fuzz tests that check succeed as long as there is no panic. A beginning corpus arrow-testing repo 2 for each file type.

A second step would be integrate with oss-fuzz (Arrow C++ already does so).

If this is of interest and i'm correct this isn't already done, I can try to see if I can prototype something.

[tustvold]

It isn't an issue IMO if the reader panics on malformed data, this is a perfectly safe and well-defined behaviour. We should try to avoid it, but its not like UB where it would indicate a bug. Panics are just exceptions.

The bigger issue with untrusted/malicious inputs is avoiding the reader getting stuck in infinite loops or exploding the memory usage. I'm not sure how easy such things are to catch using a fuzz testing framework.

With regards to parquet, I can't help feeling the format is sufficiently complex that supporting untrusted input is essentially a fools errand though...

That's all to say adding fuzzing support would be a nice add. I'm not too familiar with the Rust ecosystem's support for it, but @crepererum may know more.

.

[crepererum]

Fuzzing is a good thing, even when you accept panics as an outcome. The fuzzer then has two wrap the method call accordingly.

Regarding the toolchain: We should use cargo-fuzz. That gives us the option to use multiple fuzzers. Then you need to choose a fuzzer. I would suggest you use libFuzzer which comes w/ LLVM, since it is the least invasive one, however it has entered maintenance mode. I see the following options:

• don't care about maintainance mode and just use libFuzzer
• use afl.rs/cargo afl[^afl]
• use libAFL -- which is a modern Rust redesign of AFL(++) -- and also provides a libFuzzer shim

That said, the choice can easily be changed later, since the fuzzer is effectively just a "run some code on this blackbox [u8] input" (like "parse parquet from bytes").

[^afl]: Note that AFL was abandoned for a while, but development is now open and active under the AFL++ project.

[emkornfield]

We've had good experience in parquet-cpp of integrating with https://github.com/google/oss-fuzz I haven't looked into what supporting Rust would look like exactly but that seems like a reasonable place to start?

I think one important item is a philosophical question, of if malformed data should trigger a panic. Can I interpret the comment above:

"We should try to avoid it, but its not like UB where it would indicate a bug. Panics are just exceptions." as if the fuzzer uncovers a panic, then submitting a patch to avoid the panic would be accepted?

[crepererum]

We've had good experience in parquet-cpp of integrating with google/oss-fuzz I haven't looked into what supporting Rust would look like exactly but that seems like a reasonable place to start?

Before we integrate anything into oss-fuzz, we need to write the fuzz harness. We also need to fix the bugs that occur when you run this for an hour or so on your developer machine (which can be a laptop), just to make sure we sure that we don't end up with SPAM when handing to to oss-fuzz.

Have a look at https://rust-fuzz.github.io/book/introduction.html . Despite what I said in https://github.com/apache/arrow-rs/issues/5332#issuecomment-1910035219 , I think combining that with libFuzzer is still the best choice[^libafl]. It results in a reasonably efficient fuzzer with a somewhat low barrier of entrance. You may want to combine this with arbitrary if you need more than just a few bytes, e.g. you also want to fuzz parameters or something.

I think one important item is a philosophical question, of if malformed data should trigger a panic. Can I interpret the comment above:

"We should try to avoid it, but its not like UB where it would indicate a bug. Panics are just exceptions." as if the fuzzer uncovers a panic, then submitting a patch to avoid the panic would be accepted?

I think there are two levels of safety here:

1. no crash: It can panic, but it should segfault or something. We have some unsafe code in our code base and even with safe code you can still trigger out-of-memory errors (e.g. when the input file header says it's 10000TB of data but the file is only 1kb large, you shouldn't try to allocate a vector/buffer with 10000TB). I think everyone agrees that this is the absolute minimum we have to provide.
2. no panic: Here different developers may have different opinions. I personally think not panicking for a bunch of seemingly random kernel computations (esp. in the DataFusion context) is hard to avoid, but merely deserializing a parquet file to arrow should never panic and only use proper errors.

That said, I would be OK if we start with a fuzz harness that catches panics, stabilize that (e.g. it doesn't crash if you run it on your machine for a while), and then in a follow-up remove the panic-catch and see how far we get.

[^libafl]: I was somewhat hoping that libAFL would evolve a bit faster but you still have to manually build it and last time I've checked -- which was a few months ago -- it was rather complicated to integrate into cargo-fuzz, even with the libFuzzer shim.

[jp0317]

not panicking for a bunch of seemingly random kernel computations (esp. in the DataFusion context) is hard to avoid,

i agree that it's tricky to fully eliminate panics, though fuzz can help reduce panics. IMHO the current codes include many unnecessary panics: e.g., overflow panics due to not using checked operation (such as shift, add, etc.), explicit panics from unimplemented!(), assert!(), panic!().

It seems we can either replace those with proper errors, or maybe just add some sanity checks while keeping the panic codes, e.g., for the bit_width panic we can reject invalid bit_width earlier here such that the panic codes would never get executed)