apache / apache/arrow-rs

arrow-row: decode_binary_view_inner casts a usize offset to u32 unchecked, wrong data past 4 GiB

Open
#11,011 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.