You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Cintu07 (via GitHub)" <gi...@apache.org> on 2026/09/04 11:55:37 UTC
[PR] fix(arrow-buffer): add a fallible collect_bool [arrow-rs]
Cintu07 opened a new pull request, #10984:
URL: https://github.com/apache/arrow-rs/pull/10984
# Which issue does this PR close?
- Closes #10973.
# Rationale for this change
`collect_bool` reserves ceil(len / 64) u64 words before it calls the closure once, so a `len` that comes from a row or bit count in user controlled data aborts the process rather than failing. the reservation went through `Vec::with_capacity`, which has no way to report that.
this file already has the shape for it. `with_capacity` is `try_with_capacity(..).unwrap_or_else(|e| panic!("{e}"))` at line 154, and there are seven other fallible twins beside it: `try_from_len_zeroed`, `try_reserve`, `try_repeat_slice_n_times`, `try_resize`, `try_shrink_to_fit`, `try_extend_from_slice` and `try_extend_zeros`. `collect_bool` is the allocating entry point that does not have one.
# What changes are included in this PR?
- add `try_collect_bool` returning `Result<Self, MutableBufferError>`
- `collect_bool` calls it and unwraps, the same way `with_capacity` calls `try_with_capacity`
- the reservation goes through `Vec::try_reserve`, and a failure maps onto the existing `AllocationError` variant carrying the layout it tried to take
no behaviour change for any `len` that already worked. `collect_bool` still panics, one level down, and its doc comment now says so.
i went with a fallible entry point rather than capping the reservation, since a cap only moves where the abort happens rather than letting a caller handle it. happy to do the cap instead if you would rather not grow the api surface.
# Are these changes tested?
yes, two tests.
`try_collect_bool_reports_a_len_it_cannot_reserve` uses the 2^60 from the issue and asserts it comes back as `AllocationError` and that the closure ran zero times. the reproducer in the issue aborts on that same input with `memory allocation of 144115188075855872 bytes failed`.
`try_collect_bool_matches_collect_bool_for_sizes_that_fit` checks the two agree byte for byte at 0, 1, 63, 64, 65 and 1000 bits, since 64 is the word boundary and 8 is the truncation boundary.
`arrow-buffer` is 351 passed, `arrow-array` is 721 passed, and fmt and clippy with `-D warnings` are clean. i ran arrow-array as well because `collect_bool` has around twenty call sites in it and this changes how the function is built.
# Are there any user-facing changes?
`try_collect_bool` is new and additive. `collect_bool` keeps its signature and still panics on a `len` it cannot reserve.
--
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(arrow-buffer): add a fallible collect_bool [arrow-rs]
Posted by "Cintu07 (via GitHub)" <gi...@apache.org>.
Cintu07 commented on code in PR #10984:
URL: https://github.com/apache/arrow-rs/pull/10984#discussion_r3937575438
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -1761,4 +1784,30 @@ mod tests {
buf.push(1u8);
buf.reserve(usize::MAX);
}
+ #[test]
+ #[cfg_attr(miri, ignore)] // miri interprets the allocation and runs out of memory itself
+ fn try_collect_bool_reports_a_len_it_cannot_reserve() {
+ // 2^60 bits is 2^54 u64 words, which is 128 PiB of backing storage.
+ // collect_bool reserves that before calling f, so it aborts the process.
+ let mut calls = 0usize;
+ let result = MutableBuffer::try_collect_bool(1usize << 60, |_| {
+ calls += 1;
+ true
+ });
+ assert!(matches!(
+ result,
+ Err(MutableBufferError::AllocationError(_))
+ ));
+ assert_eq!(calls, 0, "f must not run when the reservation fails");
+ }
+
+ #[test]
+ fn try_collect_bool_matches_collect_bool_for_sizes_that_fit() {
Review Comment:
dropped this one too in b58729e.
--
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(arrow-buffer): add a fallible collect_bool [arrow-rs]
Posted by "Cintu07 (via GitHub)" <gi...@apache.org>.
Cintu07 commented on code in PR #10984:
URL: https://github.com/apache/arrow-rs/pull/10984#discussion_r3935362771
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -1761,4 +1784,29 @@ mod tests {
buf.push(1u8);
buf.reserve(usize::MAX);
}
+ #[test]
+ fn try_collect_bool_reports_a_len_it_cannot_reserve() {
Review Comment:
thanks, fixed in a2e0417. #[cfg_attr(miri, ignore)] on that one the way the rest of
the repo does it. the other test only uses small sizes so it should still run under
miri, ci will confirm.
--
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(arrow-buffer): add a fallible collect_bool [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10984:
URL: https://github.com/apache/arrow-rs/pull/10984#discussion_r3934107440
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -1761,4 +1784,29 @@ mod tests {
buf.push(1u8);
buf.reserve(usize::MAX);
}
+ #[test]
+ fn try_collect_bool_reports_a_len_it_cannot_reserve() {
Review Comment:
@Cintu07 this is causing the miri to fail
```
test buffer::mutable::tests::try_collect_bool_reports_a_len_it_cannot_reserve ...
stderr ───
warning: extern crate `criterion` is unused in crate `arrow_buffer`
--> arrow-buffer/src/lib.rs:18:1
|
18 | //! Low-level buffer abstractions for [Apache Arrow Rust](https://docs.rs/arrow)
| ^
|
= help: remove the dependency or add `use criterion as _;` to the crate root
= note: requested on the command line with `--force-warn unused-crate-dependencies`
error: resource exhaustion: tried to allocate more memory than available to compiler
--> /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/raw_vec/mod.rs:568:13
|
568 | self.alloc.allocate(new_layout)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ resource exhaustion occurred 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
Re: [PR] fix(arrow-buffer): add a fallible collect_bool [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #10984:
URL: https://github.com/apache/arrow-rs/pull/10984#discussion_r3939037672
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -737,9 +737,32 @@ impl MutableBuffer {
///
/// This is similar to `from_trusted_len_iter_bool`, however, can be significantly faster
/// as it eliminates the conditional `Iterator::next`
+ ///
+ /// # Panics
+ ///
+ /// Panics if the backing storage for `len` bits cannot be allocated. Use
+ /// [`MutableBuffer::try_collect_bool`] when `len` comes from untrusted input.
+ #[inline]
+ pub fn collect_bool<F: FnMut(usize) -> bool>(len: usize, f: F) -> Self {
+ Self::try_collect_bool(len, f).unwrap_or_else(|e| panic!("{e}"))
+ }
+
+ /// Fallible version of [`MutableBuffer::collect_bool`].
+ ///
+ /// `len` is a bit count, so the reservation is `ceil(len / 64)` u64 slots. Reserving that
+ /// up front means a `len` taken from a row or bit count in user controlled data aborts
+ /// the process before `f` is ever called, so this reports the failure instead.
Review Comment:
```suggestion
/// `len` is a bit count, so the reservation is `ceil(len / 64)` u64 slots. This function
/// returns an error if that much memory cannot be reserved up front.
```
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -737,9 +737,32 @@ impl MutableBuffer {
///
/// This is similar to `from_trusted_len_iter_bool`, however, can be significantly faster
/// as it eliminates the conditional `Iterator::next`
+ ///
+ /// # Panics
+ ///
+ /// Panics if the backing storage for `len` bits cannot be allocated. Use
+ /// [`MutableBuffer::try_collect_bool`] when `len` comes from untrusted input.
Review Comment:
```suggestion
/// Panics if the backing storage for `len` bits cannot be allocated. Use
/// [`MutableBuffer::try_collect_bool`] for a fallible version.
```
--
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(arrow-buffer): add a fallible collect_bool [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #10984:
URL: https://github.com/apache/arrow-rs/pull/10984#issuecomment-5563494154
thanks @Cintu07 & @Rich-T-kid
--
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(arrow-buffer): add a fallible collect_bool [arrow-rs]
Posted by "Cintu07 (via GitHub)" <gi...@apache.org>.
Cintu07 commented on code in PR #10984:
URL: https://github.com/apache/arrow-rs/pull/10984#discussion_r3937604544
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -737,9 +737,32 @@ impl MutableBuffer {
///
/// This is similar to `from_trusted_len_iter_bool`, however, can be significantly faster
/// as it eliminates the conditional `Iterator::next`
+ ///
+ /// # Panics
+ ///
+ /// Panics if the backing storage for `len` bits cannot be allocated. Use
+ /// [`MutableBuffer::try_collect_bool`] when `len` comes from untrusted input.
#[inline]
- pub fn collect_bool<F: FnMut(usize) -> bool>(len: usize, mut f: F) -> Self {
- let mut buffer: Vec<u64> = Vec::with_capacity(bit_util::ceil(len, 64));
+ pub fn collect_bool<F: FnMut(usize) -> bool>(len: usize, f: F) -> Self {
+ Self::try_collect_bool(len, f).unwrap_or_else(|e| panic!("{e}"))
+ }
+
+ /// Fallible version of [`MutableBuffer::collect_bool`].
+ ///
+ /// `len` is a bit count, so the reservation is `ceil(len / 64)` words. Reserving that
Review Comment:
done in b58729e, reads ceil(len / 64) u64 slots now.
--
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(arrow-buffer): add a fallible collect_bool [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #10984:
URL: https://github.com/apache/arrow-rs/pull/10984
--
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(arrow-buffer): add a fallible collect_bool [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10984:
URL: https://github.com/apache/arrow-rs/pull/10984#discussion_r3937322373
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -737,9 +737,32 @@ impl MutableBuffer {
///
/// This is similar to `from_trusted_len_iter_bool`, however, can be significantly faster
/// as it eliminates the conditional `Iterator::next`
+ ///
+ /// # Panics
+ ///
+ /// Panics if the backing storage for `len` bits cannot be allocated. Use
+ /// [`MutableBuffer::try_collect_bool`] when `len` comes from untrusted input.
#[inline]
- pub fn collect_bool<F: FnMut(usize) -> bool>(len: usize, mut f: F) -> Self {
- let mut buffer: Vec<u64> = Vec::with_capacity(bit_util::ceil(len, 64));
+ pub fn collect_bool<F: FnMut(usize) -> bool>(len: usize, f: F) -> Self {
+ Self::try_collect_bool(len, f).unwrap_or_else(|e| panic!("{e}"))
+ }
+
+ /// Fallible version of [`MutableBuffer::collect_bool`].
+ ///
+ /// `len` is a bit count, so the reservation is `ceil(len / 64)` words. Reserving that
Review Comment:
nit: This is true if “words” means u64 storage words, but it’s a bit ambiguous on 32-bit targets where “word” usually means 32 bits. Maybe say `ceil(len / 64) u64 slots` instead.
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -1761,4 +1784,29 @@ mod tests {
buf.push(1u8);
buf.reserve(usize::MAX);
}
+ #[test]
+ fn try_collect_bool_reports_a_len_it_cannot_reserve() {
Review Comment:
I'd prefer to not skip the miri check here.
Im not sure that we need either of these test 🤔. I think the fix here is self explanatory enough to not warrant them.
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -1761,4 +1784,30 @@ mod tests {
buf.push(1u8);
buf.reserve(usize::MAX);
}
+ #[test]
+ #[cfg_attr(miri, ignore)] // miri interprets the allocation and runs out of memory itself
+ fn try_collect_bool_reports_a_len_it_cannot_reserve() {
+ // 2^60 bits is 2^54 u64 words, which is 128 PiB of backing storage.
+ // collect_bool reserves that before calling f, so it aborts the process.
+ let mut calls = 0usize;
+ let result = MutableBuffer::try_collect_bool(1usize << 60, |_| {
+ calls += 1;
+ true
+ });
+ assert!(matches!(
+ result,
+ Err(MutableBufferError::AllocationError(_))
+ ));
+ assert_eq!(calls, 0, "f must not run when the reservation fails");
+ }
+
+ #[test]
+ fn try_collect_bool_matches_collect_bool_for_sizes_that_fit() {
Review Comment:
nit: I don't think `try_collect_bool` does enough thats different from `collect_bool` to warrant adding another test
--
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(arrow-buffer): add a fallible collect_bool [arrow-rs]
Posted by "Cintu07 (via GitHub)" <gi...@apache.org>.
Cintu07 commented on code in PR #10984:
URL: https://github.com/apache/arrow-rs/pull/10984#discussion_r3937575438
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -1761,4 +1784,30 @@ mod tests {
buf.push(1u8);
buf.reserve(usize::MAX);
}
+ #[test]
+ #[cfg_attr(miri, ignore)] // miri interprets the allocation and runs out of memory itself
+ fn try_collect_bool_reports_a_len_it_cannot_reserve() {
+ // 2^60 bits is 2^54 u64 words, which is 128 PiB of backing storage.
+ // collect_bool reserves that before calling f, so it aborts the process.
+ let mut calls = 0usize;
+ let result = MutableBuffer::try_collect_bool(1usize << 60, |_| {
+ calls += 1;
+ true
+ });
+ assert!(matches!(
+ result,
+ Err(MutableBufferError::AllocationError(_))
+ ));
+ assert_eq!(calls, 0, "f must not run when the reservation fails");
+ }
+
+ #[test]
+ fn try_collect_bool_matches_collect_bool_for_sizes_that_fit() {
Review Comment:
done in b58729e, reads ceil(len / 64) u64 slots now.
--
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