promoted ScopeFd to file scope and applied it to tests and other codesb/block-clone

fixes potential file handle leak in ScanFile (if callback throws) and generally makes code more maintainable and aligned with how handles are typically managed on Windows

1 files changed, 94 insertions, 92 deletions

diff --git a/src/zencore/filesystem.cpp b/src/zencore/filesystem.cpp
index 14ecfe8df..a64fb6d66 100644
--- a/src/zencore/filesystem.cpp
+++ b/src/zencore/filesystem.cpp
@@ -69,6 +69,53 @@ namespace zen {
 
 using namespace std::literals;
 
+#if ZEN_PLATFORM_LINUX || ZEN_PLATFORM_MAC
+struct ScopedFd
+{
+	int Fd = -1;
+
+	ScopedFd() = default;
+	explicit ScopedFd(int InFd) : Fd(InFd) {}
+
+	~ScopedFd()
+	{
+		if (Fd >= 0)
+		{
+			close(Fd);
+		}
+	}
+
+	ScopedFd(const ScopedFd&) = delete;
+	ScopedFd& operator=(const ScopedFd&) = delete;
+
+	ScopedFd(ScopedFd&& Other) noexcept : Fd(Other.Fd) { Other.Fd = -1; }
+
+	ScopedFd& operator=(ScopedFd&& Other) noexcept
+	{
+		if (this != &Other)
+		{
+			if (Fd >= 0)
+			{
+				close(Fd);
+			}
+			Fd		 = Other.Fd;
+			Other.Fd = -1;
+		}
+		return *this;
+	}
+
+	// Release ownership of the file descriptor, returning it without closing
+	int Release()
+	{
+		int Result = Fd;
+		Fd		   = -1;
+		return Result;
+	}
+
+	explicit operator bool() const { return Fd >= 0; }
+};
+#endif	// ZEN_PLATFORM_LINUX || ZEN_PLATFORM_MAC
+
 #if ZEN_PLATFORM_WINDOWS
 
 static bool
@@ -1173,43 +1220,28 @@ TryCloneFile(const std::filesystem::path& FromPath, const std::filesystem::path&
 	return TryCloneFile((void*)FromFile.m_Handle, (void*)TargetFile.m_Handle);
 
 #elif ZEN_PLATFORM_LINUX
-	struct ScopedFd
-	{
-		~ScopedFd()
-		{
-			if (Fd >= 0)
-			{
-				close(Fd);
-			}
-		}
-		int Fd;
-	};
-
 	// The 'from' file
-	int FromFd = open(FromPath.c_str(), O_RDONLY | O_CLOEXEC);
-	if (FromFd < 0)
+	ScopedFd FromFd(open(FromPath.c_str(), O_RDONLY | O_CLOEXEC));
+	if (!FromFd)
 	{
 		return false;
 	}
-	ScopedFd $From = {FromFd};
 
 	// Remove any existing target so we can create a fresh clone
 	unlink(ToPath.c_str());
 
 	// The 'to' file
-	int ToFd = open(ToPath.c_str(), O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, 0666);
-	if (ToFd < 0)
+	ScopedFd ToFd(open(ToPath.c_str(), O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, 0666));
+	if (!ToFd)
 	{
 		return false;
 	}
-	ScopedFd $To = {ToFd};
 
-	if (ioctl(ToFd, FICLONE, FromFd) != 0)
+	if (ioctl(ToFd.Fd, FICLONE, FromFd.Fd) != 0)
 	{
 		// Clone not supported by this filesystem or files are on different volumes.
 		// Remove the empty target file we created.
-		close(ToFd);
-		$To.Fd = -1;
+		ToFd = ScopedFd();
 		unlink(ToPath.c_str());
 		return false;
 	}
@@ -1274,49 +1306,41 @@ CopyFile(const std::filesystem::path& FromPath, const std::filesystem::path& ToP
 							  &CancelFlag,
 							  /* dwCopyFlags */ 0);
 #else
-	struct ScopedFd
-	{
-		~ScopedFd() { close(Fd); }
-		int Fd;
-	};
-
 	// From file
-	int FromFd = open(FromPath.c_str(), O_RDONLY | O_CLOEXEC);
-	if (FromFd < 0)
+	ScopedFd FromFd(open(FromPath.c_str(), O_RDONLY | O_CLOEXEC));
+	if (!FromFd)
 	{
 		ThrowLastError(fmt::format("failed to open file {}", FromPath));
 	}
-	ScopedFd $From = {FromFd};
 
 	// To file
-	int ToFd = open(ToPath.c_str(), O_WRONLY | O_CREAT | O_CLOEXEC, 0666);
-	if (ToFd < 0)
+	ScopedFd ToFd(open(ToPath.c_str(), O_WRONLY | O_CREAT | O_CLOEXEC, 0666));
+	if (!ToFd)
 	{
 		ThrowLastError(fmt::format("failed to create file {}", ToPath));
 	}
-	fchmod(ToFd, 0666);
-	ScopedFd $To = {ToFd};
+	fchmod(ToFd.Fd, 0666);
 
 	struct stat Stat;
-	fstat(FromFd, &Stat);
+	fstat(FromFd.Fd, &Stat);
 
 	size_t FileSizeBytes = Stat.st_size;
 
-	fchown(ToFd, Stat.st_uid, Stat.st_gid);
+	fchown(ToFd.Fd, Stat.st_uid, Stat.st_gid);
 
 	// Copy impl
 	const size_t BufferSize = Min(FileSizeBytes, 64u << 10);
 	void*		 Buffer		= malloc(BufferSize);
 	while (true)
 	{
-		int BytesRead = read(FromFd, Buffer, BufferSize);
+		int BytesRead = read(FromFd.Fd, Buffer, BufferSize);
 		if (BytesRead <= 0)
 		{
 			Success = (BytesRead == 0);
 			break;
 		}
 
-		if (write(ToFd, Buffer, BytesRead) != BytesRead)
+		if (write(ToFd.Fd, Buffer, BytesRead) != BytesRead)
 		{
 			Success = false;
 			break;
@@ -1560,20 +1584,20 @@ WriteFile(std::filesystem::path Path, const IoBuffer* const* Data, size_t Buffer
 	}
 
 #else
-	int OpenFlags = O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC;
-	int Fd		  = open(Path.c_str(), OpenFlags, 0666);
-	if (Fd < 0)
+	int		 OpenFlags = O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC;
+	ScopedFd OutFd(open(Path.c_str(), OpenFlags, 0666));
+	if (!OutFd)
 	{
 		zen::CreateDirectories(Path.parent_path());
-		Fd = open(Path.c_str(), OpenFlags, 0666);
+		OutFd = ScopedFd(open(Path.c_str(), OpenFlags, 0666));
 	}
 
-	if (Fd < 0)
+	if (!OutFd)
 	{
 		ThrowLastError(fmt::format("File open failed for '{}'", Path));
 	}
 
-	fchmod(Fd, 0666);
+	fchmod(OutFd.Fd, 0666);
 #endif
 
 	// TODO: this should be block-enlightened
@@ -1597,9 +1621,9 @@ WriteFile(std::filesystem::path Path, const IoBuffer* const* Data, size_t Buffer
 				ThrowSystemException(hRes, fmt::format("File write failed for '{}'", Path).c_str());
 			}
 #else
-			if (write(Fd, DataPtr, WriteSize) != int64_t(WriteSize))
+			if (write(OutFd.Fd, DataPtr, WriteSize) != int64_t(WriteSize))
 			{
-				close(Fd);
+				OutFd = ScopedFd();
 				std::error_code DummyEc;
 				RemoveFile(Path, DummyEc);
 				ThrowLastError(fmt::format("File write failed for '{}'", Path));
@@ -1613,8 +1637,6 @@ WriteFile(std::filesystem::path Path, const IoBuffer* const* Data, size_t Buffer
 
 #if ZEN_PLATFORM_WINDOWS
 	Outfile.Close();
-#else
-	close(Fd);
 #endif
 }
 
@@ -1896,8 +1918,8 @@ ScanFile(std::filesystem::path Path, const uint64_t ChunkSize, std::function<voi
 		ProcessFunc(ReadBuffer.data(), dwBytesRead);
 	}
 #else
-	int Fd = open(Path.c_str(), O_RDONLY | O_CLOEXEC);
-	if (Fd < 0)
+	ScopedFd InFd(open(Path.c_str(), O_RDONLY | O_CLOEXEC));
+	if (!InFd)
 	{
 		return false;
 	}
@@ -1907,7 +1929,7 @@ ScanFile(std::filesystem::path Path, const uint64_t ChunkSize, std::function<voi
 	void* Buffer = malloc(ChunkSize);
 	while (true)
 	{
-		int BytesRead = read(Fd, Buffer, ChunkSize);
+		int BytesRead = read(InFd.Fd, Buffer, ChunkSize);
 		if (BytesRead < 0)
 		{
 			Success = false;
@@ -1923,7 +1945,6 @@ ScanFile(std::filesystem::path Path, const uint64_t ChunkSize, std::function<voi
 	}
 
 	free(Buffer);
-	close(Fd);
 
 	if (!Success)
 	{
@@ -3312,28 +3333,26 @@ public:
 		ZEN_UNUSED(SystemGlobal);
 		std::string InstanceMapName = fmt::format("/{}", Name);
 
-		int Fd = shm_open(InstanceMapName.c_str(), O_RDWR, 0666);
-		if (Fd < 0)
+		ScopedFd FdGuard(shm_open(InstanceMapName.c_str(), O_RDWR, 0666));
+		if (!FdGuard)
 		{
 			return {};
 		}
-		void* hMap = (void*)intptr_t(Fd);
 
 		struct stat Stat;
-		fstat(Fd, &Stat);
+		fstat(FdGuard.Fd, &Stat);
 
 		if (size_t(Stat.st_size) < Size)
 		{
-			close(Fd);
 			return {};
 		}
 
-		void* pBuf = mmap(nullptr, Size, PROT_READ | PROT_WRITE, MAP_SHARED, Fd, 0);
+		void* pBuf = mmap(nullptr, Size, PROT_READ | PROT_WRITE, MAP_SHARED, FdGuard.Fd, 0);
 		if (pBuf == MAP_FAILED)
 		{
-			close(Fd);
 			return {};
 		}
+		void* hMap = (void*)intptr_t(FdGuard.Release());
 		return Data{.Handle = hMap, .DataPtr = pBuf, .Size = Size, .Name = std::string(Name)};
 #endif	// ZEN_PLATFORM_LINUX || ZEN_PLATFORM_MAC
 	}
@@ -3388,23 +3407,22 @@ public:
 		ZEN_UNUSED(SystemGlobal);
 		std::string InstanceMapName = fmt::format("/{}", Name);
 
-		int Fd = shm_open(InstanceMapName.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0666);
-		if (Fd < 0)
+		ScopedFd FdGuard(shm_open(InstanceMapName.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0666));
+		if (!FdGuard)
 		{
 			return {};
 		}
-		fchmod(Fd, 0666);
-		void* hMap = (void*)intptr_t(Fd);
+		fchmod(FdGuard.Fd, 0666);
 
-		int Result = ftruncate(Fd, Size);
+		int Result = ftruncate(FdGuard.Fd, Size);
 		ZEN_UNUSED(Result);
 
-		void* pBuf = mmap(nullptr, Size, PROT_READ | PROT_WRITE, MAP_SHARED, Fd, 0);
+		void* pBuf = mmap(nullptr, Size, PROT_READ | PROT_WRITE, MAP_SHARED, FdGuard.Fd, 0);
 		if (pBuf == MAP_FAILED)
 		{
-			close(Fd);
 			return {};
 		}
+		void* hMap = (void*)intptr_t(FdGuard.Release());
 		return Data{.Handle = hMap, .DataPtr = pBuf, .Size = Size, .Name = std::string(Name)};
 #endif	// ZEN_PLATFORM_LINUX || ZEN_PLATFORM_MAC
 	}
@@ -3945,13 +3963,13 @@ TEST_CASE("CloneQueryInterface")
 		CHECK(DstHandle != INVALID_HANDLE_VALUE);
 		void* DstNativeHandle = (void*)DstHandle.m_Handle;
 #	else
-		int SrcFd = open(SrcPath.c_str(), O_RDONLY | O_CLOEXEC);
-		CHECK(SrcFd >= 0);
-		void* SrcNativeHandle = (void*)uintptr_t(SrcFd);
+		ScopedFd SrcFd(open(SrcPath.c_str(), O_RDONLY | O_CLOEXEC));
+		CHECK(bool(SrcFd));
+		void* SrcNativeHandle = (void*)uintptr_t(SrcFd.Fd);
 
-		int DstFd = open(DstPath.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0666);
-		CHECK(DstFd >= 0);
-		void* DstNativeHandle = (void*)uintptr_t(DstFd);
+		ScopedFd DstFd(open(DstPath.c_str(), O_RDWR | O_CREAT | O_CLOEXEC, 0666));
+		CHECK(bool(DstFd));
+		void* DstNativeHandle = (void*)uintptr_t(DstFd.Fd);
 #	endif
 
 		SUBCASE("CanClone returns true for same volume") { CHECK(CloneQuery->CanClone(SrcNativeHandle)); }
@@ -3983,19 +4001,17 @@ TEST_CASE("CloneQueryInterface")
 						Ov.OffsetHigh	   = (DWORD)(Clonable >> 32);
 						::WriteFile(DstHandle, SrcData, (DWORD)PostBytes, &Written, &Ov);
 #	else
-						pwrite(DstFd, SrcData, PostBytes, Clonable);
+						pwrite(DstFd.Fd, SrcData, PostBytes, Clonable);
 #	endif
 					}
 
-					// Verify the cloned file content matches
+					// Close handles before reading back the file for verification
 #	if ZEN_PLATFORM_WINDOWS
 					SrcHandle.Close();
 					DstHandle.Close();
 #	else
-					close(SrcFd);
-					close(DstFd);
-					SrcFd = -1;
-					DstFd = -1;
+					SrcFd = ScopedFd();
+					DstFd = ScopedFd();
 #	endif
 
 					FileContents DstContents = ReadFile(DstPath);
@@ -4006,20 +4022,6 @@ TEST_CASE("CloneQueryInterface")
 				}
 			}
 		}
-
-		// Cleanup handles if still open
-#	if ZEN_PLATFORM_WINDOWS
-		// windows::Handle auto-closes
-#	else
-		if (SrcFd >= 0)
-		{
-			close(SrcFd);
-		}
-		if (DstFd >= 0)
-		{
-			close(DstFd);
-		}
-#	endif
 	}
 	else
 	{