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

DoubleArray::new() does not check untrusted source #24

Open
vbkaisetsu opened this issue May 29, 2022 · 2 comments
Open

DoubleArray::new() does not check untrusted source #24

vbkaisetsu opened this issue May 29, 2022 · 2 comments

Comments

@vbkaisetsu
Copy link

vbkaisetsu commented May 29, 2022

DoubleArray::get_unit() calls slice::get_unchecked() internally, but this slice is not checked in DoubleArray::new().
So, it is possible to cause a segmentation fault even though the caller does not use unsafe blocks.

use yada::DoubleArray;

fn main() {
    let da = DoubleArray::new(vec![]);
    if let Some(index) = da.exact_match_search("example input") {
        dbg!(index);
    }
}
$ cargo run --release
...
[1]    81236 segmentation fault  cargo run --release

Solution 1: Change DoubleArray::new() to an unsafe function

This solution does not pay the additional runtime cost, but requires callers to be responsible for using the correct slice.

let da = unsafe { DoubleArray::new(trusted_bytes) };

Solution 2: Change slice::get_unchecked() to a checked function

The opposite of Solution 1.

let b = &self.0[index * UNIT_SIZE..(index + 1) * UNIT_SIZE];

or

let b = self.0.get(index * UNIT_SIZE..(index + 1) * UNIT_SIZE)
          .ok_or_else(|| ...)?;

Solution 3: Check the argument of DoubleArray::new()

This is too much work, but it does not require any responsibility on callers and does not pay any additional costs during the search.

@takuyaa
Copy link
Owner

takuyaa commented Jun 4, 2022

@vbkaisetsu Thanks for raising this issue. This behavior that might cause unexpected SIGSEGV should be fixed to avoid user confusion.

The policy of the fix I would like to propose is as follows:

  1. Introduce validate function that checks whether an array is valid or not as a representation of a double array.
    • The validation needs to check carefully so that all transitions of a double array are safe.
  2. Add a procedure that calls validate function to check a given array of DoubleArray::new.
    • This means DoubleArray::new will be a safe method as far as we validated.
  3. Introduce unsafe DoubleArray::new_without_verification method for expert users.
    • This method doesn't check an array at initialization. The behavior is the same as the current DoubleArray::new method.
    • Some users who are sure their array is valid might want this feature for faster initialization.

Does this proposal make sense to you?

@vbkaisetsu
Copy link
Author

SGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants