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

Unsound DecimalArray Validation in Unreleased arrow-rs #1813

Closed
tustvold opened this issue Jun 7, 2022 · 3 comments · Fixed by #1816
Closed

Unsound DecimalArray Validation in Unreleased arrow-rs #1813

tustvold opened this issue Jun 7, 2022 · 3 comments · Fixed by #1816
Labels
bug development-process Related to development process of arrow-rs

Comments

@tustvold
Copy link
Contributor

tustvold commented Jun 7, 2022

Describe the bug

The validation logic added in #1767 is incorrect. In particular

let values_buffer = &self.buffers[0];

for pos in 0..values_buffer.len() {
    let raw_val = unsafe {
        std::slice::from_raw_parts(
            values_buffer.as_ptr().add(pos),
            16_usize,
        )
    };
    let value = i128::from_le_bytes(raw_val.try_into().unwrap());
    validate_decimal_precision(value, *p)?;
}

This will read 16 byte slices at 1 byte intervals in the underlying array of decimal data, which will as a result:

  • This will interpret data at non-value intervals (avoiding UB largely by accident)
  • This will read beyond the bounds of values_buffer which is unsound
  • It will not take into account any offset

To Reproduce

#[test]
fn test_decimal_validation() {
    let mut builder = DecimalBuilder::new(4, 10, 4);
    builder.append_value(10000).unwrap();
    builder.append_value(20000).unwrap();
    let array = builder.finish();

    array.data().validate_full().unwrap();
}

Fails with

Invalid argument error: 42535295865117307932921825928971026471 is too large to store in a Decimal of precision 10. Max is 9999999999"

Expected behavior

This should not incorrectly fail for valid data

Additional context

This was discovered by DataFusion's test suite for Decimals

@tustvold tustvold added the bug label Jun 7, 2022
@viirya
Copy link
Member

viirya commented Jun 7, 2022

Oops, I will fix it. Thanks @tustvold

@tustvold
Copy link
Contributor Author

tustvold commented Jun 7, 2022

Already preparing a fix 😄

tustvold added a commit to tustvold/arrow-rs that referenced this issue Jun 7, 2022
Fix offset validation for sliced children of list arrays (apache#1814)
tustvold added a commit to tustvold/arrow-rs that referenced this issue Jun 7, 2022
Fix offset validation for sliced children of list arrays (apache#1814)
tustvold added a commit to tustvold/arrow-rs that referenced this issue Jun 7, 2022
Fix offset validation for sliced children of list arrays (apache#1814)
tustvold added a commit to tustvold/arrow-rs that referenced this issue Jun 7, 2022
Fix offset validation for sliced children of list arrays (apache#1814)
tustvold added a commit to tustvold/arrow-rs that referenced this issue Jun 7, 2022
Fix offset validation for sliced children of list arrays (apache#1814)
tustvold added a commit that referenced this issue Jun 7, 2022
* Fix DecimalArray validation (#1813)

Fix offset validation for sliced children of list arrays (#1814)

* Update arrow/src/array/data.rs

Co-authored-by: Liang-Chi Hsieh <[email protected]>

Co-authored-by: Liang-Chi Hsieh <[email protected]>
@alamb alamb added the development-process Related to development process of arrow-rs label Jun 9, 2022
@alamb
Copy link
Contributor

alamb commented Jun 9, 2022

marked as development process to suppress from release notes, as this was not released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug development-process Related to development process of arrow-rs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants