You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pitrou (via GitHub)" <gi...@apache.org> on 2026/04/29 14:29:48 UTC
[PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
pitrou opened a new pull request, #49897:
URL: https://github.com/apache/arrow/pull/49897
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49897:
URL: https://github.com/apache/arrow/pull/49897#issuecomment-4352894648
I've created https://github.com/apache/arrow/issues/49904 for the deprecation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #49897:
URL: https://github.com/apache/arrow/pull/49897
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #49897:
URL: https://github.com/apache/arrow/pull/49897#discussion_r3166524670
##########
cpp/src/arrow/io/interfaces.h:
##########
@@ -279,24 +280,56 @@ class ARROW_EXPORT RandomAccessFile : public InputStream, public Seekable {
///
/// \param[in] position Where to read bytes from
/// \param[in] nbytes The number of bytes to read
+ /// \param[in] allow_short_read Whether to allow reading less than `nbytes`
+ /// \param[out] out The buffer to read bytes into
+ /// \return The number of bytes read, or an error
+ virtual Result<int64_t> ReadAt(int64_t position, int64_t nbytes, bool allow_short_read,
+ void* out);
+
+ /// \brief Read data from given file position.
+ ///
+ /// Like `ReadAt(position, nbytes, allow_short_read, out)` with `allow_short_read`
+ /// set to true.
Review Comment:
Yes, I think it would be safer if they were eventually removed to avoid this cropping up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49897:
URL: https://github.com/apache/arrow/pull/49897#issuecomment-4345690252
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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #49897:
URL: https://github.com/apache/arrow/pull/49897#discussion_r3166569139
##########
cpp/src/arrow/io/interfaces.h:
##########
@@ -279,24 +280,56 @@ class ARROW_EXPORT RandomAccessFile : public InputStream, public Seekable {
///
/// \param[in] position Where to read bytes from
/// \param[in] nbytes The number of bytes to read
+ /// \param[in] allow_short_read Whether to allow reading less than `nbytes`
+ /// \param[out] out The buffer to read bytes into
+ /// \return The number of bytes read, or an error
+ virtual Result<int64_t> ReadAt(int64_t position, int64_t nbytes, bool allow_short_read,
+ void* out);
+
+ /// \brief Read data from given file position.
+ ///
+ /// Like `ReadAt(position, nbytes, allow_short_read, out)` with `allow_short_read`
+ /// set to true.
Review Comment:
(I'll open a separate issue and PR for deprecation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #49897:
URL: https://github.com/apache/arrow/pull/49897#discussion_r3166517526
##########
cpp/src/arrow/io/interfaces.cc:
##########
@@ -149,38 +149,73 @@ RandomAccessFile::~RandomAccessFile() = default;
RandomAccessFile::RandomAccessFile() : interface_impl_(new Impl()) {}
+Result<int64_t> RandomAccessFile::ReadAt(int64_t position, int64_t nbytes,
+ bool allow_short_read, void* out) {
+ ARROW_ASSIGN_OR_RAISE(auto real_nbytes, ReadAt(position, nbytes, out));
+ if (!allow_short_read && real_nbytes != nbytes) {
+ return Status::IOError("File too short: expected to be able to read ", nbytes,
+ " bytes, got ", real_nbytes);
+ }
+ return real_nbytes;
+}
+
Result<int64_t> RandomAccessFile::ReadAt(int64_t position, int64_t nbytes, void* out) {
std::lock_guard<std::mutex> lock(interface_impl_->lock_);
RETURN_NOT_OK(Seek(position));
return Read(nbytes, out);
}
+Result<std::shared_ptr<Buffer>> RandomAccessFile::ReadAt(int64_t position, int64_t nbytes,
+ bool allow_short_read) {
+ ARROW_ASSIGN_OR_RAISE(auto buffer, ReadAt(position, nbytes));
+ // XXX the internal `IoRecordedRandomAccessFile` can return a null buffer
+ if (!allow_short_read && buffer && buffer->size() != nbytes) {
+ return Status::IOError("File too short: expected to be able to read ", nbytes,
+ " bytes, got ", buffer->size());
+ }
Review Comment:
From my reading of the code, `IoRecordedRandomAccessFile::ReadAt` _always_ returns a null buffer unfortunately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49897:
URL: https://github.com/apache/arrow/pull/49897#issuecomment-4345316641
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #49897:
URL: https://github.com/apache/arrow/pull/49897#issuecomment-4353260509
After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit 5a331a9513018141fbfabf94e9331cc98953c353.
None of the specified runs were found on the Conbench server.
The [full Conbench report](https://github.com/apache/arrow/runs/73789481489) has more details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #49897:
URL: https://github.com/apache/arrow/pull/49897#issuecomment-4450626922
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 5a331a9513018141fbfabf94e9331cc98953c353.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/75986648133) has more details. It also includes information about 30 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #49897:
URL: https://github.com/apache/arrow/pull/49897#discussion_r3166534669
##########
cpp/src/arrow/io/interfaces.cc:
##########
@@ -149,38 +149,73 @@ RandomAccessFile::~RandomAccessFile() = default;
RandomAccessFile::RandomAccessFile() : interface_impl_(new Impl()) {}
+Result<int64_t> RandomAccessFile::ReadAt(int64_t position, int64_t nbytes,
+ bool allow_short_read, void* out) {
+ ARROW_ASSIGN_OR_RAISE(auto real_nbytes, ReadAt(position, nbytes, out));
+ if (!allow_short_read && real_nbytes != nbytes) {
+ return Status::IOError("File too short: expected to be able to read ", nbytes,
+ " bytes, got ", real_nbytes);
+ }
+ return real_nbytes;
+}
+
Result<int64_t> RandomAccessFile::ReadAt(int64_t position, int64_t nbytes, void* out) {
std::lock_guard<std::mutex> lock(interface_impl_->lock_);
RETURN_NOT_OK(Seek(position));
return Read(nbytes, out);
}
+Result<std::shared_ptr<Buffer>> RandomAccessFile::ReadAt(int64_t position, int64_t nbytes,
+ bool allow_short_read) {
+ ARROW_ASSIGN_OR_RAISE(auto buffer, ReadAt(position, nbytes));
+ // XXX the internal `IoRecordedRandomAccessFile` can return a null buffer
+ if (!allow_short_read && buffer && buffer->size() != nbytes) {
+ return Status::IOError("File too short: expected to be able to read ", nbytes,
+ " bytes, got ", buffer->size());
+ }
Review Comment:
Ah that's right, it's the implementation that doesn't actually do anything...I suppose ideally we'd refactor to avoid the need for this in the first place but in the meantime we have no choice here then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #49897:
URL: https://github.com/apache/arrow/pull/49897#discussion_r3166503515
##########
cpp/src/arrow/io/interfaces.h:
##########
@@ -279,24 +280,56 @@ class ARROW_EXPORT RandomAccessFile : public InputStream, public Seekable {
///
/// \param[in] position Where to read bytes from
/// \param[in] nbytes The number of bytes to read
+ /// \param[in] allow_short_read Whether to allow reading less than `nbytes`
+ /// \param[out] out The buffer to read bytes into
+ /// \return The number of bytes read, or an error
+ virtual Result<int64_t> ReadAt(int64_t position, int64_t nbytes, bool allow_short_read,
+ void* out);
+
+ /// \brief Read data from given file position.
+ ///
+ /// Like `ReadAt(position, nbytes, allow_short_read, out)` with `allow_short_read`
+ /// set to true.
Review Comment:
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++.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #49897:
URL: https://github.com/apache/arrow/pull/49897#issuecomment-4345343721
Revision: 5ced4d57cad7c5e2a6929fcfda8165e4062e004e
Submitted crossbow builds: [ursacomputing/crossbow @ actions-ccefa1cf50](https://github.com/ursacomputing/crossbow/branches/all?query=actions-ccefa1cf50)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/25119222021/job/73614918658)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/25119220952/job/73614914371)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/25119221040/job/73614915719)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/25119220216/job/73614912608)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/25119221895/job/73614919167)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/25119219561/job/73614910481)|
|test-debian-13-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/25119222150/job/73614920957)|
|test-debian-13-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/25119221517/job/73614916335)|
|test-debian-experimental-cpp-gcc-15|[](https://github.com/ursacomputing/crossbow/actions/runs/25119224427/job/73614928987)|
|test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/25119220482/job/73614913896)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/25119222893/job/73614923262)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/25119223479/job/73614926627)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/25119222084/job/73614918706)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/25119222017/job/73614919748)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/25119221206/job/73614915115)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/25119219835/job/73614911265)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/25119219943/job/73614911352)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/25119219551/job/73614910592)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/25119219741/job/73614910922)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/25119220391/job/73614912822)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49897:
URL: https://github.com/apache/arrow/pull/49897#issuecomment-4345696264
(if I could redo this, the `Read` and `ReadAt` methods would read exactly the given number of bytes by default)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #49897:
URL: https://github.com/apache/arrow/pull/49897#discussion_r3164920167
##########
cpp/src/arrow/io/interfaces.h:
##########
@@ -279,24 +280,56 @@ class ARROW_EXPORT RandomAccessFile : public InputStream, public Seekable {
///
/// \param[in] position Where to read bytes from
/// \param[in] nbytes The number of bytes to read
+ /// \param[in] allow_short_read Whether to allow reading less than `nbytes`
+ /// \param[out] out The buffer to read bytes into
+ /// \return The number of bytes read, or an error
+ virtual Result<int64_t> ReadAt(int64_t position, int64_t nbytes, bool allow_short_read,
+ void* out);
+
+ /// \brief Read data from given file position.
+ ///
+ /// Like `ReadAt(position, nbytes, allow_short_read, out)` with `allow_short_read`
+ /// set to true.
Review Comment:
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)
##########
cpp/src/arrow/io/interfaces.cc:
##########
@@ -149,38 +149,73 @@ RandomAccessFile::~RandomAccessFile() = default;
RandomAccessFile::RandomAccessFile() : interface_impl_(new Impl()) {}
+Result<int64_t> RandomAccessFile::ReadAt(int64_t position, int64_t nbytes,
+ bool allow_short_read, void* out) {
+ ARROW_ASSIGN_OR_RAISE(auto real_nbytes, ReadAt(position, nbytes, out));
+ if (!allow_short_read && real_nbytes != nbytes) {
+ return Status::IOError("File too short: expected to be able to read ", nbytes,
+ " bytes, got ", real_nbytes);
+ }
+ return real_nbytes;
+}
+
Result<int64_t> RandomAccessFile::ReadAt(int64_t position, int64_t nbytes, void* out) {
std::lock_guard<std::mutex> lock(interface_impl_->lock_);
RETURN_NOT_OK(Seek(position));
return Read(nbytes, out);
}
+Result<std::shared_ptr<Buffer>> RandomAccessFile::ReadAt(int64_t position, int64_t nbytes,
+ bool allow_short_read) {
+ ARROW_ASSIGN_OR_RAISE(auto buffer, ReadAt(position, nbytes));
+ // XXX the internal `IoRecordedRandomAccessFile` can return a null buffer
+ if (!allow_short_read && buffer && buffer->size() != nbytes) {
+ return Status::IOError("File too short: expected to be able to read ", nbytes,
+ " bytes, got ", buffer->size());
+ }
Review Comment:
If the buffer is null and nbytes is > 0 then we should also fail right? Or is it only possible for nbytes == 0 in the first place?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49896: [C++] Reject short buffer reads in IPC reader [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #49897:
URL: https://github.com/apache/arrow/pull/49897#discussion_r3166537395
##########
cpp/src/arrow/io/interfaces.h:
##########
@@ -279,24 +280,56 @@ class ARROW_EXPORT RandomAccessFile : public InputStream, public Seekable {
///
/// \param[in] position Where to read bytes from
/// \param[in] nbytes The number of bytes to read
+ /// \param[in] allow_short_read Whether to allow reading less than `nbytes`
+ /// \param[out] out The buffer to read bytes into
+ /// \return The number of bytes read, or an error
+ virtual Result<int64_t> ReadAt(int64_t position, int64_t nbytes, bool allow_short_read,
+ void* out);
+
+ /// \brief Read data from given file position.
+ ///
+ /// Like `ReadAt(position, nbytes, allow_short_read, out)` with `allow_short_read`
+ /// set to true.
Review Comment:
Ah sorry I missed your comment above. Yes, I agree, we should do that in a later PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org