Skip to content

ARROW-1096: [C++] CreateFileMapping maximum size calculation issue #756

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.

Sign up for GitHub

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

Closed
maxhora wants to merge 1 commit into from
Closed

ARROW-1096: [C++] CreateFileMapping maximum size calculation issue #756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions cpp/src/arrow/io/io-file-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,41 @@ TEST_F(TestMemoryMappedFile, ReadOnly) {
rommap->Close();
}

TEST_F(TestMemoryMappedFile, DISABLED_ReadWriteOver4GbFile) {
// ARROW-1096
const int64_t buffer_size = 1000 * 1000;
std::vector<uint8_t> buffer(buffer_size);

test::random_bytes(buffer_size, 0, buffer.data());

const int64_t reps = 5000;

std::string path = "ipc-read-over-4gb-file-test";
std::shared_ptr<MemoryMappedFile> rwmmap;
ASSERT_OK(InitMemoryMap(reps * buffer_size, path, &rwmmap));
AppendFile(path);
Copy link
Contributor Author

@maxhora maxhora Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That adds file to be deleted on TearDown


int64_t position = 0;
for (int i = 0; i < reps; ++i) {
ASSERT_OK(rwmmap->Write(buffer.data(), buffer_size));
position += buffer_size;
}
rwmmap->Close();

std::shared_ptr<MemoryMappedFile> rommap;
ASSERT_OK(MemoryMappedFile::Open(path, FileMode::READ, &rommap));

position = 0;
std::shared_ptr<Buffer> out_buffer;
for (int i = 0; i < reps; ++i) {
ASSERT_OK(rommap->ReadAt(position, buffer_size, &out_buffer));

ASSERT_EQ(0, memcmp(out_buffer->data(), buffer.data(), buffer_size));
position += buffer_size;
}
rommap->Close();
}

TEST_F(TestMemoryMappedFile, RetainMemoryMapReference) {
// ARROW-494

Expand Down
9 changes: 3 additions & 6 deletions cpp/src/arrow/io/mman.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,10 @@ static void* mmap(void* addr, size_t len, int prot, int flags, int fildes, off_t
const DWORD protect = __map_mmap_prot_page(prot);
const DWORD desiredAccess = __map_mmap_prot_file(prot);

const off_t maxSize = off + (off_t)len;
Copy link
Contributor Author

@maxhora maxhora Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSVC defines off_t as

typedef long _off_t; // file offset value

msvc long type width is 32 bits and it's not enough to keep 64 bits of size_t(unsigned long long).
File size over 4Gb needs more than 32 bits, that caused the issue.

const size_t maxSize = off + len;

const DWORD dwMaxSizeLow =
(sizeof(off_t) <= sizeof(DWORD)) ? (DWORD)maxSize : (DWORD)(maxSize & 0xFFFFFFFFL);
const DWORD dwMaxSizeHigh = (sizeof(off_t) <= sizeof(DWORD))
? (DWORD)0
: (DWORD)((maxSize >> 32) & 0xFFFFFFFFL);
const DWORD dwMaxSizeLow = static_cast<DWORD>(maxSize & 0xFFFFFFFFL);
const DWORD dwMaxSizeHigh = static_cast<DWORD>((maxSize >> 32) & 0xFFFFFFFFL);

#ifdef _MSC_VER
#pragma warning(pop)
Expand Down
2 changes: 2 additions & 0 deletions cpp/src/arrow/io/test-common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ class MemoryMapFixture {
return Status::OK();
}

void AppendFile(const std::string& path) { tmp_files_.push_back(path); }

private:
std::vector<std::string> tmp_files_;
};
Expand Down