Make sure error logging or destructors don't throw exception when trying to get file name from handle (#393)

- Bugfix: Make sure error logging or destructors don't throw exception when trying to get file name from handle

7 files changed, 125 insertions, 66 deletions

diff --git a/src/zen/internalfile.cpp b/src/zen/internalfile.cpp
index 5cfd98d9e..9a2011229 100644
--- a/src/zen/internalfile.cpp
+++ b/src/zen/internalfile.cpp
@@ -17,38 +17,40 @@
 
 #include <gsl/gsl-lite.hpp>
 
+namespace zen {
+
 #define ZEN_USE_SLIST ZEN_PLATFORM_WINDOWS
 
 #if ZEN_USE_SLIST == 0
 struct FileBufferManager::Impl
 {
-	zen::RwLock				 m_Lock;
-	std::list<zen::IoBuffer> m_FreeBuffers;
+	RwLock				m_Lock;
+	std::list<IoBuffer> m_FreeBuffers;
 
 	uint64_t m_BufferSize;
 	uint64_t m_MaxBufferCount;
 
 	Impl(uint64_t BufferSize, uint64_t MaxBuffers) : m_BufferSize(BufferSize), m_MaxBufferCount(MaxBuffers) {}
 
-	zen::IoBuffer AllocBuffer()
+	IoBuffer AllocBuffer()
 	{
-		zen::RwLock::ExclusiveLockScope _(m_Lock);
+		RwLock::ExclusiveLockScope _(m_Lock);
 
 		if (m_FreeBuffers.empty())
 		{
-			return zen::IoBuffer{m_BufferSize, 64 * 1024};
+			return IoBuffer{m_BufferSize, 64 * 1024};
 		}
 		else
 		{
-			zen::IoBuffer Buffer = std::move(m_FreeBuffers.front());
+			IoBuffer Buffer = std::move(m_FreeBuffers.front());
 			m_FreeBuffers.pop_front();
 			return Buffer;
 		}
 	}
 
-	void ReturnBuffer(zen::IoBuffer Buffer)
+	void ReturnBuffer(IoBuffer Buffer)
 	{
-		zen::RwLock::ExclusiveLockScope _(m_Lock);
+		RwLock::ExclusiveLockScope _(m_Lock);
 
 		m_FreeBuffers.push_front(std::move(Buffer));
 	}
@@ -58,8 +60,8 @@ struct FileBufferManager::Impl
 {
 	struct BufferItem
 	{
-		SLIST_ENTRY	  ItemEntry;
-		zen::IoBuffer Buffer;
+		SLIST_ENTRY ItemEntry;
+		IoBuffer	Buffer;
 	};
 
 	SLIST_HEADER m_FreeList;
@@ -80,25 +82,25 @@ struct FileBufferManager::Impl
 		}
 	}
 
-	zen::IoBuffer AllocBuffer()
+	IoBuffer AllocBuffer()
 	{
 		SLIST_ENTRY* Entry = InterlockedPopEntrySList(&m_FreeList);
 
 		if (Entry == nullptr)
 		{
-			return zen::IoBuffer{m_BufferSize, 64 * 1024};
+			return IoBuffer{m_BufferSize, 64 * 1024};
 		}
 		else
 		{
-			BufferItem*	  Item	 = reinterpret_cast<BufferItem*>(Entry);
-			zen::IoBuffer Buffer = std::move(Item->Buffer);
+			BufferItem* Item   = reinterpret_cast<BufferItem*>(Entry);
+			IoBuffer	Buffer = std::move(Item->Buffer);
 			delete Item;  // Todo: could keep this around in another list
 
 			return Buffer;
 		}
 	}
 
-	void ReturnBuffer(zen::IoBuffer Buffer)
+	void ReturnBuffer(IoBuffer Buffer)
 	{
 		BufferItem* Item = new BufferItem{nullptr, std::move(Buffer)};
 
@@ -117,14 +119,14 @@ FileBufferManager::~FileBufferManager()
 	delete m_Impl;
 }
 
-zen::IoBuffer
+IoBuffer
 FileBufferManager::AllocBuffer()
 {
 	return m_Impl->AllocBuffer();
 }
 
 void
-FileBufferManager::ReturnBuffer(zen::IoBuffer Buffer)
+FileBufferManager::ReturnBuffer(IoBuffer Buffer)
 {
 	return m_Impl->ReturnBuffer(Buffer);
 }
@@ -138,7 +140,7 @@ InternalFile::InternalFile()
 InternalFile::~InternalFile()
 {
 	if (m_Memory)
-		zen::Memory::Free(m_Memory);
+		Memory::Free(m_Memory);
 
 #if ZEN_PLATFORM_LINUX || ZEN_PLATFORM_MAC
 	if (m_Mmap)
@@ -192,7 +194,7 @@ InternalFile::OpenWrite(std::filesystem::path FileName, bool IsCreate)
 
 	if (Success)
 	{
-		zen::ThrowLastError(fmt::format("Failed to open file for writing: '{}'", FileName));
+		ThrowLastError(fmt::format("Failed to open file for writing: '{}'", FileName));
 	}
 }
 
@@ -217,7 +219,7 @@ InternalFile::OpenRead(std::filesystem::path FileName)
 
 	if (Success)
 	{
-		zen::ThrowLastError(fmt::format("Failed to open file for reading: '{}'", FileName));
+		ThrowLastError(fmt::format("Failed to open file for reading: '{}'", FileName));
 	}
 }
 
@@ -228,10 +230,10 @@ InternalFile::MemoryMapFile()
 
 	if (FileSize <= 100 * 1024 * 1024)
 	{
-		m_Memory = zen::Memory::Alloc(FileSize, 64);
+		m_Memory = Memory::Alloc(FileSize, 64);
 		if (!m_Memory)
 		{
-			zen::ThrowOutOfMemory(fmt::format("failed allocating {:#x} bytes aligned to {:#x}", FileSize, 64));
+			ThrowOutOfMemory(fmt::format("failed allocating {:#x} bytes aligned to {:#x}", FileSize, 64));
 		}
 		Read(m_Memory, FileSize, 0);
 
@@ -269,7 +271,8 @@ InternalFile::Read(void* Data, uint64_t Size, uint64_t Offset)
 
 	if (Success)
 	{
-		zen::ThrowLastError(fmt::format("Failed to read from file '{}'", ""));	// zen::PathFromHandle(m_File)));
+		std::error_code DummyEc;
+		ThrowLastError(fmt::format("Failed to read from file '{}'", PathFromHandle(m_File, DummyEc)));
 	}
 }
 
@@ -294,6 +297,9 @@ InternalFile::Write(const void* Data, uint64_t Size, uint64_t Offset)
 
 	if (Success)
 	{
-		zen::ThrowLastError(fmt::format("Failed to write to file '{}'", zen::PathFromHandle(m_File)));
+		std::error_code DummyEc;
+		ThrowLastError(fmt::format("Failed to write to file '{}'", PathFromHandle(m_File, DummyEc)));
 	}
 }
+
+}  // namespace zen
diff --git a/src/zen/internalfile.h b/src/zen/internalfile.h
index f7bd155fa..22153a460 100644
--- a/src/zen/internalfile.h
+++ b/src/zen/internalfile.h
@@ -15,16 +15,18 @@
 #include <filesystem>
 #include <list>
 
+namespace zen {
+
 //////////////////////////////////////////////////////////////////////////
 
-class FileBufferManager : public zen::RefCounted
+class FileBufferManager : public RefCounted
 {
 public:
 	FileBufferManager(uint64_t BufferSize, uint64_t MaxBufferCount);
 	~FileBufferManager();
 
-	zen::IoBuffer AllocBuffer();
-	void		  ReturnBuffer(zen::IoBuffer Buffer);
+	IoBuffer AllocBuffer();
+	void	 ReturnBuffer(IoBuffer Buffer);
 
 private:
 	struct Impl;
@@ -32,7 +34,7 @@ private:
 	Impl* m_Impl;
 };
 
-class InternalFile : public zen::RefCounted
+class InternalFile : public RefCounted
 {
 public:
 	InternalFile();
@@ -49,8 +51,8 @@ public:
 
 private:
 #if ZEN_PLATFORM_WINDOWS
-	zen::windows::FileHandle  m_File;
-	zen::windows::FileMapping m_Mmap;
+	windows::FileHandle	 m_File;
+	windows::FileMapping m_Mmap;
 #else
 	void* m_File = nullptr;
 	void* m_Mmap = nullptr;
@@ -58,3 +60,5 @@ private:
 
 	void* m_Memory = nullptr;
 };
+
+}  // namespace zen
diff --git a/src/zencore/filesystem.cpp b/src/zencore/filesystem.cpp
index 8abe5af00..b1ec14a37 100644
--- a/src/zencore/filesystem.cpp
+++ b/src/zencore/filesystem.cpp
@@ -1063,19 +1063,25 @@ FileSystemTraversal::TraverseFileSystem(const std::filesystem::path& RootDir, Tr
 	closedir(Dir);
 #endif	// ZEN_PLATFORM_WINDOWS
 }
-
 std::filesystem::path
-PathFromHandle(void* NativeHandle)
+PathFromHandle(void* NativeHandle, std::error_code& Ec)
 {
+	if (NativeHandle == nullptr)
+	{
+		return std::filesystem::path();
+	}
 #if ZEN_PLATFORM_WINDOWS
-	if (NativeHandle == nullptr || NativeHandle == INVALID_HANDLE_VALUE)
+	if (NativeHandle == INVALID_HANDLE_VALUE)
 	{
 		return std::filesystem::path();
 	}
 
-	auto GetFinalPathNameByHandleWRetry = [](HANDLE hFile, LPWSTR lpszFilePath, DWORD cchFilePath, DWORD dwFlags) -> DWORD {
-		while (true)
+	auto GetFinalPathNameByHandleWRetry =
+		[&Ec](HANDLE hFile, LPWSTR lpszFilePath, DWORD cchFilePath, DWORD dwFlags, DWORD& OutRequiredLength) -> DWORD {
+		size_t MaxTries = 5;
+		while (MaxTries > 0)
 		{
+			MaxTries--;
 			DWORD Res = GetFinalPathNameByHandleW(hFile, lpszFilePath, cchFilePath, dwFlags);
 			if (Res == 0)
 			{
@@ -1084,22 +1090,27 @@ PathFromHandle(void* NativeHandle)
 				// Retry if that is the case.
 				if (LastError != ERROR_ACCESS_DENIED)
 				{
-					ThrowSystemError(LastError, fmt::format("failed to get path from file handle {}", hFile));
+					Sleep(2);
+					return LastError;
 				}
 				// Retry
 				continue;
 			}
 			ZEN_ASSERT(Res != 1);  // We don't accept empty path names
-			return Res;
+			OutRequiredLength = Res;
+			return ERROR_SUCCESS;
 		}
+		return ERROR_ACCESS_DENIED;
 	};
 
 	static const DWORD PathDataSize = 512;
 	wchar_t			   PathData[PathDataSize];
-	DWORD			   RequiredLengthIncludingNul = GetFinalPathNameByHandleWRetry(NativeHandle, PathData, PathDataSize, FILE_NAME_OPENED);
-	if (RequiredLengthIncludingNul == 0)
+	DWORD			   RequiredLengthIncludingNul = 0;
+	DWORD Error = GetFinalPathNameByHandleWRetry(NativeHandle, PathData, PathDataSize, FILE_NAME_OPENED, RequiredLengthIncludingNul);
+	if (Error != ERROR_SUCCESS)
 	{
-		ThrowLastError(fmt::format("failed to get path from file handle {}", NativeHandle));
+		Ec = MakeErrorCodeFromLastError();
+		return std::filesystem::path();
 	}
 
 	if (RequiredLengthIncludingNul < PathDataSize)
@@ -1111,7 +1122,13 @@ PathFromHandle(void* NativeHandle)
 	std::wstring FullPath;
 	FullPath.resize(RequiredLengthIncludingNul - 1);
 
-	const DWORD FinalLength = GetFinalPathNameByHandleWRetry(NativeHandle, FullPath.data(), RequiredLengthIncludingNul, FILE_NAME_OPENED);
+	DWORD FinalLength = 0;
+	Error = GetFinalPathNameByHandleWRetry(NativeHandle, FullPath.data(), RequiredLengthIncludingNul, FILE_NAME_OPENED, FinalLength);
+	if (Error != ERROR_SUCCESS)
+	{
+		Ec = MakeErrorCodeFromLastError();
+		return std::filesystem::path();
+	}
 	ZEN_UNUSED(FinalLength);
 	return FullPath;
 
@@ -1123,7 +1140,8 @@ PathFromHandle(void* NativeHandle)
 	ssize_t BytesRead = readlink(Path, Link, sizeof(Link) - 1);
 	if (BytesRead <= 0)
 	{
-		return std::filesystem::path();
+		Ec = MakeErrorCodeFromLastError();
+		return {};
 	}
 
 	Link[BytesRead] = '\0';
@@ -1133,13 +1151,26 @@ PathFromHandle(void* NativeHandle)
 	char Path[MAXPATHLEN];
 	if (fcntl(Fd, F_GETPATH, Path) < 0)
 	{
-		return std::filesystem::path();
+		Ec = MakeErrorCodeFromLastError();
+		return {};
 	}
 
 	return Path;
 #endif	// ZEN_PLATFORM_WINDOWS
 }
 
+std::filesystem::path
+PathFromHandle(void* NativeHandle)
+{
+	std::error_code		  Ec;
+	std::filesystem::path Result = PathFromHandle(NativeHandle, Ec);
+	if (Ec)
+	{
+		throw std::system_error(Ec, fmt::format("failed to get path from file handle '{}'", NativeHandle));
+	}
+	return Result;
+}
+
 uint64_t
 FileSizeFromHandle(void* NativeHandle)
 {
diff --git a/src/zencore/include/zencore/filesystem.h b/src/zencore/include/zencore/filesystem.h
index 37a562664..a78edd16b 100644
--- a/src/zencore/include/zencore/filesystem.h
+++ b/src/zencore/include/zencore/filesystem.h
@@ -34,6 +34,10 @@ ZENCORE_API bool CleanDirectory(const std::filesystem::path& dir);
  */
 ZENCORE_API std::filesystem::path PathFromHandle(void* NativeHandle);
 
+/** Map native file handle to a path
+ */
+ZENCORE_API std::filesystem::path PathFromHandle(void* NativeHandle, std::error_code& Ec);
+
 /** Query file size from native file handle
  */
 ZENCORE_API uint64_t FileSizeFromHandle(void* NativeHandle);
diff --git a/src/zencore/iobuffer.cpp b/src/zencore/iobuffer.cpp
index efec06f7f..73600a09e 100644
--- a/src/zencore/iobuffer.cpp
+++ b/src/zencore/iobuffer.cpp
@@ -332,17 +332,18 @@ IoBufferExtendedCore::Materialize() const
 #endif	// ZEN_PLATFORM_WINDOWS
 		if (Error || (BytesRead != m_DataBytes))
 		{
+			std::error_code DummyEc;
 			ZEN_ERROR("ReadFile/pread failed (offset {:#x}, size {:#x}) file: '{}' (size {:#x}), {}",
 					  m_FileOffset,
 					  m_DataBytes,
-					  zen::PathFromHandle(m_FileHandle),
+					  zen::PathFromHandle(m_FileHandle, DummyEc),
 					  zen::FileSizeFromHandle(m_FileHandle),
 					  GetSystemErrorAsString(Error));
 			throw std::system_error(std::error_code(Error, std::system_category()),
 									fmt::format("ReadFile/pread failed (offset {:#x}, size {:#x}) file: '{}' (size {:#x})",
 												m_FileOffset,
 												m_DataBytes,
-												PathFromHandle(m_FileHandle),
+												PathFromHandle(m_FileHandle, DummyEc),
 												FileSizeFromHandle(m_FileHandle)));
 		}
 
@@ -368,10 +369,11 @@ IoBufferExtendedCore::Materialize() const
 
 	if (NewMmapHandle == nullptr)
 	{
-		int32_t Error = zen::GetLastError();
-		ZEN_ERROR("CreateFileMapping failed on file '{}', {}", zen::PathFromHandle(m_FileHandle), GetSystemErrorAsString(Error));
+		int32_t			Error = zen::GetLastError();
+		std::error_code DummyEc;
+		ZEN_ERROR("CreateFileMapping failed on file '{}', {}", zen::PathFromHandle(m_FileHandle, DummyEc), GetSystemErrorAsString(Error));
 		throw std::system_error(std::error_code(Error, std::system_category()),
-								fmt::format("CreateFileMapping failed on file '{}'", zen::PathFromHandle(m_FileHandle)));
+								fmt::format("CreateFileMapping failed on file '{}'", zen::PathFromHandle(m_FileHandle, DummyEc)));
 	}
 
 	NewFlags |= kOwnsMmap;
@@ -401,17 +403,18 @@ IoBufferExtendedCore::Materialize() const
 		CloseHandle(NewMmapHandle);
 #endif	// ZEN_PLATFORM_WINDOWS
 
+		std::error_code DummyEc;
 		ZEN_ERROR("MapViewOfFile/mmap failed (offset {:#x}, size {:#x}) file: '{}' (size {:#x}), {}",
 				  MapOffset,
 				  MapSize,
-				  zen::PathFromHandle(m_FileHandle),
+				  zen::PathFromHandle(m_FileHandle, DummyEc),
 				  zen::FileSizeFromHandle(m_FileHandle),
 				  GetSystemErrorAsString(Error));
 		throw std::system_error(std::error_code(Error, std::system_category()),
 								fmt::format("MapViewOfFile failed (offset {:#x}, size {:#x}) file: '{}' (size {:#x})",
 											MapOffset,
 											MapSize,
-											zen::PathFromHandle(m_FileHandle),
+											zen::PathFromHandle(m_FileHandle, DummyEc),
 											zen::FileSizeFromHandle(m_FileHandle)));
 	}
 
diff --git a/src/zenhttp/httpclient.cpp b/src/zenhttp/httpclient.cpp
index b77b31933..5ae5f78d6 100644
--- a/src/zenhttp/httpclient.cpp
+++ b/src/zenhttp/httpclient.cpp
@@ -292,26 +292,33 @@ public:
 	TempPayloadFile() : m_FileHandle(nullptr), m_WriteOffset(0) {}
 	~TempPayloadFile()
 	{
-		if (m_FileHandle)
+		try
 		{
+			if (m_FileHandle)
+			{
 #if ZEN_PLATFORM_WINDOWS
-			// Mark file for deletion when final handle is closed
-			FILE_DISPOSITION_INFO Fdi{.DeleteFile = TRUE};
+				// Mark file for deletion when final handle is closed
+				FILE_DISPOSITION_INFO Fdi{.DeleteFile = TRUE};
 
-			SetFileInformationByHandle(m_FileHandle, FileDispositionInfo, &Fdi, sizeof Fdi);
-			BOOL Success = CloseHandle(m_FileHandle);
+				SetFileInformationByHandle(m_FileHandle, FileDispositionInfo, &Fdi, sizeof Fdi);
+				BOOL Success = CloseHandle(m_FileHandle);
 #else
-			std::filesystem::path FilePath = zen::PathFromHandle(m_FileHandle);
-			unlink(FilePath.c_str());
-			int	 Fd		 = int(uintptr_t(m_FileHandle));
-			bool Success = (close(Fd) == 0);
+				std::filesystem::path FilePath = zen::PathFromHandle(m_FileHandle);
+				unlink(FilePath.c_str());
+				int	 Fd		 = int(uintptr_t(m_FileHandle));
+				bool Success = (close(Fd) == 0);
 #endif
-			if (!Success)
-			{
-				ZEN_WARN("Error reported on file handle close, reason '{}'", GetLastErrorAsString());
-			}
+				if (!Success)
+				{
+					ZEN_WARN("Error reported on file handle close, reason '{}'", GetLastErrorAsString());
+				}
 
-			m_FileHandle = nullptr;
+				m_FileHandle = nullptr;
+			}
+		}
+		catch (std::exception& Ex)
+		{
+			ZEN_ERROR("Failed deleting temp file {}. Reason '{}'", m_FileHandle, Ex.what());
 		}
 	}
 
diff --git a/src/zenutil/basicfile.cpp b/src/zenutil/basicfile.cpp
index 84e8bed85..99aa6bf39 100644
--- a/src/zenutil/basicfile.cpp
+++ b/src/zenutil/basicfile.cpp
@@ -452,8 +452,12 @@ TemporaryFile::Close()
 
 		SetFileInformationByHandle(m_FileHandle, FileDispositionInfo, &Fdi, sizeof Fdi);
 #else
-		std::filesystem::path FilePath = zen::PathFromHandle(m_FileHandle);
-		unlink(FilePath.c_str());
+		std::error_code		  Ec;
+		std::filesystem::path FilePath = zen::PathFromHandle(m_FileHandle, Ec);
+		if (!Ec)
+		{
+			unlink(FilePath.c_str());
+		}
 #endif
 
 		BasicFile::Close();