Support zero-size `Mmap` and `MmapMut` #25

SimonSapin · 2021-09-16T10:20:39Z

Previously, mapping an empty file or requesting a zero-size mapping would return an error because underlying OS APIs do not support zero-size mappings. Now, this crate instead requests a one-byte mapping (which most lilkely ends up rounded up to one memory page) but still has Deref return an empty slice in those cases.

This is the behavior suggested for Unix in danburkert#72 (comment)

On Windows, docs for CreateFileMappingW say:
https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-createfilemappingw

dwMaximumSizeHigh

The high-order DWORD of the maximum size of the file mapping object.

dwMaximumSizeLow

The low-order DWORD of the maximum size of the file mapping object.

If this parameter and dwMaximumSizeHigh are 0 (zero), the maximum size of the file mapping object is equal to the current size of the file that hFile identifies.

An attempt to map a file with a length of 0 (zero) fails with an error code of ERROR_FILE_INVALID. Applications should test for files with a length of 0 (zero) and reject those files.

~~Hopefully that last paragraph only applies when dwMaximumSize is zero (which this PR avoids), but that is not clear to me.~~

Edit: nope: #25 (comment)

SimonSapin · 2021-09-16T10:26:14Z

I cannot easily run tests for Windows on my machine (only cargo check with cross-compilation). On CI test::map_empty_file panics with IoError::Os { code: 1006, kind: Uncategorized, message: "The volume for a file has been externally altered so that the opened file is no longer valid." }, so it looks like this approach of creating a mapping larger than the file does not work for Windows unfortunately.

RazrFalcon · 2021-09-16T10:36:06Z

Maybe we should simply have a flag that indicates that the mmap len was created empty and return &[] in deref?
This would require having a separate code path for this edge case. Basically, we would have to change MmapInner from a struct to an enum. This would make code a bit bloated, but I don't see another solution.

SimonSapin · 2021-09-16T10:37:08Z

Do you mean for windows::MmapInner only or unix::MmapInner too?

RazrFalcon · 2021-09-16T10:40:34Z

For both, to be more consistent. Or just for Windows + a long comment for Unix explaining why it's ok and why it's different from Win.

Fixes danburkert#72 Previously, mapping an empty file or requesting a zero-size mapping would return an error because underlying OS APIs do not support zero-size mappings. Now, this crate instead requests a one-byte mapping (which most lilkely ends up rounded up to one memory page) but still has `Deref` return an empty slice in those cases. This is the behavior suggested for Unix in danburkert#72 (comment) On Windows, docs for `CreateFileMappingW` say: https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-createfilemappingw > `dwMaximumSizeHigh` > > The high-order DWORD of the maximum size of the file mapping object. > > `dwMaximumSizeLow` > > The low-order DWORD of the maximum size of the file mapping object. > > If this parameter and dwMaximumSizeHigh are 0 (zero), the maximum size of > the file mapping object is equal to the current size of the file that hFile > identifies. > > An attempt to map a file with a length of 0 (zero) fails with an error code > of ERROR_FILE_INVALID. Applications should test for files with a length of > 0 (zero) and reject those files. Hopefully that last paragraph only applies when `dwMaximumSize` is zero (which this PR avoids), but that is not clear to me.

src/windows.rs

RazrFalcon · 2021-09-16T12:29:18Z

src/windows.rs

@@ -119,6 +119,11 @@ extern "system" {
    fn GetSystemInfo(lpSystemInfo: LPSYSTEM_INFO);
 }

+/// Returns a fixed pointer that is valid for `slice::from_raw_parts::<u8>` with `len == 0`.
+fn empty_slice_ptr() -> *mut c_void {
+    std::ptr::NonNull::<u8>::dangling().cast().as_ptr()


What kind of black magic is that? 😄

NonNull::dangling interprets align_of() as an address to create a pointer that is not null but still correctly aligned. It is mentioned in the docs of slice::from_raw_parts as a way to obtain an arbitrary pointer that can be used to create an empty slice.

NonNull::cast is similar to (and based on) as for casting for example *mut u8 to *mut c_void. It can be turbofished, but in this case type inference fills in the destination type.

RazrFalcon · 2021-09-16T12:30:36Z

src/unix.rs

-                "memory map must have a non-zero length",
-            ));
-        }
+        // Ensure a non-zero length for the underlying mapping


Can you expand this comment a bit to explain why we have to do this.

I’d like to also explain why the work around is correct, but I’m getting less and less convinced it actually is 😅

RazrFalcon · 2021-09-16T12:36:33Z

I don't understand the Unix logic. Is it ok to map a zero-sized file to a non-zero sized, aligned memory block?

On Windows, afaik, you're simply skipping CreateFileMappingW altogether, which is understandable. But the Unix solution is confusing to me.

PS: I have close to zero knowledge about mmap, despite the fact I'm maintaining this crate. Therefore I want to have a good understanding and reason behind all major changes to it.

The previous commit’s trick of creating a non-zero-size mapping for an empty file does not work on Windows like it does with `mmap` on Unix. It cause an `IoError` with code 1006 and message `The volume for a file has been externally altered so that the opened file is no longer valid.` https://github.com/RazrFalcon/memmap2-rs/pull/25/checks?check_run_id=3620122580#step:4:46 Instead, in that case skip creating a mapping entirely. We don’t need one for `Deref` to return a valid zero-size slice.

SimonSapin · 2021-09-16T13:04:09Z

Is it ok to map a zero-sized file to a non-zero sized, aligned memory block?

I went by this comment danburkert#72 (comment) suggesting that it is ok, but I don’t find something supporting this idea in the man page. I’ve asked that commenter: danburkert#72 (comment)

To skip calling mmap at all on Unix for empty files like this PR now does for Windows would work around this issue, but get into the one with virtual filesystems and pseudo-files like /proc/cpuinfo that incorrectly report a zero size, as discussed in the rest of danburkert#72

SimonSapin · 2021-09-16T14:50:03Z

I’ve found POSIX defining the guarantee we need: danburkert#72 (comment)

RazrFalcon · 2021-09-16T18:44:34Z

Looks good to me. And thanks for a detailed comment.

SimonSapin mentioned this pull request Sep 16, 2021

Mmap fails for empty files on Windows danburkert/memmap-rs#72

Open

SimonSapin force-pushed the zero branch from f7a60f0 to 5c3eae8 Compare September 16, 2021 12:23

RazrFalcon reviewed Sep 16, 2021

View reviewed changes

src/windows.rs Outdated Show resolved Hide resolved

RazrFalcon reviewed Sep 16, 2021

View reviewed changes

SimonSapin force-pushed the zero branch from 5c3eae8 to 08de78d Compare September 16, 2021 12:53

Document the Unix hack for mapping empty files

97f049f

RazrFalcon merged commit 1183449 into RazrFalcon:master Sep 16, 2021

SimonSapin deleted the zero branch September 16, 2021 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support zero-size `Mmap` and `MmapMut` #25

Support zero-size `Mmap` and `MmapMut` #25

SimonSapin commented Sep 16, 2021 •

edited

Loading

SimonSapin commented Sep 16, 2021

RazrFalcon commented Sep 16, 2021

SimonSapin commented Sep 16, 2021 •

edited

Loading

RazrFalcon commented Sep 16, 2021

RazrFalcon Sep 16, 2021

SimonSapin Sep 16, 2021

RazrFalcon Sep 16, 2021

SimonSapin Sep 16, 2021

RazrFalcon commented Sep 16, 2021

SimonSapin commented Sep 16, 2021

SimonSapin commented Sep 16, 2021

RazrFalcon commented Sep 16, 2021

Support zero-size Mmap and MmapMut #25

Support zero-size Mmap and MmapMut #25

Conversation

SimonSapin commented Sep 16, 2021 • edited Loading

SimonSapin commented Sep 16, 2021

RazrFalcon commented Sep 16, 2021

SimonSapin commented Sep 16, 2021 • edited Loading

RazrFalcon commented Sep 16, 2021

RazrFalcon Sep 16, 2021

Choose a reason for hiding this comment

SimonSapin Sep 16, 2021

Choose a reason for hiding this comment

RazrFalcon Sep 16, 2021

Choose a reason for hiding this comment

SimonSapin Sep 16, 2021

Choose a reason for hiding this comment

RazrFalcon commented Sep 16, 2021

SimonSapin commented Sep 16, 2021

SimonSapin commented Sep 16, 2021

RazrFalcon commented Sep 16, 2021

Support zero-size `Mmap` and `MmapMut` #25

Support zero-size `Mmap` and `MmapMut` #25

SimonSapin commented Sep 16, 2021 •

edited

Loading

SimonSapin commented Sep 16, 2021 •

edited

Loading