Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OOB access in Buffer::from_iter #5412

Closed
kawadakk opened this issue Feb 20, 2024 · 2 comments · Fixed by #5417
Closed

OOB access in Buffer::from_iter #5412

kawadakk opened this issue Feb 20, 2024 · 2 comments · Fixed by #5417
Assignees
Labels
arrow Changes to the arrow crate bug help wanted

Comments

@kawadakk
Copy link
Contributor

Describe the bug
When overflow checks are disabled, <Buffer as FromIterator<T>>::from_iter may construct MutableBuffer with a smaller capacity due to wrap-around arithmetic, causing out-of-bounds write.

impl<T: ArrowNativeType> FromIterator<T> for Buffer {
fn from_iter<I: IntoIterator<Item = T>>(iter: I) -> Self {
let mut iterator = iter.into_iter();
let size = std::mem::size_of::<T>();
// first iteration, which will likely reserve sufficient space for the buffer.
let mut buffer = match iterator.next() {
None => MutableBuffer::new(0),
Some(element) => {
let (lower, _) = iterator.size_hint();
let mut buffer = MutableBuffer::new(lower.saturating_add(1) * size);
unsafe {
std::ptr::write(buffer.as_mut_ptr() as *mut T, element);
buffer.set_len(size);
}

pub fn with_capacity(capacity: usize) -> Self {
let capacity = bit_util::round_upto_multiple_of_64(capacity);

/// Returns the nearest number that is `>=` than `num` and is a multiple of 64
#[inline]
pub fn round_upto_multiple_of_64(num: usize) -> usize {
round_upto_power_of_2(num, 64)
}
/// Returns the nearest multiple of `factor` that is `>=` than `num`. Here `factor` must
/// be a power of 2.
pub fn round_upto_power_of_2(num: usize, factor: usize) -> usize {
debug_assert!(factor > 0 && (factor & (factor - 1)) == 0);
(num + (factor - 1)) & !(factor - 1)
}

To Reproduce
The following code segfaults in release build:

// Trigger wrap-wround in `Buffer::from_iter`
arrow_buffer::buffer::Buffer::from_iter(
    std::iter::repeat(42u64).take(usize::MAX / std::mem::size_of::<u64>() + 1));
// Trigger wrap-wround in `bit_util::round_upto_multiple_of_64`
arrow_buffer::buffer::Buffer::from_iter(
    std::iter::repeat(42u64).take(usize::MAX / std::mem::size_of::<u64>()));

Expected behavior
Panicking, aborting, or successful UB-free execution

Additional context

@kawadakk kawadakk added the bug label Feb 20, 2024
@Jefffrey Jefffrey self-assigned this Feb 21, 2024
@Jefffrey
Copy link
Contributor

Nice find 👀

@tustvold
Copy link
Contributor

tustvold commented Mar 1, 2024

label_issue.py automatically added labels {'arrow'} from #5417

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate bug help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants