ARROW-5605: [C++] Verify Flatbuffer messages in more places to prevent crashes due to bad inputs

[crepererum]

While the first commit (fix ReadRecordBatch validation) is sufficient to fix ARROW-5605, I took the time to fix very similar issues in the code IPC code base, so our users are probably protected and also to help the fuzzer to not run straight into a similar problem again.

[wesm]

Only really DRY issues. Have you run arrow-ipc-read-write-benchmark to assess performance changes if any?

[wesm]

Can you factor this into a helper function to avoid code duplication?

[crepererum]

helper function doesn't work due to the early IOError return

[crepererum]

And honestly I was not able to find a nice macro that does this. The issue is basically that you would need to cover 2 things: the early exit and the assignment to message_ (or another variable)

[wesm]

This assertion about the message type could be handled more generically (since we can pass in the expected union value), then this helps with code duplication

[crepererum]

And I have the same issue here: how to pull that code out that it includes the assignment AND the return?

[pitrou]

Do we have a doc on how to run fuzzing builds somewhere?

[wesm]

https://github.com/apache/arrow/blob/master/docs/source/developers/cpp.rst#fuzz-testing-with-libfuzzer

[wesm]

@crepererum will you have time this week to look into my comments? Would be great to get this into 0.14.0

[crepererum]

Is Friday sufficient? If not, I can try to get it through tomorrow, but that's nothing I can promise.

[wesm]

Sure, that's fine

[crepererum]

For the benchmarks:

Before:

Running ./release/arrow-ipc-read-write-benchmark
Run on (4 X 2700 MHz CPU s)
CPU Caches:
  L1 Data 32K (x2)
  L1 Instruction 32K (x2)
  L2 Unified 256K (x2)
  L3 Unified 3072K (x1)
-----------------------------------------------------------------------
Benchmark                                Time           CPU Iterations
-----------------------------------------------------------------------
WriteRecordBatch/1/real_time         79490 ns      79344 ns       8796   12.2853GB/s
WriteRecordBatch/4/real_time         83765 ns      83509 ns       8059   11.6584GB/s
WriteRecordBatch/16/real_time        85620 ns      85374 ns       7566   11.4057GB/s
WriteRecordBatch/64/real_time       101110 ns     100823 ns       7265   9.65839GB/s
WriteRecordBatch/256/real_time      153011 ns     152772 ns       4563   6.38229GB/s
WriteRecordBatch/1024/real_time     414444 ns     413206 ns       1680   2.35632GB/s
WriteRecordBatch/4096/real_time    1554502 ns    1549857 ns        448   643.293MB/s
WriteRecordBatch/8192/real_time    3817506 ns    3798506 ns        183   261.951MB/s
ReadRecordBatch/1/real_time            790 ns        789 ns     875702    1.2072TB/s
ReadRecordBatch/4/real_time           1595 ns       1593 ns     437008    612.43GB/s
ReadRecordBatch/16/real_time          4878 ns       4874 ns     141866   200.187GB/s
ReadRecordBatch/64/real_time         18741 ns      18728 ns      37061   52.1076GB/s
ReadRecordBatch/256/real_time        94392 ns      94323 ns       7445   10.3458GB/s
ReadRecordBatch/1024/real_time      395453 ns     394916 ns       1795   2.46948GB/s
ReadRecordBatch/4096/real_time     1641739 ns    1638249 ns        428    609.11MB/s
ReadRecordBatch/8192/real_time     4639762 ns    4628376 ns        151   215.528MB/s

After:

2019-06-21 14:38:49
Running ./release/arrow-ipc-read-write-benchmark
Run on (4 X 2700 MHz CPU s)
CPU Caches:
  L1 Data 32K (x2)
  L1 Instruction 32K (x2)
  L2 Unified 256K (x2)
  L3 Unified 3072K (x1)
-----------------------------------------------------------------------
Benchmark                                Time           CPU Iterations
-----------------------------------------------------------------------
WriteRecordBatch/1/real_time         84217 ns      84065 ns       8143   11.5958GB/s
WriteRecordBatch/4/real_time         80919 ns      80799 ns       8483   12.0684GB/s
WriteRecordBatch/16/real_time        86917 ns      86746 ns       7794   11.2355GB/s
WriteRecordBatch/64/real_time       105813 ns     105620 ns       6361   9.22917GB/s
WriteRecordBatch/256/real_time      156334 ns     156052 ns       4498   6.24666GB/s
WriteRecordBatch/1024/real_time     430898 ns     430019 ns       1600   2.26634GB/s
WriteRecordBatch/4096/real_time    1562643 ns    1557517 ns        444   639.941MB/s
WriteRecordBatch/8192/real_time    3787067 ns    3776088 ns        184   264.057MB/s
ReadRecordBatch/1/real_time            871 ns        870 ns     809603   1121.07GB/s
ReadRecordBatch/4/real_time           1695 ns       1693 ns     413385   576.301GB/s
ReadRecordBatch/16/real_time          5131 ns       5128 ns     135990   190.322GB/s
ReadRecordBatch/64/real_time         19336 ns      19323 ns      36370   50.5058GB/s
ReadRecordBatch/256/real_time        94685 ns      94597 ns       7232   10.3138GB/s
ReadRecordBatch/1024/real_time      391960 ns     391594 ns       1776   2.49149GB/s
ReadRecordBatch/4096/real_time     1632510 ns    1627994 ns        429   612.554MB/s
ReadRecordBatch/8192/real_time     4567485 ns    4555617 ns        152   218.939MB/s

I have to admit though that the benchmarks are extremely flaky, even with benchmark_min_time increased to 10.0. So not sure what to read from that. I think we don't have to expect a noticeable regression here.

[wesm]

I'm taking care of the refactoring. Done shortly