diff --git a/include/cpp_utils/io/memory_mapped_file.hpp b/include/cpp_utils/io/memory_mapped_file.hpp index 4774ab5..3e88d8a 100644 --- a/include/cpp_utils/io/memory_mapped_file.hpp +++ b/include/cpp_utils/io/memory_mapped_file.hpp @@ -84,8 +84,8 @@ struct memory_mapped_file #ifdef USE_MMAP if (fd = open(path.c_str(), O_RDONLY, static_cast(0600)); fd != -1) { - mapped_file = details::checked_mmap(mmap( - nullptr, this->f_size, PROT_READ, MAP_FILE | MAP_PRIVATE, fd, 0UL)); + mapped_file = details::checked_mmap( + mmap(nullptr, this->f_size, PROT_READ, MAP_FILE | MAP_PRIVATE, fd, 0UL)); if (mapped_file == nullptr) { close(fd); @@ -109,22 +109,25 @@ struct memory_mapped_file } } } - ~memory_mapped_file() + // A mapping owns a file descriptor and an address range (or Windows handles): a copy would + // unmap twice, so copies are deleted and only ownership transfer is allowed. + memory_mapped_file(const memory_mapped_file&) = delete; + memory_mapped_file& operator=(const memory_mapped_file&) = delete; + + memory_mapped_file(memory_mapped_file&& other) noexcept { adopt(other); } + + memory_mapped_file& operator=(memory_mapped_file&& other) noexcept { - if (mapped_file) + if (this != &other) { -#ifdef USE_MMAP - munmap(mapped_file, f_size); - close(fd); -#endif -#ifdef USE_MapViewOfFile - ::UnmapViewOfFile(mapped_file); - ::CloseHandle(hFile); - ::CloseHandle(hMapFile); -#endif + release(); + adopt(other); } + return *this; } + ~memory_mapped_file() { release(); } + auto read(const std::size_t offset, const std::size_t) const { return mapped_file + offset; } template @@ -167,6 +170,52 @@ struct memory_mapped_file return hFile != NULL && hMapFile != NULL && mapped_file != nullptr; #endif } + +private: + void release() noexcept + { + if (mapped_file) + { +#ifdef USE_MMAP + munmap(mapped_file, f_size); + close(fd); +#endif +#ifdef USE_MapViewOfFile + ::UnmapViewOfFile(mapped_file); + ::CloseHandle(hFile); + ::CloseHandle(hMapFile); +#endif + } + reset(); + } + + void adopt(memory_mapped_file& other) noexcept + { +#ifdef USE_MMAP + fd = other.fd; +#endif +#ifdef USE_MapViewOfFile + hMapFile = other.hMapFile; + hFile = other.hFile; +#endif + mapped_file = other.mapped_file; + f_size = other.f_size; + other.reset(); + } + + // Leaves the object owning nothing, so its destructor is a no-op. + void reset() noexcept + { +#ifdef USE_MMAP + fd = -1; +#endif +#ifdef USE_MapViewOfFile + hMapFile = NULL; + hFile = NULL; +#endif + mapped_file = nullptr; + f_size = 0UL; + } }; static_assert(std::ranges::contiguous_range); diff --git a/tests/io/test_memory_mapped_file.cpp b/tests/io/test_memory_mapped_file.cpp index 854e8d1..e891267 100644 --- a/tests/io/test_memory_mapped_file.cpp +++ b/tests/io/test_memory_mapped_file.cpp @@ -156,3 +156,67 @@ TEST_CASE("memory_mapped_file cleans up its fd when mmap() itself fails", "[io]" std::filesystem::remove(path); } #endif + +TEST_CASE("memory_mapped_file survives a move and unmaps only once", "[io]") +{ + auto path = std::filesystem::temp_directory_path() / "cpp_utils_mmf_move_test.txt"; + { + std::ofstream out(path, std::ios::binary); + out << "hello world"; + } + + // The moved-from object dies before the data is read: if it still owned the mapping its + // destructor would unmap the bytes the moved-to object is about to read. + auto moved = [&path] + { + memory_mapped_file source { path.string() }; + REQUIRE(source.is_valid()); + memory_mapped_file target { std::move(source) }; + REQUIRE_FALSE(source.is_valid()); + REQUIRE(source.data() == nullptr); + REQUIRE(source.size() == 0); + return target; + }(); + + REQUIRE(moved.is_valid()); + REQUIRE(moved.size() == 11); + REQUIRE(std::string(moved.data(), moved.size()) == "hello world"); + + std::filesystem::remove(path); +} + +TEST_CASE("memory_mapped_file move assignment releases the previous mapping", "[io]") +{ + auto first = std::filesystem::temp_directory_path() / "cpp_utils_mmf_move_a.txt"; + auto second = std::filesystem::temp_directory_path() / "cpp_utils_mmf_move_b.txt"; + { + std::ofstream out(first, std::ios::binary); + out << "first"; + } + { + std::ofstream out(second, std::ios::binary); + out << "second payload"; + } + + memory_mapped_file target { first.string() }; + REQUIRE(std::string(target.data(), target.size()) == "first"); + + { + memory_mapped_file source { second.string() }; + target = std::move(source); + REQUIRE_FALSE(source.is_valid()); + } + + REQUIRE(target.is_valid()); + REQUIRE(target.size() == 14); + REQUIRE(std::string(target.data(), target.size()) == "second payload"); + + std::filesystem::remove(first); + std::filesystem::remove(second); +} + +// Owning a raw mapping/handle means copies would unmap twice; CEFast already hit this. +static_assert(!std::is_copy_constructible_v); +static_assert(!std::is_copy_assignable_v); +static_assert(std::is_move_constructible_v); +static_assert(std::is_move_assignable_v);