Skip to content

C
capnproto
Commit 97673947 authored by Kenton Varda's avatar Kenton Varda
Browse files
Options

Fix filesystem API: Empty mmaps should succeed. 

I had a snarky comment in the Windows code being annoyed that mapping an empty file was documented to fail... but it turns out zero-length mappings fail on Linux, too. Also my work-around on Windows didn't work (but was never tested).

This PR fixes both.

This was prompted by @KubaO observing that `capnp compile` on an empty source file failed with a cryptic mmap error.

Closes #854, @KubaO's attempt at fixing this, since the correct fix is in the filesystem API, not in the compiler.
parent 6b6f824a
Hide whitespace changes
Inline Side-by-side
Showing with 30 additions and 11 deletions
c++/src/kj/filesystem-disk-test.c++
View file @ 97673947
......@@ -276,6 +276,11 @@ KJ_TEST("DiskFile") {
KJ_EXPECT(file->readAllText() == "");
// mmaping empty file should work
KJ_EXPECT(file->mmap(0, 0).size() == 0);
KJ_EXPECT(file->mmapPrivate(0, 0).size() == 0);
KJ_EXPECT(file->mmapWritable(0, 0)->get().size() == 0);
file->writeAll("foo");
KJ_EXPECT(file->readAllText() == "foo");
......@@ -294,6 +299,14 @@ KJ_TEST("DiskFile") {
file->truncate(18);
KJ_EXPECT(file->readAllText() == kj::StringPtr("foobaz\0\0\0\0\0\0\0\0\0\0\0\0", 18));
// empty mappings work, even if useless
KJ_EXPECT(file->mmap(0, 0).size() == 0);
KJ_EXPECT(file->mmapPrivate(0, 0).size() == 0);
KJ_EXPECT(file->mmapWritable(0, 0)->get().size() == 0);
KJ_EXPECT(file->mmap(2, 0).size() == 0);
KJ_EXPECT(file->mmapPrivate(2, 0).size() == 0);
KJ_EXPECT(file->mmapWritable(2, 0)->get().size() == 0);
{
auto mapping = file->mmap(0, 18);
auto privateMapping = file->mmapPrivate(0, 18);
......
c++/src/kj/filesystem-disk-unix.c++
View file @ 97673947
......@@ -346,6 +346,7 @@ public:
}
Array<const byte> mmap(uint64_t offset, uint64_t size) const {
if (size == 0) return nullptr; // zero-length mmap() returns EINVAL, so avoid it
auto range = getMmapRange(offset, size);
const void* mapping = ::mmap(NULL, range.size, PROT_READ, MAP_SHARED, fd, range.offset);
if (mapping == MAP_FAILED) {
......@@ -356,6 +357,7 @@ public:
}
Array<byte> mmapPrivate(uint64_t offset, uint64_t size) const {
if (size == 0) return nullptr; // zero-length mmap() returns EINVAL, so avoid it
auto range = getMmapRange(offset, size);
void* mapping = ::mmap(NULL, range.size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, range.offset);
if (mapping == MAP_FAILED) {
......@@ -454,6 +456,7 @@ public:
void changed(ArrayPtr<byte> slice) const override {
KJ_REQUIRE(slice.begin() >= bytes.begin() && slice.end() <= bytes.end(),
"byte range is not part of this mapping");
if (slice.size() == 0) return;
// msync() requires page-alignment, apparently, so use getMmapRange() to accomplish that.
auto range = getMmapRange(reinterpret_cast<uintptr_t>(slice.begin()), slice.size());
......@@ -463,6 +466,7 @@ public:
void sync(ArrayPtr<byte> slice) const override {
KJ_REQUIRE(slice.begin() >= bytes.begin() && slice.end() <= bytes.end(),
"byte range is not part of this mapping");
if (slice.size() == 0) return;
// msync() requires page-alignment, apparently, so use getMmapRange() to accomplish that.
auto range = getMmapRange(reinterpret_cast<uintptr_t>(slice.begin()), slice.size());
......@@ -474,6 +478,10 @@ public:
};
Own<const WritableFileMapping> mmapWritable(uint64_t offset, uint64_t size) const {
if (size == 0) {
// zero-length mmap() returns EINVAL, so avoid it
return heap<WritableFileMappingImpl>(nullptr);
}
auto range = getMmapRange(offset, size);
void* mapping = ::mmap(NULL, range.size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, range.offset);
if (mapping == MAP_FAILED) {
......
c++/src/kj/filesystem-disk-win32.c++
View file @ 97673947
......@@ -307,16 +307,7 @@ constexpr MmapDisposer mmapDisposer = MmapDisposer();
void* win32Mmap(HANDLE handle, MmapRange range, DWORD pageProtect, DWORD access) {
HANDLE mappingHandle;
mappingHandle = CreateFileMappingW(handle, NULL, pageProtect, 0, 0, NULL);
if (mappingHandle == INVALID_HANDLE_VALUE) {
auto error = GetLastError();
if (error == ERROR_FILE_INVALID && range.size == 0) {
// The documentation says that CreateFileMapping will fail with ERROR_FILE_INVALID if the
// file size is zero. Ugh.
return nullptr;
}
KJ_FAIL_WIN32("CreateFileMapping", error);
}
KJ_WIN32(mappingHandle = CreateFileMappingW(handle, NULL, pageProtect, 0, 0, NULL));
KJ_DEFER(KJ_WIN32(CloseHandle(mappingHandle)) { break; });
void* mapping = MapViewOfFile(mappingHandle, access,
......@@ -428,6 +419,7 @@ public:
}
Array<const byte> mmap(uint64_t offset, uint64_t size) const {
if (size == 0) return nullptr; // Windows won't allow zero-length mappings
auto range = getMmapRange(offset, size);
const void* mapping = win32Mmap(handle, range, PAGE_READONLY, FILE_MAP_READ);
return Array<const byte>(reinterpret_cast<const byte*>(mapping) + (offset - range.offset),
......@@ -435,6 +427,7 @@ public:
}
Array<byte> mmapPrivate(uint64_t offset, uint64_t size) const {
if (size == 0) return nullptr; // Windows won't allow zero-length mappings
auto range = getMmapRange(offset, size);
void* mapping = win32Mmap(handle, range, PAGE_READONLY, FILE_MAP_COPY);
return Array<byte>(reinterpret_cast<byte*>(mapping) + (offset - range.offset),
......@@ -552,7 +545,8 @@ public:
KJ_REQUIRE(slice.begin() >= bytes.begin() && slice.end() <= bytes.end(),
"byte range is not part of this mapping");
// Zero is treated specially by FlushViewOfFile(), so check for it.
// Zero is treated specially by FlushViewOfFile(), so check for it. (This also handles the
// case where `bytes` is actually empty and not a real mapping.)
if (slice.size() > 0) {
KJ_WIN32(FlushViewOfFile(slice.begin(), slice.size()));
}
......@@ -563,6 +557,10 @@ public:
};
Own<const WritableFileMapping> mmapWritable(uint64_t offset, uint64_t size) const {
if (size == 0) {
// Windows won't allow zero-length mappings
return heap<WritableFileMappingImpl>(nullptr);
}
auto range = getMmapRange(offset, size);
void* mapping = win32Mmap(handle, range, PAGE_READWRITE, FILE_MAP_ALL_ACCESS);
auto array = Array<byte>(reinterpret_cast<byte*>(mapping) + (offset - range.offset),
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment