GH-49896: [C++] Reject short buffer reads in IPC reader

[pitrou]

Rationale for this change

IO methods like ReadAt can return less bytes than asked for if the file is too short, but the IPC reader doesn't always detect for this situation. On invalid IPC files, this can produce issues down the road such as half-initialized buffers and large processing times (with a potential denial of service).

This issue was detected by OSS-Fuzz: https://issues.oss-fuzz.com/issues/489758017

What changes are included in this PR?

1. Add ReadAt and ReadAsync overloads that accept a bool allow_short_read argument
2. Pass allow_short_read = false in all suitable places in IPC and Parquet readers

Are these changes tested?

Yes, by existing tests and new fuzz regression file.

Are there any user-facing changes?

No, except potentially better detection of invalid IPC streams and files.

• GitHub Issue: [C++] IPC reader does not always check for short reads #49896

[pitrou]

@github-actions crossbow submit -g cpp

[github-actions]

Revision: 5ced4d5

Submitted crossbow builds: ursacomputing/crossbow @ actions-ccefa1cf50

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-debian-13-cpp-amd64
test-debian-13-cpp-i386
test-debian-experimental-cpp-gcc-15
test-fedora-42-cpp
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

[pitrou]

This is adding to the number of virtual methods and overloads in the IO interfaces. I think we could deprecate the legacy ReadAt and ReadAsync that don't take a bool allow_short_read argument in a later PR. Thoughts @lidavidm @WillAyd @zanmato1984 ?

[pitrou]

(if I could redo this, the ReadAt and ReadAsync methods would read exactly the given number of bytes by default)

[lidavidm]

I wonder if we should deprecate these overloads over time (it feels like it would be safer to have allow_short_read be the opt-in rather than opt-out behavior at least)

[pitrou]

By "these overloads", you mean those without the allow_short_read parameter, right?

And, yes, I agree that disallowing short reads by default would definitely be safer. Short reads by default is fine in a "safe" language like Python, not so much in C++.

[lidavidm]

Yes, I think it would be safer if they were eventually removed to avoid this cropping up.

[lidavidm]

Ah sorry I missed your comment above. Yes, I agree, we should do that in a later PR.

[pitrou]

(I'll open a separate issue and PR for deprecation)

[pitrou]

I've created #49904 for the deprecation.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit 5a331a9.

None of the specified runs were found on the Conbench server.

The full Conbench report has more details.