You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/07/13 17:49:36 UTC
[GitHub] [arrow] pitrou opened a new pull request #7733: ARROW-9439: [C++] Fix crash on invalid IPC input
pitrou opened a new pull request #7733:
URL: https://github.com/apache/arrow/pull/7733
Also add argument-checking variants of SliceBuffer, SliceMutableBuffer, Array::Slice and ArrayData::Slice.
Should fix the following issue:
* https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=24101
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7733: ARROW-9439: [C++] Fix crash on invalid IPC input
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7733:
URL: https://github.com/apache/arrow/pull/7733#issuecomment-657706437
I've checked that this runs fine on Valgrind.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #7733: ARROW-9439: [C++] Fix crash on invalid IPC input
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #7733:
URL: https://github.com/apache/arrow/pull/7733#discussion_r453830282
##########
File path: cpp/src/arrow/buffer.cc
##########
@@ -43,6 +44,56 @@ Result<std::shared_ptr<Buffer>> Buffer::CopySlice(const int64_t start,
return std::move(new_buffer);
}
+namespace {
+
+Status CheckBufferSlice(const Buffer& buffer, int64_t offset, int64_t length) {
Review comment:
Maybe this should just be extracted to `int_util.h`
```c++
Status CheckSlice(int64_t original_length, int64_t slice_offset, int64_t slice_length) {
//...
}
```
then reused for `ArrayData` and `Buffer`.
##########
File path: cpp/src/arrow/array/concatenate.cc
##########
@@ -151,6 +151,10 @@ static Status PutOffsets(const std::shared_ptr<Buffer>& src, Offset first_offset
return Status::Invalid("offset overflow while concatenating arrays");
}
+ // Concatenate can be called during IPC reads to append delta dictionaries,
+ // on non-validate input. Be sure to avoid reading out of buffer boundaries.
+ // XXX
Review comment:
```suggestion
// Concatenate can be called during IPC reads to append delta dictionaries,
// on input which has not been validated. Be sure to avoid reading out of
// buffer boundaries.
// XXX
```
##########
File path: cpp/src/arrow/array/data.cc
##########
@@ -105,6 +106,22 @@ std::shared_ptr<ArrayData> ArrayData::Slice(int64_t off, int64_t len) const {
return copy;
}
+Result<std::shared_ptr<ArrayData>> ArrayData::SliceSafe(int64_t off, int64_t len) const {
+ if (off < 0) {
+ return Status::Invalid("Negative array slice offset");
+ }
+ if (len < 0) {
+ return Status::Invalid("Negative array slice length");
+ }
+ if (internal::HasAdditionOverflow(off, len)) {
+ return Status::Invalid("Array slice would overflow");
+ }
+ if (off + len > length) {
+ return Status::Invalid("Array slice would exceed array length");
+ }
Review comment:
This differs from the behavior of `Slice` which clamps `len` to the available length (so `[1, 3, 2].Slice(2, 2) == [2]`). Is that intentional? If so, please add a comment clarifying the difference
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7733: ARROW-9439: [C++] Fix crash on invalid IPC input
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7733:
URL: https://github.com/apache/arrow/pull/7733#issuecomment-657714890
https://issues.apache.org/jira/browse/ARROW-9439
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #7733: ARROW-9439: [C++] Fix crash on invalid IPC input
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7733:
URL: https://github.com/apache/arrow/pull/7733#discussion_r453832881
##########
File path: cpp/src/arrow/array/data.cc
##########
@@ -105,6 +106,22 @@ std::shared_ptr<ArrayData> ArrayData::Slice(int64_t off, int64_t len) const {
return copy;
}
+Result<std::shared_ptr<ArrayData>> ArrayData::SliceSafe(int64_t off, int64_t len) const {
+ if (off < 0) {
+ return Status::Invalid("Negative array slice offset");
+ }
+ if (len < 0) {
+ return Status::Invalid("Negative array slice length");
+ }
+ if (internal::HasAdditionOverflow(off, len)) {
+ return Status::Invalid("Array slice would overflow");
+ }
+ if (off + len > length) {
+ return Status::Invalid("Array slice would exceed array length");
+ }
Review comment:
Yes, it's intentional.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7733: ARROW-9439: [C++] Fix crash on invalid IPC input
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7733:
URL: https://github.com/apache/arrow/pull/7733#issuecomment-657725054
AppVeyor: https://ci.appveyor.com/project/pitrou/arrow/builds/34070302
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #7733: ARROW-9439: [C++] Fix crash on invalid IPC input
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7733:
URL: https://github.com/apache/arrow/pull/7733#discussion_r453864724
##########
File path: cpp/src/arrow/buffer.cc
##########
@@ -43,6 +44,56 @@ Result<std::shared_ptr<Buffer>> Buffer::CopySlice(const int64_t start,
return std::move(new_buffer);
}
+namespace {
+
+Status CheckBufferSlice(const Buffer& buffer, int64_t offset, int64_t length) {
Review comment:
Can you push the change? Otherwise that will wait until tomorrow.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #7733: ARROW-9439: [C++] Fix crash on invalid IPC input
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #7733:
URL: https://github.com/apache/arrow/pull/7733#discussion_r453851648
##########
File path: cpp/src/arrow/buffer.cc
##########
@@ -43,6 +44,56 @@ Result<std::shared_ptr<Buffer>> Buffer::CopySlice(const int64_t start,
return std::move(new_buffer);
}
+namespace {
+
+Status CheckBufferSlice(const Buffer& buffer, int64_t offset, int64_t length) {
Review comment:
```c++
Status CheckSlice(int64_t original_length, int64_t slice_offset, int64_t slice_length, const char* sliced_type) {
//...
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #7733: ARROW-9439: [C++] Fix crash on invalid IPC input
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7733:
URL: https://github.com/apache/arrow/pull/7733#discussion_r453833175
##########
File path: cpp/src/arrow/buffer.cc
##########
@@ -43,6 +44,56 @@ Result<std::shared_ptr<Buffer>> Buffer::CopySlice(const int64_t start,
return std::move(new_buffer);
}
+namespace {
+
+Status CheckBufferSlice(const Buffer& buffer, int64_t offset, int64_t length) {
Review comment:
I don't know. The logic is the same but the error messages are different.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #7733: ARROW-9439: [C++] Fix crash on invalid IPC input
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7733:
URL: https://github.com/apache/arrow/pull/7733#discussion_r453833318
##########
File path: cpp/src/arrow/array/concatenate.cc
##########
@@ -151,6 +151,10 @@ static Status PutOffsets(const std::shared_ptr<Buffer>& src, Offset first_offset
return Status::Invalid("offset overflow while concatenating arrays");
}
+ // Concatenate can be called during IPC reads to append delta dictionaries,
+ // on non-validate input. Be sure to avoid reading out of buffer boundaries.
+ // XXX
Review comment:
Ha, I fixed that blunder already :-)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #7733: ARROW-9439: [C++] Fix crash on invalid IPC input
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7733:
URL: https://github.com/apache/arrow/pull/7733#discussion_r453835979
##########
File path: cpp/src/arrow/array/data.cc
##########
@@ -105,6 +106,22 @@ std::shared_ptr<ArrayData> ArrayData::Slice(int64_t off, int64_t len) const {
return copy;
}
+Result<std::shared_ptr<ArrayData>> ArrayData::SliceSafe(int64_t off, int64_t len) const {
+ if (off < 0) {
+ return Status::Invalid("Negative array slice offset");
+ }
+ if (len < 0) {
+ return Status::Invalid("Negative array slice length");
+ }
+ if (internal::HasAdditionOverflow(off, len)) {
+ return Status::Invalid("Array slice would overflow");
+ }
+ if (off + len > length) {
+ return Status::Invalid("Array slice would exceed array length");
+ }
Review comment:
Added a comment, thank you :-)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm closed pull request #7733: ARROW-9439: [C++] Fix crash on invalid IPC input
Posted by GitBox <gi...@apache.org>.
wesm closed pull request #7733:
URL: https://github.com/apache/arrow/pull/7733
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #7733: ARROW-9439: [C++] Fix crash on invalid IPC input
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #7733:
URL: https://github.com/apache/arrow/pull/7733#issuecomment-657701477
@github-actions crossbow submit conda-cpp-valgrind
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org