Panic on invalid flatbuffers

[jorgecarleitao]

I am running a penetration test on arrow2 to ensure that we do not panic or abort on invalid data from flatbuffers, and I am hitting a panic here.

Somehow we are reaching this expect.

How to reproduce it:

git clone [email protected]:jorgecarleitao/arrow2.git
cd arrow2

cargo run --features io_ipc,io_print --example ipc_file_read invalid.zip

where invalid.zip is attached to this issue (it is not a zip file, but it makes github accept it).

I am sorry I am not very familiar with planus to be able to reproduce this in a smaller example without arrow2. If you provide some guides I can try to do it.

invalid.zip

[TethysSvensson]

First of all: we're extremely grateful that you are testing our library. How did you find the example? Did you use fuzzing?

Regarding the bug: this doesn't look like something that should be an IMPOSSIBLE. It looks like it should be an Err. A git blame on the file says, that it's been there since we made the repo public, so I don't currently have any context of why it was added.

At some point I would like for either me or @kristoff3r to take a look in the internal history to see if there is any reason for it being there.

I'm not expecting to find anything though and I think the resolution to the issue is too convert it to a suitable error no matter what we find.

Unfortunately I'm on vacation without a computer right now, so I won't have time to create a PR until July. Perhaps Kris will have time in the coming week. If that doesn't work for you, also feel free to create one yourself and I will accept it.

[TethysSvensson]

I would prefer if we keep this PR open until we've:

• Fixed the bug
• Investigated why the line was introduced initially
• Added a test what would have caught this

[jorgecarleitao]

I can't address the 2nd item (imo it is not so relevant, anyways ^^), but 1 and 3 are in place on #100

[TethysSvensson]

I found out I brought my laptop anyways 😅

I checked the internal git history now and there is no good reason that this line was introduced.

I am working on adding a test for this and similar cases.

[TethysSvensson]

@jorgecarleitao I think #102 should address the issue. Let me know if it doesn't.

Btw, how did you find the bug?

[jorgecarleitao]

Btw, how did you find the bug?

Something along the lines of

#[test]
fn test_does_not_panic() {
    use rand::Rng; // 0.8.0

    let original: Vec<u8> = [valid arrow file];

    for errors in 0..1000 {
        let mut data = original.clone();
        for _ in 0..errors {
            let position: usize = rand::thread_rng().gen_range(0..data.len());
            let new_byte: u8 = rand::thread_rng().gen_range(0..u8::MAX);
            data[position] = new_byte
        }
        std::fs::write("invalid.arrow", &data);
        let _ = read_corrupted_ipc(data);
    }
}

i.e. permute random bytes from a valid file and confirm that we do not panic.

[jorgecarleitao]

@TethysSvensson , thanks a lot for the fast response and fix!

[jorgecarleitao]

Would it be possible to release a patch on 0.2 or 0.3? This is the last bit we need to claim a panic free read on IPC files in arrow2 :P

[kristoff3r]

@jorgecarleitao I'll get the docs PR merged and do a release tomorrow