You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2023/03/28 17:33:41 UTC
[GitHub] [arrow-nanoarrow] paleolimbot opened a new pull request, #174: feat: Implement `ArrowArrayViewValidateFull()`
paleolimbot opened a new pull request, #174:
URL: https://github.com/apache/arrow-nanoarrow/pull/174
This is required for IPC reading because corrupted offset and/or union type ID buffers could result in consumers accessing out-of-bounds elements. `ArrowArrayViewSetArray()` already checked the *last* element of offset buffers against lengths but didn't check the first element and didn't check for negative sequential offsets.
TODO: Unions 🔥 🔥 🔥
--
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
[GitHub] [arrow-nanoarrow] wjones127 commented on a diff in pull request #174: feat: Implement `ArrowArrayViewValidateFull()`
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #174:
URL: https://github.com/apache/arrow-nanoarrow/pull/174#discussion_r1151263085
##########
src/nanoarrow/array.c:
##########
@@ -758,3 +804,138 @@ ArrowErrorCode ArrowArrayViewSetArray(struct ArrowArrayView* array_view,
return NANOARROW_OK;
}
+
+static int ArrowAssertIncreasingInt32(struct ArrowBufferView view,
+ struct ArrowError* error) {
+ if (view.size_bytes <= (int64_t)sizeof(int32_t)) {
+ return NANOARROW_OK;
+ }
+
+ for (int64_t i = 1; i < view.size_bytes / (int64_t)sizeof(int32_t); i++) {
+ int32_t diff = view.data.as_int32[i] - view.data.as_int32[i - 1];
+ if (diff < 0) {
+ ArrowErrorSet(error,
+ "Expected element size >0 but found element size %ld at position %ld",
+ (long)diff, (long)i);
+ return EINVAL;
+ }
+ }
+
+ return NANOARROW_OK;
+}
+
+static int ArrowAssertIncreasingInt64(struct ArrowBufferView view,
+ struct ArrowError* error) {
+ if (view.size_bytes <= (int64_t)sizeof(int64_t)) {
+ return NANOARROW_OK;
+ }
+
+ for (int64_t i = 1; i < view.size_bytes / (int64_t)sizeof(int64_t); i++) {
+ int64_t diff = view.data.as_int64[i] - view.data.as_int64[i - 1];
+ if (diff < 0) {
+ ArrowErrorSet(error,
+ "Expected element size >0 but found element size %ld at position %ld",
+ (long)diff, (long)i);
+ return EINVAL;
+ }
+ }
+
+ return NANOARROW_OK;
+}
+
+static int ArrowAssertRangeInt8(struct ArrowBufferView view, int8_t min_value,
+ int8_t max_value, struct ArrowError* error) {
+ for (int64_t i = 0; i < view.size_bytes; i++) {
+ if (view.data.as_int8[i] < min_value || view.data.as_int8[i] > max_value) {
+ ArrowErrorSet(
+ error,
+ "Expected buffer value between %d and %d but found value %d at position %ld",
+ (int)min_value, (int)max_value, (int)view.data.as_int8[i], (long)i);
+ return EINVAL;
+ }
+ }
+
+ return NANOARROW_OK;
+}
+
+static int ArrowAssertInt8In(struct ArrowBufferView view, const int8_t* values,
+ int64_t n_values, struct ArrowError* error) {
+ for (int64_t i = 0; i < view.size_bytes; i++) {
+ int item_found = 0;
Review Comment:
😮
--
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
[GitHub] [arrow-nanoarrow] wjones127 commented on a diff in pull request #174: feat: Implement `ArrowArrayViewValidateFull()`
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #174:
URL: https://github.com/apache/arrow-nanoarrow/pull/174#discussion_r1151193559
##########
src/nanoarrow/nanoarrow_types.h:
##########
@@ -503,7 +503,8 @@ struct ArrowArrayView {
///
/// If storage_type is a union type, a 256-byte ArrowMalloc()ed buffer
/// such that child_index == union_type_id_map[type_id] and
- /// type_id == union_type_id_map[128 + child_index]
+ /// type_id == union_type_id_map[128 + child_index]. This value may be
+ /// NULL in the case where child_id == type_id.
Review Comment:
Is that part of the Arrow format? I couldn't find such a detail.
##########
src/nanoarrow/array.c:
##########
@@ -648,15 +663,25 @@ ArrowErrorCode ArrowArrayViewSetArray(struct ArrowArrayView* array_view,
}
if (array_view->n_children != array->n_children) {
+ ArrowErrorSet(error, "Expected %ld children but found %ld children",
+ (long)array_view->n_children, (long)array->n_children);
return EINVAL;
}
// Check child sizes and calculate sizes that depend on data in the array buffers
+ int64_t first_offset;
int64_t last_offset;
switch (array_view->storage_type) {
case NANOARROW_TYPE_STRING:
case NANOARROW_TYPE_BINARY:
if (array_view->buffer_views[1].size_bytes != 0) {
+ first_offset = array_view->buffer_views[1].data.as_int32[0];
+ if (first_offset < 0) {
+ ArrowErrorSet(error, "Expected first offset >0 but found %ld",
Review Comment:
```suggestion
ArrowErrorSet(error, "Expected first offset >=0 but found %ld",
```
##########
src/nanoarrow/array.c:
##########
@@ -665,6 +690,13 @@ ArrowErrorCode ArrowArrayViewSetArray(struct ArrowArrayView* array_view,
case NANOARROW_TYPE_LARGE_STRING:
case NANOARROW_TYPE_LARGE_BINARY:
if (array_view->buffer_views[1].size_bytes != 0) {
+ first_offset = array_view->buffer_views[1].data.as_int64[0];
+ if (first_offset < 0) {
+ ArrowErrorSet(error, "Expected first offset >0 but found %ld",
Review Comment:
And similarly in other places
##########
src/nanoarrow/array.c:
##########
@@ -758,3 +804,138 @@ ArrowErrorCode ArrowArrayViewSetArray(struct ArrowArrayView* array_view,
return NANOARROW_OK;
}
+
+static int ArrowAssertIncreasingInt32(struct ArrowBufferView view,
+ struct ArrowError* error) {
+ if (view.size_bytes <= (int64_t)sizeof(int32_t)) {
+ return NANOARROW_OK;
+ }
+
+ for (int64_t i = 1; i < view.size_bytes / (int64_t)sizeof(int32_t); i++) {
+ int32_t diff = view.data.as_int32[i] - view.data.as_int32[i - 1];
+ if (diff < 0) {
+ ArrowErrorSet(error,
+ "Expected element size >0 but found element size %ld at position %ld",
+ (long)diff, (long)i);
+ return EINVAL;
+ }
+ }
+
+ return NANOARROW_OK;
+}
+
+static int ArrowAssertIncreasingInt64(struct ArrowBufferView view,
+ struct ArrowError* error) {
+ if (view.size_bytes <= (int64_t)sizeof(int64_t)) {
+ return NANOARROW_OK;
+ }
+
+ for (int64_t i = 1; i < view.size_bytes / (int64_t)sizeof(int64_t); i++) {
+ int64_t diff = view.data.as_int64[i] - view.data.as_int64[i - 1];
+ if (diff < 0) {
+ ArrowErrorSet(error,
+ "Expected element size >0 but found element size %ld at position %ld",
+ (long)diff, (long)i);
+ return EINVAL;
+ }
+ }
+
+ return NANOARROW_OK;
+}
+
+static int ArrowAssertRangeInt8(struct ArrowBufferView view, int8_t min_value,
+ int8_t max_value, struct ArrowError* error) {
+ for (int64_t i = 0; i < view.size_bytes; i++) {
+ if (view.data.as_int8[i] < min_value || view.data.as_int8[i] > max_value) {
+ ArrowErrorSet(
+ error,
+ "Expected buffer value between %d and %d but found value %d at position %ld",
+ (int)min_value, (int)max_value, (int)view.data.as_int8[i], (long)i);
+ return EINVAL;
+ }
+ }
+
+ return NANOARROW_OK;
+}
+
+static int ArrowAssertInt8In(struct ArrowBufferView view, const int8_t* values,
+ int64_t n_values, struct ArrowError* error) {
+ for (int64_t i = 0; i < view.size_bytes; i++) {
+ int item_found = 0;
Review Comment:
Question: why `int` over `bool`?
##########
src/nanoarrow/array.c:
##########
@@ -665,6 +690,13 @@ ArrowErrorCode ArrowArrayViewSetArray(struct ArrowArrayView* array_view,
case NANOARROW_TYPE_LARGE_STRING:
case NANOARROW_TYPE_LARGE_BINARY:
if (array_view->buffer_views[1].size_bytes != 0) {
+ first_offset = array_view->buffer_views[1].data.as_int64[0];
+ if (first_offset < 0) {
+ ArrowErrorSet(error, "Expected first offset >0 but found %ld",
Review Comment:
```suggestion
ArrowErrorSet(error, "Expected first offset >=0 but found %ld",
```
--
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
[GitHub] [arrow-nanoarrow] paleolimbot commented on a diff in pull request #174: feat: Implement `ArrowArrayViewValidateFull()`
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #174:
URL: https://github.com/apache/arrow-nanoarrow/pull/174#discussion_r1151298323
##########
src/nanoarrow/nanoarrow_types.h:
##########
@@ -503,7 +503,8 @@ struct ArrowArrayView {
///
/// If storage_type is a union type, a 256-byte ArrowMalloc()ed buffer
/// such that child_index == union_type_id_map[type_id] and
- /// type_id == union_type_id_map[128 + child_index]
+ /// type_id == union_type_id_map[128 + child_index]. This value may be
+ /// NULL in the case where child_id == type_id.
Review Comment:
This is an implementation detail of the `ArrowArrayView`...in the spec the type ids are always there. Arguably they should always be here, too, but there's at least one test that exploits that behaviour that I discovered so I figured I should probably document it. I think the reason it ended up that way was because you can `ArrowArrayViewInitFromType()` and `ArrowArrayViewAllocateChildren()` and letting the type id map stay NULL saves some special casing of unions there.
--
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
[GitHub] [arrow-nanoarrow] codecov-commenter commented on pull request #174: feat: Implement `ArrowArrayViewValidateFull()`
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #174:
URL: https://github.com/apache/arrow-nanoarrow/pull/174#issuecomment-1487350617
## [Codecov](https://codecov.io/gh/apache/arrow-nanoarrow/pull/174?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> :exclamation: No coverage uploaded for pull request base (`main@bebb790`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#section-missing-base-commit).
> The diff coverage is `15.38%`.
```diff
@@ Coverage Diff @@
## main #174 +/- ##
=======================================
Coverage ? 89.74%
=======================================
Files ? 7
Lines ? 1716
Branches ? 53
=======================================
Hits ? 1540
Misses ? 144
Partials ? 32
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-nanoarrow/pull/174?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [src/nanoarrow/array.c](https://codecov.io/gh/apache/arrow-nanoarrow/pull/174?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL25hbm9hcnJvdy9hcnJheS5j) | `78.66% <15.38%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [arrow-nanoarrow] paleolimbot merged pull request #174: feat: Implement `ArrowArrayViewValidateFull()`
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #174:
URL: https://github.com/apache/arrow-nanoarrow/pull/174
--
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
[GitHub] [arrow-nanoarrow] paleolimbot commented on a diff in pull request #174: feat: Implement `ArrowArrayViewValidateFull()`
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #174:
URL: https://github.com/apache/arrow-nanoarrow/pull/174#discussion_r1151308539
##########
src/nanoarrow/array.c:
##########
@@ -758,3 +804,138 @@ ArrowErrorCode ArrowArrayViewSetArray(struct ArrowArrayView* array_view,
return NANOARROW_OK;
}
+
+static int ArrowAssertIncreasingInt32(struct ArrowBufferView view,
+ struct ArrowError* error) {
+ if (view.size_bytes <= (int64_t)sizeof(int32_t)) {
+ return NANOARROW_OK;
+ }
+
+ for (int64_t i = 1; i < view.size_bytes / (int64_t)sizeof(int32_t); i++) {
+ int32_t diff = view.data.as_int32[i] - view.data.as_int32[i - 1];
+ if (diff < 0) {
+ ArrowErrorSet(error,
+ "Expected element size >0 but found element size %ld at position %ld",
+ (long)diff, (long)i);
+ return EINVAL;
+ }
+ }
+
+ return NANOARROW_OK;
+}
+
+static int ArrowAssertIncreasingInt64(struct ArrowBufferView view,
+ struct ArrowError* error) {
+ if (view.size_bytes <= (int64_t)sizeof(int64_t)) {
+ return NANOARROW_OK;
+ }
+
+ for (int64_t i = 1; i < view.size_bytes / (int64_t)sizeof(int64_t); i++) {
+ int64_t diff = view.data.as_int64[i] - view.data.as_int64[i - 1];
+ if (diff < 0) {
+ ArrowErrorSet(error,
+ "Expected element size >0 but found element size %ld at position %ld",
+ (long)diff, (long)i);
+ return EINVAL;
+ }
+ }
+
+ return NANOARROW_OK;
+}
+
+static int ArrowAssertRangeInt8(struct ArrowBufferView view, int8_t min_value,
+ int8_t max_value, struct ArrowError* error) {
+ for (int64_t i = 0; i < view.size_bytes; i++) {
+ if (view.data.as_int8[i] < min_value || view.data.as_int8[i] > max_value) {
+ ArrowErrorSet(
+ error,
+ "Expected buffer value between %d and %d but found value %d at position %ld",
+ (int)min_value, (int)max_value, (int)view.data.as_int8[i], (long)i);
+ return EINVAL;
+ }
+ }
+
+ return NANOARROW_OK;
+}
+
+static int ArrowAssertInt8In(struct ArrowBufferView view, const int8_t* values,
+ int64_t n_values, struct ArrowError* error) {
+ for (int64_t i = 0; i < view.size_bytes; i++) {
+ int item_found = 0;
Review Comment:
There's `#include <stdbool.h>` too, which defines macros for `bool`, `true`, and `false`. I didn't use it initially because there are some functions that return true/false in public headers and I wanted as few includes there as possible. I should probably go through and make it consistent (I may have used `char` in some places).
--
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
[GitHub] [arrow-nanoarrow] lidavidm commented on a diff in pull request #174: feat: Implement `ArrowArrayViewValidateFull()`
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #174:
URL: https://github.com/apache/arrow-nanoarrow/pull/174#discussion_r1151110697
##########
src/nanoarrow/array.c:
##########
@@ -758,3 +804,138 @@ ArrowErrorCode ArrowArrayViewSetArray(struct ArrowArrayView* array_view,
return NANOARROW_OK;
}
+
+static int ArrowAssertIncreasingInt32(struct ArrowBufferView view,
+ struct ArrowError* error) {
+ if (view.size_bytes <= (int64_t)sizeof(int32_t)) {
+ return NANOARROW_OK;
+ }
+
+ for (int64_t i = 1; i < view.size_bytes / (int64_t)sizeof(int32_t); i++) {
+ int32_t diff = view.data.as_int32[i] - view.data.as_int32[i - 1];
+ if (diff < 0) {
+ ArrowErrorSet(error,
+ "Expected element size >0 but found element size %ld at position %ld",
+ (long)diff, (long)i);
Review Comment:
nit: the message is kind of a non-sequitur for the error case here
--
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
[GitHub] [arrow-nanoarrow] lidavidm commented on a diff in pull request #174: feat: Implement `ArrowArrayViewValidateFull()`
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #174:
URL: https://github.com/apache/arrow-nanoarrow/pull/174#discussion_r1151239964
##########
src/nanoarrow/array.c:
##########
@@ -758,3 +804,138 @@ ArrowErrorCode ArrowArrayViewSetArray(struct ArrowArrayView* array_view,
return NANOARROW_OK;
}
+
+static int ArrowAssertIncreasingInt32(struct ArrowBufferView view,
+ struct ArrowError* error) {
+ if (view.size_bytes <= (int64_t)sizeof(int32_t)) {
+ return NANOARROW_OK;
+ }
+
+ for (int64_t i = 1; i < view.size_bytes / (int64_t)sizeof(int32_t); i++) {
+ int32_t diff = view.data.as_int32[i] - view.data.as_int32[i - 1];
+ if (diff < 0) {
+ ArrowErrorSet(error,
+ "Expected element size >0 but found element size %ld at position %ld",
+ (long)diff, (long)i);
+ return EINVAL;
+ }
+ }
+
+ return NANOARROW_OK;
+}
+
+static int ArrowAssertIncreasingInt64(struct ArrowBufferView view,
+ struct ArrowError* error) {
+ if (view.size_bytes <= (int64_t)sizeof(int64_t)) {
+ return NANOARROW_OK;
+ }
+
+ for (int64_t i = 1; i < view.size_bytes / (int64_t)sizeof(int64_t); i++) {
+ int64_t diff = view.data.as_int64[i] - view.data.as_int64[i - 1];
+ if (diff < 0) {
+ ArrowErrorSet(error,
+ "Expected element size >0 but found element size %ld at position %ld",
+ (long)diff, (long)i);
+ return EINVAL;
+ }
+ }
+
+ return NANOARROW_OK;
+}
+
+static int ArrowAssertRangeInt8(struct ArrowBufferView view, int8_t min_value,
+ int8_t max_value, struct ArrowError* error) {
+ for (int64_t i = 0; i < view.size_bytes; i++) {
+ if (view.data.as_int8[i] < min_value || view.data.as_int8[i] > max_value) {
+ ArrowErrorSet(
+ error,
+ "Expected buffer value between %d and %d but found value %d at position %ld",
+ (int)min_value, (int)max_value, (int)view.data.as_int8[i], (long)i);
+ return EINVAL;
+ }
+ }
+
+ return NANOARROW_OK;
+}
+
+static int ArrowAssertInt8In(struct ArrowBufferView view, const int8_t* values,
+ int64_t n_values, struct ArrowError* error) {
+ for (int64_t i = 0; i < view.size_bytes; i++) {
+ int item_found = 0;
Review Comment:
There's not really bool in C (there is _Bool, I suppose with C99 we can use that and I guess we're assuming C99 here?)
--
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
[GitHub] [arrow-nanoarrow] paleolimbot commented on a diff in pull request #174: feat: Implement `ArrowArrayViewValidateFull()`
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #174:
URL: https://github.com/apache/arrow-nanoarrow/pull/174#discussion_r1151305160
##########
src/nanoarrow/array.c:
##########
@@ -758,3 +804,138 @@ ArrowErrorCode ArrowArrayViewSetArray(struct ArrowArrayView* array_view,
return NANOARROW_OK;
}
+
+static int ArrowAssertIncreasingInt32(struct ArrowBufferView view,
+ struct ArrowError* error) {
+ if (view.size_bytes <= (int64_t)sizeof(int32_t)) {
+ return NANOARROW_OK;
+ }
+
+ for (int64_t i = 1; i < view.size_bytes / (int64_t)sizeof(int32_t); i++) {
+ int32_t diff = view.data.as_int32[i] - view.data.as_int32[i - 1];
+ if (diff < 0) {
+ ArrowErrorSet(error,
+ "Expected element size >0 but found element size %ld at position %ld",
+ (long)diff, (long)i);
Review Comment:
I took another pass at making them all a bit better!
--
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