arrow-row: decode_binary_view_inner casts a usize offset to u32 unchecked, wrong data past 4 GiB
- Dominant language
- Rust
- Stars
- 3.6k
- Forks
- 1.3k
- Avg merge
- 2d 18h
- Merged PRs (30d)
- 169
Description
### Describe the bug
decode_binary_view_inner in arrow-row/src/variable.rs:358 casts a usize buffer offset down to u32 with nothing bounding it.
views[i] = make_view(val, 0, start_offset as u32);
start_offset is values.len(). values is a single MutableBuffer sized from values_capacity, which is summed over every long string in the batch, so nothing caps it. once the decoded data crosses 4 GiB the cast wraps and every view built after that resolves to the wrong bytes inside the right buffer. no panic, no error, you get back different strings than you put in. reachable through decode_binary_view and decode_string_view.
the same crate already guards the i32 version of this. try_into_binary in arrow-row/src/lib.rs:1464 rejects a buffer over i32::MAX, and the comment under it says the offsets follow from the length check. the u32 path in the same crate has neither the check nor an assert.
concat_elements.rs handles it a third way, hoisting the check into concat_elements_view_array and leaving a debug_assert on the offset, with a comment that skipping u32::try_from per insertion is worth about 5% in the benchmark.
### To Reproduce
build Rows over a string column whose long values total more than 4 GiB, decode with decode_string_view, views past the 4 GiB mark resolve to the wrong slice. i have not run it, thats 4 GiB of strings and i dont have the machine, the finding is from reading the code.
### Expected behavior
offsets stay inside u32, or the call fails loudly.
the format already gives the way out. make_view takes a block id and the decode hardcodes 0, so starting a new buffer when the current one would cross u32::MAX and bumping the block keeps every offset in range. coalesce/byte_view.rs already does that with its completed buffers. it touches a hot loop in more than one place though, so worth a call before anyone writes it.
### Additional context
same unguarded cast in three other spots,
arrow-cast/src/cast/dictionary.rs:255 and :298
arrow-select/src/coalesce/byte_view.rs:340, three lines under a try_into().expect("too many buffers") that does check the buffer index
arrow-array/src/array/byte_view_array.rs:1059
happy to take the PR once theres a decision on split versus fail.
Contributor guide
Research direction
Start at arrow-row/src/variable.rs:358 and compare the u32 cast with the i32 guard in arrow-row/src/lib.rs:1464 and the buffer handling in coalesce/byte_view.rs. Decide between rejecting oversized data and splitting buffers, then trace decode_binary_view and decode_string_view plus the other listed casts. Done means offsets cannot wrap past u32::MAX and the chosen behavior is validated across the affected paths.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- data
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100