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

avoid silently truncating the offset #75

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

Conversation

oconnor663
Copy link

This should address one of the concerns that came up in #65.

I'm not sure exactly what test I should add for this case. Please advise :)

This should address one of the concerns that came up in
danburkert#65.
This avoids casting negative numbers into an unsigned type.
@danburkert
Copy link
Owner

I expected that the fix to this would be to use mmap64 instead of mmap (on select platforms), or is that infeasible?

@oconnor663
Copy link
Author

That definitely sounds like the better fix where it's possible, but I don't know how wide mmap64 support is. The libc crate doesn't seem to support it for macOS or any of the other BSD variants. Do you think we should start calling that function on Linux, as part of this PR? A couple of thoughts:

  • Because off64_t is an i64, but our offset parameter is a u64, we'll still need to check for overflow when we cast it.
  • It looks like emscripten perversely defines off64_t as i32. I assume mmap fails under emscripten anyway, but if there might be more platforms in the future doing this, we might not want to assume off64_t is actually always 64 bits.

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