You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "ump45nose (via GitHub)" <gi...@apache.org> on 2026/10/21 21:00:37 UTC
[PR] fix: Avoid implicit 64-to-32-bit conversions on 32-bit platforms [arrow-nanoarrow]
ump45nose opened a new pull request, #983:
URL: https://github.com/apache/arrow-nanoarrow/pull/983
Closes #797.
Building nanoarrow with `-Wconversion` on a 32-bit target (the armv7 Android build in the vcpkg CI from the issue) fails with `-Wshorten-64-to-32` errors, because a number of `int64_t` sizes are passed straight to functions that take `size_t`: `memcpy`/`memset`/`memmove`, `snprintf`, `malloc`/`realloc`, `fread`/`fwrite`, `strncmp`, and the flatcc builder/vector helpers.
This adds explicit `(size_t)` casts at those call sites in `common/`, `device/device.c` and `ipc/`. There's no behaviour change on 64-bit platforms. In `ArrowArrayViewCompareBuffer()` the `size_bytes > 0` check stays on the signed value and only the length handed to `memcmp()` is converted, so the stringified comparison message the tests check doesn't change.
### Testing
- `clang -m32 -std=gnu99 -fsyntax-only -Wall -Wextra -Wpedantic -Wconversion -Wno-sign-conversion` (clang 19, i386) over `src/nanoarrow/common/*.c`, `device/device.c` and `ipc/{codecs,decoder,encoder,reader,writer}.c`: 40 distinct `-Wshorten-64-to-32` warning locations on `main`, none with this change. I don't have the Android NDK setup locally, so this is a 32-bit x86 stand-in for the armv7 build.
- Native 64-bit Debug build with `NANOARROW_BUILD_TESTS=ON NANOARROW_IPC=ON NANOARROW_DEVICE=ON`: `ctest` 422/422 passed.
- clang-format 16.0.6 (the pre-commit version) applied.
This PR was prepared with AI assistance; I reviewed the changes and ran the checks above.
--
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] fix: Avoid implicit 64-to-32-bit conversions on 32-bit platforms [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #983:
URL: https://github.com/apache/arrow-nanoarrow/pull/983#issuecomment-6366355692
Thank you!
It looks like CI has a few complaints, but once those are resolved I think this is good to go.
--
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] fix: Avoid implicit 64-to-32-bit conversions on 32-bit platforms [arrow-nanoarrow]
Posted by "ump45nose (via GitHub)" <gi...@apache.org>.
ump45nose commented on PR #983:
URL: https://github.com/apache/arrow-nanoarrow/pull/983#issuecomment-6368320018
Thanks @paleolimbot! I looked at the two failing jobs:
- **clang-tidy**: all eight warnings are in `src/nanoarrow/integration/ipc_integration.cc` and `src/nanoarrow/hpp/view_test.cc` (`clang-analyzer-unix.Stream` / `clang-analyzer-security.ArrayBound`), neither of which this PR touches. clang-tidy passed on the last `main` run (2026-10-22), so this looks like a newer analyzer in the runner image rather than something from this diff; happy to fix those warnings in a separate PR if you would like.
- **meson-build**: the same job also fails on the latest `main` run of `test-c`, so it is pre-existing.
Let me know if you see anything in the diff itself that needs changing. (AI-assisted reply.)
--
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] fix: Avoid implicit 64-to-32-bit conversions on 32-bit platforms [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #983:
URL: https://github.com/apache/arrow-nanoarrow/pull/983#discussion_r4349403663
##########
src/nanoarrow/common/utils.c:
##########
@@ -237,9 +237,9 @@ void ArrowLayoutInit(struct ArrowLayout* layout, enum ArrowType storage_type) {
}
}
-void* ArrowMalloc(int64_t size) { return malloc(size); }
+void* ArrowMalloc(int64_t size) { return malloc((size_t)size); }
-void* ArrowRealloc(void* ptr, int64_t size) { return realloc(ptr, size); }
+void* ArrowRealloc(void* ptr, int64_t size) { return realloc(ptr, (size_t)size); }
Review Comment:
I think these conversions should be checked (and return null on failure), cleaning up the pointer in the realloc case
##########
src/nanoarrow/ipc/reader.c:
##########
@@ -66,7 +66,8 @@ static ArrowErrorCode ArrowIpcInputStreamBufferRead(struct ArrowIpcInputStream*
}
if (bytes_to_read > 0) {
- memcpy(buf, private_data->input.data + private_data->cursor_bytes, bytes_to_read);
+ memcpy(buf, private_data->input.data + private_data->cursor_bytes,
+ (size_t)bytes_to_read);
Review Comment:
I think this can be a NANOARROW_DCHECK that bytes_to_read is small enough (there shouldn't have been a single in memory buffer bigger than 4 GB in the process anyway)
##########
src/nanoarrow/ipc/reader.c:
##########
@@ -293,7 +295,7 @@ static int ArrowIpcArrayStreamReaderNextHeader(
NANOARROW_RETURN_NOT_OK_WITH_ERROR(ArrowBufferReserve(&private_data->header, 4),
&private_data->error);
memmove(private_data->header.data + 4, private_data->header.data,
- private_data->header.size_bytes);
+ (size_t)private_data->header.size_bytes);
Review Comment:
Let's check this and return an error if outside the bounds
##########
src/nanoarrow/ipc/encoder.c:
##########
@@ -250,7 +250,8 @@ ArrowErrorCode ArrowIpcEncoderFinalizeBuffer(struct ArrowIpcEncoder* encoder,
int64_t padded_size = _ArrowRoundUpToMultipleOf8(size);
NANOARROW_RETURN_NOT_OK(
ArrowBufferReserve(out, sizeof(int32_t) + sizeof(int32_t) + padded_size));
- NANOARROW_ASSERT_OK(ArrowIpcEncoderWriteContinuationAndSize(out, padded_size));
+ NANOARROW_ASSERT_OK(
+ ArrowIpcEncoderWriteContinuationAndSize(out, (size_t)padded_size));
Review Comment:
We should check the padded size here (or update ArrowIpcEncoderWriteContinuationAndSize() to accept an int64 and check the failure code)
##########
src/nanoarrow/ipc/reader.c:
##########
@@ -133,7 +134,8 @@ static ArrowErrorCode ArrowIpcInputStreamFileRead(struct ArrowIpcInputStream* st
}
// Do the read
- int64_t bytes_read = (int64_t)fread(buf, 1, buf_size_bytes, private_data->file_ptr);
+ int64_t bytes_read =
+ (int64_t)fread(buf, 1, (size_t)buf_size_bytes, private_data->file_ptr);
Review Comment:
I think here we can also NANOARROW_DCHECK (because the output can't have been allocated to a size bigger than 4 GB in a 32 bit process)
##########
src/nanoarrow/ipc/writer.c:
##########
@@ -123,7 +123,8 @@ static ArrowErrorCode ArrowIpcOutputStreamFileWrite(struct ArrowIpcOutputStream*
}
// Do the write
- int64_t bytes_written = (int64_t)fwrite(buf, 1, buf_size_bytes, private_data->file_ptr);
+ int64_t bytes_written =
+ (int64_t)fwrite(buf, 1, (size_t)buf_size_bytes, private_data->file_ptr);
Review Comment:
I think a DCHECK would be good here too
--
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] fix: Avoid implicit 64-to-32-bit conversions on 32-bit platforms [arrow-nanoarrow]
Posted by "ump45nose (via GitHub)" <gi...@apache.org>.
ump45nose commented on PR #983:
URL: https://github.com/apache/arrow-nanoarrow/pull/983#issuecomment-6344471917
Thanks @paleolimbot. 487d8a9 adds `_ArrowInt64FitsInSizeT()` and `_ArrowInt64ToSizeT()` to `inline_types.h` (the second one `NANOARROW_DCHECK`s that the value is non-negative and fits in `size_t`). The conversions in this PR now go through it, so the IPC reader/writer `memcpy`/`fread`/`fwrite` sizes are DCHECKs as you suggested.
The places you asked to check for real:
- `ArrowMalloc()`/`ArrowRealloc()` return `NULL` if the size doesn't fit in `size_t`, and `ArrowRealloc()` frees `ptr` in that case.
- `ArrowIpcEncoderWriteContinuationAndSize()` takes an `int64_t` and checks `0 <= size <= INT32_MAX`. `ArrowIpcEncoderFinalizeBuffer()` now returns its error instead of `NANOARROW_ASSERT_OK`.
- The legacy-prefix `memmove` in `ArrowIpcArrayStreamReaderNextHeader()` returns `EINVAL` with an error message if the header size is out of range.
`ArrowErrorSetString()` keeps a plain cast because it comes before `NANOARROW_DCHECK` is defined, and `src_len` is already bounded by `sizeof(error->message)` there.
I ran a Debug build (`NANOARROW_DEBUG`) with IPC and device enabled, and all 422 ctest tests pass. I couldn't do a real 32-bit build here. The failing `meson-build` (`-Warray-bounds` in `utils_test.cc`) and `clang-tidy` jobs fail the same way on `main`, and `test-python (3.9)` fails because the runner can't find Python 3.9 on Ubuntu 26.04. (AI-assisted reply.)
--
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] fix: Avoid implicit 64-to-32-bit conversions on 32-bit platforms [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #983:
URL: https://github.com/apache/arrow-nanoarrow/pull/983#issuecomment-6379096787
Thanks! I hadn't noticed, but I've put up a PR to fix that and will sort through it this weekend. 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: Avoid implicit 64-to-32-bit conversions on 32-bit platforms [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #983:
URL: https://github.com/apache/arrow-nanoarrow/pull/983
--
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] fix: Avoid implicit 64-to-32-bit conversions on 32-bit platforms [arrow-nanoarrow]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #983:
URL: https://github.com/apache/arrow-nanoarrow/pull/983#issuecomment-6342742219
## [Codecov](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/983?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `83.33333%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 78.59%. Comparing base ([`efe4274`](https://app.codecov.io/gh/apache/arrow-nanoarrow/commit/efe42748545c5c886391e1794786828aff09711d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`12278b3`](https://app.codecov.io/gh/apache/arrow-nanoarrow/commit/12278b3a5f2b8364ee054882848051efbb3108cf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/983?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [src/nanoarrow/ipc/encoder.c](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/983?src=pr&el=tree&filepath=src%2Fnanoarrow%2Fipc%2Fencoder.c&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL25hbm9hcnJvdy9pcGMvZW5jb2Rlci5j) | 50.00% | [0 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/983?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #983 +/- ##
=======================================
Coverage 78.59% 78.59%
=======================================
Files 106 106
Lines 17153 17158 +5
Branches 2043 2043
=======================================
+ Hits 13482 13486 +4
- Misses 2422 2423 +1
Partials 1249 1249
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/983?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</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