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

guarantee that length will never exceed isize::MAX #77

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

oconnor663
Copy link

I think there were two separate issues with the previous version:

  1. The main issue, which we've been discussing in what happens if we exceed isize::MAX on 32-bit platforms? #69, is that it's
    unsound to construct a slice larger that isize::MAX. That said, it
    might be impossible to trigger this behavior on some 32-bit systems.
    The comment linked to in raw_vec.rs says it requires "a platform
    which can use all 4GB in user-space. e.g. PAE or x32."

  2. The previous version was subtracting self.offset from the file
    length without checking for negative overflow. Very large offsets
    could have turned into an accidentally valid section of the file, in
    release mode when the overflow didn't panic.

I think there were two separate issues with the previous version:

1. The main issue, which we've been discussing in danburkert#69, is that it's
   unsound to construct a slice larger that isize::MAX. That said, it
   might be impossible to trigger this behavior on some 32-bit systems.
   The comment linked to in raw_vec.rs says it requires "a platform
   which can use all 4GB in user-space. e.g. PAE or x32."

2. The previous version was subtracting `self.offset` from the file
   length without checking for negative overflow. Very large offsets
   could have turned into an accidentally valid section of the file, in
   release mode when the overflow didn't panic.
@oconnor663
Copy link
Author

I'm putting this up as a discussion point for fixing #69, but I definitely could've missed some details, and I haven't thought about the right way to test it yet.

@oconnor663
Copy link
Author

Update: As of rust-lang/rust#53784, slice::from_raw_parts will panic in debug mode, if the length in bytes exceeds isize::MAX.

@danburkert
Copy link
Owner

OK this looks good, but please add some tests, with cases covering:

offset len outcome notes
0 isize::MAX fail
0 isize::MAX - 1 pass
x isize::MAX - x fail x > 0 && x < PAGE_SIZE
x isize::MAX - x - 1 pass x > 0 && x < PAGE_SIZE
x + y isize::MAX + y - x fail x > 0 && x < PAGE_SIZE && y > 0 && y % PAGE_SIZE == 0
x + y isize::MAX + y - x - 1 pass x > 0 && x < PAGE_SIZE && y > 0 && y % PAGE_SIZE == 0

Let me know if any of this isn't clear. I imagine you will have to disable the test via cfg on some platforms, but it should at least work on 32-bit platforms as well as 64-bit platforms which have sparse files (Linux).

@danburkert
Copy link
Owner

The test can look like the one in #74, but with the parameters I mentioned here to test edge cases.

@oconnor663
Copy link
Author

The Ext4 filesystem on my Linux machine doesn't support files larger than 16 TB. I think you'd have to be on Btrfs or similar that to create a sparse file big enough. Also, even when using a small file with a large length (which does work for me with lengths as large as 1<<46), trying to map anything close to isize::MAX bytes fails on my system with a "Cannot allocate memory" error.

Would you be ok with only testing the failure cases here, and making sure they're returning the error they're supposed to?

@oconnor663
Copy link
Author

Looking closely at the table there, I think I would change some of the cases:

  • I believe a slice of length isize::MAX is sound, and it's isize::MAX + 1 where we should start failing.
  • For the cases where the length is fine, but the length + offset is overflowing, do we need to fail? I think if the OS is willing to allocate that much memory, we should be ok, as long as the part we make a slice out of isn't too long.

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

Successfully merging this pull request may close these issues.

2 participants