commit 1426e3f: [Minor] More tests and fixes to raii file

[Vsevolod Stakhov]

Author: Vsevolod Stakhov
Date: 2022-10-16 15:22:10 +0100
URL: https://github.com/rspamd/rspamd/commit/1426e3fe4ca1cd248e1cf2e35a3f946000b031e2

[Minor] More tests and fixes to raii file

---
 src/libutil/cxx/locked_file.cxx | 60 +++++++++++++++++++++++++----------------
 src/libutil/cxx/locked_file.hxx | 19 +++++++++++++
 2 files changed, 56 insertions(+), 23 deletions(-)

diff --git a/src/libutil/cxx/locked_file.cxx b/src/libutil/cxx/locked_file.cxx
index 4c91f44ed..c972e1de3 100644
--- a/src/libutil/cxx/locked_file.cxx
+++ b/src/libutil/cxx/locked_file.cxx
@@ -41,11 +41,6 @@ auto raii_file::open(const char *fname, int flags) -> tl::expected<raii_file, st
 		return tl::make_unexpected(fmt::format("cannot open file {}: {}", fname, ::strerror(errno)));
 	}
 
-	if (!rspamd_file_lock(fd, TRUE)) {
-		close(fd);
-		return tl::make_unexpected(fmt::format("cannot lock file {}: {}", fname, ::strerror(errno)));
-	}
-
 	auto ret = raii_file{fname, fd, false};
 
 	if (fstat(ret.fd, &ret.st) == -1) {
@@ -72,11 +67,6 @@ auto raii_file::create(const char *fname, int flags, int perms) -> tl::expected<
 		return tl::make_unexpected(fmt::format("cannot create file {}: {}", fname, ::strerror(errno)));
 	}
 
-	if (!rspamd_file_lock(fd, TRUE)) {
-		close(fd);
-		return tl::make_unexpected(fmt::format("cannot lock file {}: {}", fname, ::strerror(errno)));
-	}
-
 	auto ret = raii_file{fname, fd, false};
 
 	if (fstat(ret.fd, &ret.st) == -1) {
@@ -102,12 +92,6 @@ auto raii_file::create_temp(const char *fname, int flags, int perms) -> tl::expe
 		return tl::make_unexpected(fmt::format("cannot create file {}: {}", fname, ::strerror(errno)));
 	}
 
-	if (!rspamd_file_lock(fd, TRUE)) {
-		unlink(fname);
-		close(fd);
-		return tl::make_unexpected(fmt::format("cannot lock file {}: {}", fname, ::strerror(errno)));
-	}
-
 	auto ret = raii_file{fname, fd, true};
 
 	if (fstat(ret.fd, &ret.st) == -1) {
@@ -135,12 +119,6 @@ auto raii_file::mkstemp(const char *pattern, int flags, int perms) -> tl::expect
 		return tl::make_unexpected(fmt::format("cannot create file {}: {}", pattern, ::strerror(errno)));
 	}
 
-	if (!rspamd_file_lock(fd, TRUE)) {
-		close(fd);
-		(void)unlink(mutable_pattern.c_str());
-		return tl::make_unexpected(fmt::format("cannot lock file {}: {}", pattern, ::strerror(errno)));
-	}
-
 	auto ret = raii_file{mutable_pattern.c_str(), fd, true};
 
 	if (fstat(ret.fd, &ret.st) == -1) {
@@ -157,11 +135,15 @@ raii_file::~raii_file() noexcept
 		if (temp) {
 			(void)unlink(fname.c_str());
 		}
-		(void) rspamd_file_unlock(fd, FALSE);
 		close(fd);
 	}
 }
 
+auto raii_file::update_stat() noexcept -> bool
+{
+	return fstat(fd, &st) != -1;
+}
+
 
 raii_locked_file::~raii_locked_file() noexcept
 {
@@ -197,6 +179,8 @@ auto raii_mmaped_file::mmap_shared(raii_file &&file,
 {
 	void *map;
 
+	/* Update stat on file to ensure it is up-to-date */
+	file.update_stat();
 	map = mmap(NULL, file.get_stat().st_size, flags, MAP_SHARED, file.get_fd(), 0);
 
 	if (map == MAP_FAILED) {
@@ -386,6 +370,36 @@ TEST_CASE("tempfile") {
 	CHECK(serrno == ENOENT);
 }
 
+TEST_CASE("mmap") {
+	std::string tmpname;
+	{
+		auto raii_file = raii_file::mkstemp("/tmp//doctest-XXXXXXXX",
+		O_RDWR|O_CREAT|O_EXCL, 00600);
+		CHECK(raii_file.has_value());
+		CHECK(raii_file->get_dir() == "/tmp");
+		CHECK(access(raii_file->get_name().data(), R_OK) == 0);
+		tmpname = std::string{raii_file->get_name()};
+		char payload[] = {'1', '2', '3'};
+		CHECK(write(raii_file->get_fd(), payload, sizeof(payload)) == sizeof(payload));
+		auto mmapped_file1 = raii_mmaped_file::mmap_shared(std::move(raii_file.value()), PROT_READ|PROT_WRITE);
+		CHECK(mmapped_file1.has_value());
+		CHECK(!raii_file->is_valid());
+		CHECK(mmapped_file1->get_size() == sizeof(payload));
+		CHECK(memcmp(mmapped_file1->get_map(), payload, sizeof(payload)) == 0);
+		*(char *)mmapped_file1->get_map() = '2';
+		auto mmapped_file2 = raii_mmaped_file::mmap_shared(tmpname.c_str(), O_RDONLY, PROT_READ);
+		CHECK(mmapped_file2.has_value());
+		CHECK(mmapped_file2->get_size() == sizeof(payload));
+		CHECK(memcmp(mmapped_file2->get_map(), payload, sizeof(payload)) != 0);
+		CHECK(memcmp(mmapped_file2->get_map(), mmapped_file1->get_map(), sizeof(payload)) == 0);
+	}
+	// File must be deleted after this call
+	auto ret = ::access(tmpname.c_str(), R_OK);
+	auto serrno = errno;
+	CHECK(ret == -1);
+	CHECK(serrno == ENOENT);
+}
+
 } // TEST_SUITE
 
 } // namespace tests
diff --git a/src/libutil/cxx/locked_file.hxx b/src/libutil/cxx/locked_file.hxx
index c7d286cbb..507b02762 100644
--- a/src/libutil/cxx/locked_file.hxx
+++ b/src/libutil/cxx/locked_file.hxx
@@ -44,6 +44,10 @@ public:
 		return st;
 	};
 
+	auto get_size() const -> std::size_t {
+		return st.st_size;
+	};
+
 	auto get_name() const -> std::string_view {
 		return std::string_view{fname};
 	}
@@ -93,10 +97,24 @@ public:
 		*this = std::move(other);
 	}
 
+	/**
+	 * Prevent file from being deleted
+	 * @return
+	 */
 	auto make_immortal() noexcept {
 		temp = false;
 	}
 
+	/**
+	 * Performs fstat on an opened file to refresh internal stat
+	 * @return
+	 */
+	auto update_stat() noexcept -> bool;
+
+	auto is_valid() noexcept -> bool {
+		return fd != -1;
+	}
+
 	/* Do not allow copy/default ctor */
 	const raii_file& operator=(const raii_file &other) = delete;
 	raii_file() = delete;
@@ -181,6 +199,7 @@ struct raii_mmaped_file final {
 	static auto mmap_shared(const char *fname, int open_flags, int mmap_flags) -> tl::expected<raii_mmaped_file, std::string>;
 	// Returns a constant pointer to the underlying map
 	auto get_map() const -> void* {return map;}
+	auto get_file() const -> const raii_file& { return file; }
 	// Passes the ownership of the mmaped memory to the callee
 	auto steal_map() -> std::tuple<void *, std::size_t> {
 		auto ret = std::make_tuple(this->map, file.get_stat().st_size);