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

Returning error if offset doesn't fit into libc::off_t. #86

Closed
wants to merge 1 commit into from

Conversation

lvella
Copy link

@lvella lvella commented Jun 21, 2023

Slightly improves the situation of issue #85.

@RazrFalcon
Copy link
Owner

I guess the map_offset should be updated to #[should_panic] on 32bit Linux.

@adamreichold
Copy link

I just noticed that libc actually defines mmap64 taking off64_t. I think we should just use that function and fix this for good.

@RazrFalcon
Copy link
Owner

@adamreichold Looks like it's not available for musl? And requires _FILE_OFFSET_BITS flag check?

@adamreichold
Copy link

Looks like it's not available for musl?

The libc sources say

// * musl has 64-bit versions only so aliases the LFS64 symbols to the standard ones

And requires _FILE_OFFSET_BITS flag check?

I though this macro only controls whether these 64-bit wide interface replace the default interfaces or stay add-ons? But they are always available as add-ons (for glibc)?

In any case, I'd say it would be preferable to use #[cfg(..)] to make this work on those platforms where it can and fallback to try_into otherwise.

@RazrFalcon
Copy link
Owner

RazrFalcon commented Jun 21, 2023

The libc sources say

I haven't been able to understand what they mean. But I would assume that mmap64 isn't available in musl and we have to use mmap, which should already support 64bit offset?

I will try adding musl target to CI.

@adamreichold
Copy link

I haven't been able to understand what they mean. But I would assume that mmap64 isn't available in musl and we have to use mmap, which should already support 64bit offset?

Yes, my understanding is that Musl v1.2+ on 32-bit platforms has mem::size_of::<off_t>() == 8 and hence this problem does not affect it and we can just continue to call mmap. It is only Glibc on 32-bit platforms which is problematic as per the OP.

@adamreichold
Copy link

adamreichold commented Jun 21, 2023

@lvella
Copy link
Author

lvella commented Jun 24, 2023

The logic could be: use mmap if off_t is 64 bits, use mmap64 if available and off_t is less than 64 bits, otherwise use mmap with the overflow runtime check of this PR.

Problem is, I have no idea how to do that. I tried detecting if mmap64 is available in a build script using crate autocfg, but I could not make it use libc in the test built files.

@RazrFalcon
Copy link
Owner

@lvella Too complicated. Let's try using mmap64 for now and see how it will go.

@RazrFalcon RazrFalcon closed this Jun 24, 2023
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.

3 participants