safer path from handle (#195)

* remove PathFromHandle that throws to give better context on failures

9 files changed, 104 insertions, 65 deletions

diff --git a/src/zencore/filesystem.cpp b/src/zencore/filesystem.cpp
index 7ca076daa..ac2aabbf0 100644
--- a/src/zencore/filesystem.cpp
+++ b/src/zencore/filesystem.cpp
@@ -891,8 +891,13 @@ MoveToFile(std::filesystem::path Path, IoBuffer Data)
 		return false;
 	}
 #elif ZEN_PLATFORM_LINUX || ZEN_PLATFORM_MAC
-	std::filesystem::path SourcePath = PathFromHandle(FileRef.FileHandle);
-	int					  Ret		 = link(SourcePath.c_str(), Path.c_str());
+	std::error_code		  Ec;
+	std::filesystem::path SourcePath = PathFromHandle(FileRef.FileHandle, Ec);
+	if (Ec)
+	{
+		return false;
+	}
+	int Ret = link(SourcePath.c_str(), Path.c_str());
 	if (Ret < 0)
 	{
 		int32_t err = errno;
@@ -1409,18 +1414,6 @@ PathFromHandle(void* NativeHandle, std::error_code& Ec)
 #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)
 {
@@ -1742,7 +1735,9 @@ TEST_CASE("filesystem")
 	Handle = (void*)uintptr_t(Fd);
 #	endif
 
-	auto FromHandle = PathFromHandle((void*)uintptr_t(Handle));
+	std::error_code Ec;
+	auto			FromHandle = PathFromHandle((void*)uintptr_t(Handle), Ec);
+	CHECK(!Ec);
 	CHECK(equivalent(FromHandle, BinPath));
 
 #	if ZEN_PLATFORM_WINDOWS
diff --git a/src/zencore/include/zencore/filesystem.h b/src/zencore/include/zencore/filesystem.h
index 0aab6a4ae..897a63d8c 100644
--- a/src/zencore/include/zencore/filesystem.h
+++ b/src/zencore/include/zencore/filesystem.h
@@ -35,10 +35,6 @@ ZENCORE_API bool CleanDirectoryExceptDotFiles(const std::filesystem::path& dir);
 
 /** Map native file handle to a path
  */
-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);
 
 /** Get canonical path name from a generic path
diff --git a/src/zencore/iobuffer.cpp b/src/zencore/iobuffer.cpp
index fdee4e5e5..604cd90d6 100644
--- a/src/zencore/iobuffer.cpp
+++ b/src/zencore/iobuffer.cpp
@@ -239,8 +239,18 @@ IoBufferExtendedCore::~IoBufferExtendedCore()
 
 			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)
+			{
+				ZEN_WARN("Error reported on file handle {}, get path for IoBufferExtendedCore destructor, reason '{}'",
+						 m_FileHandle,
+						 Ec.message());
+			}
+			else
+			{
+				unlink(FilePath.c_str());
+			}
 #endif
 		}
 #if ZEN_PLATFORM_WINDOWS
diff --git a/src/zencore/thread.cpp b/src/zencore/thread.cpp
index 394197b8e..ca3759fbf 100644
--- a/src/zencore/thread.cpp
+++ b/src/zencore/thread.cpp
@@ -348,8 +348,16 @@ NamedEvent::Close()
 
 	if (flock(Fd, LOCK_EX | LOCK_NB) == 0)
 	{
-		std::filesystem::path Name = PathFromHandle((void*)(intptr_t(Fd)));
-		unlink(Name.c_str());
+		std::error_code		  Ec;
+		std::filesystem::path Name = PathFromHandle((void*)(intptr_t(Fd)), Ec);
+		if (Ec)
+		{
+			ZEN_WARN("Error reported on get file path from handle {} for named event unlink operation, reason '{}'", Fd, Ec.message());
+		}
+		else
+		{
+			unlink(Name.c_str());
+		}
 
 		flock(Fd, LOCK_UN | LOCK_NB);
 		close(Fd);
diff --git a/src/zenhttp/httpclient.cpp b/src/zenhttp/httpclient.cpp
index 7ac0028cc..0d12cf815 100644
--- a/src/zenhttp/httpclient.cpp
+++ b/src/zenhttp/httpclient.cpp
@@ -367,8 +367,18 @@ public:
 				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());
+				std::error_code		  Ec;
+				std::filesystem::path FilePath = zen::PathFromHandle(m_FileHandle, Ec);
+				if (Ec)
+				{
+					ZEN_WARN("Error reported on get file path from handle {} for temp payload unlink operation, reason '{}'",
+							 m_FileHandle,
+							 Ec.message());
+				}
+				else
+				{
+					unlink(FilePath.c_str());
+				}
 				int	 Fd		 = int(uintptr_t(m_FileHandle));
 				bool Success = (close(Fd) == 0);
 #endif
diff --git a/src/zenstore/filecas.cpp b/src/zenstore/filecas.cpp
index 57b42beb2..6d5bcff96 100644
--- a/src/zenstore/filecas.cpp
+++ b/src/zenstore/filecas.cpp
@@ -459,41 +459,30 @@ FileCasStrategy::InsertChunk(IoBuffer Chunk, const IoHash& ChunkHash, CasStore::
 				 ChunkHash);
 
 #elif ZEN_PLATFORM_LINUX || ZEN_PLATFORM_MAC
-
-		std::filesystem::path SourcePath = PathFromHandle(FileRef.FileHandle);
-		std::filesystem::path DestPath	 = Name.ShardedPath.c_str();
-		int					  Ret		 = link(SourcePath.c_str(), DestPath.c_str());
-		if (Ret < 0 && zen::GetLastError() == ENOENT)
+		std::error_code		  Ec;
+		std::filesystem::path SourcePath = PathFromHandle(FileRef.FileHandle, Ec);
+		if (Ec)
 		{
-			// Destination directory doesn't exist. Create it any try again.
-			CreateDirectories(DestPath.parent_path().c_str());
-			Ret = link(SourcePath.c_str(), DestPath.c_str());
+			ZEN_WARN("link of CAS payload file {} failed ('{}'), falling back to regular write for insert of {}",
+					 FileRef.FileHandle,
+					 Ec.message(),
+					 ChunkHash);
 		}
-		if (Ret == 0)
+		else
 		{
-			m_CasLog.Append({.Key = ChunkHash, .Size = Chunk.Size()});
-			Chunk.SetDeleteOnClose(false);
-
-			HashLock.ReleaseNow();
-			bool IsNew = false;
+			std::filesystem::path DestPath = Name.ShardedPath.c_str();
+			int					  Ret	   = link(SourcePath.c_str(), DestPath.c_str());
+			if (Ret < 0 && zen::GetLastError() == ENOENT)
 			{
-				RwLock::ExclusiveLockScope __(m_Lock);
-				IsNew = m_Index.insert({ChunkHash, IndexEntry{.Size = Chunk.Size()}}).second;
+				// Destination directory doesn't exist. Create it any try again.
+				CreateDirectories(DestPath.parent_path().c_str());
+				Ret = link(SourcePath.c_str(), DestPath.c_str());
 			}
-			if (IsNew)
+			if (Ret == 0)
 			{
-				m_TotalSize.fetch_add(Chunk.Size(), std::memory_order::relaxed);
-			}
-			return CasStore::InsertResult{.New = IsNew};
-		}
-		else
-		{
-			int LinkError = zen::GetLastError();
+				m_CasLog.Append({.Key = ChunkHash, .Size = Chunk.Size()});
+				Chunk.SetDeleteOnClose(false);
 
-			// It is possible that someone beat us to it in linking the file. In that
-			// case a "file exists" error is okay. All others are not.
-			if (LinkError == EEXIST)
-			{
 				HashLock.ReleaseNow();
 				bool IsNew = false;
 				{
@@ -506,10 +495,31 @@ FileCasStrategy::InsertChunk(IoBuffer Chunk, const IoHash& ChunkHash, CasStore::
 				}
 				return CasStore::InsertResult{.New = IsNew};
 			}
+			else
+			{
+				int LinkError = zen::GetLastError();
 
-			ZEN_WARN("link of CAS payload file failed ('{}'), falling back to regular write for insert of {}",
-					 GetSystemErrorAsString(LinkError),
-					 ChunkHash);
+				// It is possible that someone beat us to it in linking the file. In that
+				// case a "file exists" error is okay. All others are not.
+				if (LinkError == EEXIST)
+				{
+					HashLock.ReleaseNow();
+					bool IsNew = false;
+					{
+						RwLock::ExclusiveLockScope __(m_Lock);
+						IsNew = m_Index.insert({ChunkHash, IndexEntry{.Size = Chunk.Size()}}).second;
+					}
+					if (IsNew)
+					{
+						m_TotalSize.fetch_add(Chunk.Size(), std::memory_order::relaxed);
+					}
+					return CasStore::InsertResult{.New = IsNew};
+				}
+
+				ZEN_WARN("link of CAS payload file failed ('{}'), falling back to regular write for insert of {}",
+						 GetSystemErrorAsString(LinkError),
+						 ChunkHash);
+			}
 		}
 #endif	// ZEN_PLATFORM_*
 	}
diff --git a/src/zenutil/basicfile.cpp b/src/zenutil/basicfile.cpp
index 575d153b2..73f27b587 100644
--- a/src/zenutil/basicfile.cpp
+++ b/src/zenutil/basicfile.cpp
@@ -858,13 +858,16 @@ TEST_CASE("TemporaryFile")
 
 	SUBCASE("DeleteOnClose")
 	{
-		TemporaryFile	TmpFile;
-		std::error_code Ec;
-		TmpFile.CreateTemporary(std::filesystem::current_path(), Ec);
-		CHECK(!Ec);
-		CHECK(std::filesystem::exists(TmpFile.GetPath()));
-		TmpFile.Close();
-		CHECK(std::filesystem::exists(TmpFile.GetPath()) == false);
+		std::filesystem::path Path;
+		{
+			TemporaryFile	TmpFile;
+			std::error_code Ec;
+			TmpFile.CreateTemporary(std::filesystem::current_path(), Ec);
+			CHECK(!Ec);
+			Path = TmpFile.GetPath();
+			CHECK(std::filesystem::exists(Path));
+		}
+		CHECK(std::filesystem::exists(Path) == false);
 	}
 
 	SUBCASE("MoveIntoPlace")
diff --git a/src/zenutil/include/zenutil/basicfile.h b/src/zenutil/include/zenutil/basicfile.h
index 674457196..03c5605df 100644
--- a/src/zenutil/include/zenutil/basicfile.h
+++ b/src/zenutil/include/zenutil/basicfile.h
@@ -96,7 +96,6 @@ public:
 	TemporaryFile(const TemporaryFile&) = delete;
 	TemporaryFile& operator=(const TemporaryFile&) = delete;
 
-	void						 Close();
 	void						 CreateTemporary(std::filesystem::path TempDirName, std::error_code& Ec);
 	void						 MoveTemporaryIntoPlace(std::filesystem::path FinalFileName, std::error_code& Ec);
 	const std::filesystem::path& GetPath() const { return m_TempPath; }
@@ -105,6 +104,7 @@ public:
 	static void SafeWriteFile(const std::filesystem::path& Path, MemoryView Data, std::error_code& OutEc);
 
 private:
+	void				  Close();
 	std::filesystem::path m_TempPath;
 
 	using BasicFile::Open;
diff --git a/src/zenutil/packageformat.cpp b/src/zenutil/packageformat.cpp
index 8087b7564..579e0d13c 100644
--- a/src/zenutil/packageformat.cpp
+++ b/src/zenutil/packageformat.cpp
@@ -126,7 +126,14 @@ IsLocalRef(tsl::robin_map<void*, std::string>& FileNameMap,
 		if (UseFilePath)
 		{
 			ExtendablePathBuilder<256> LocalRefFile;
-			LocalRefFile.Append(std::filesystem::absolute(PathFromHandle(Ref.FileHandle)));
+			std::error_code			   Ec;
+			std::filesystem::path	   FilePath = PathFromHandle(Ref.FileHandle, Ec);
+			if (Ec)
+			{
+				ZEN_WARN("Failed to get path for file handle {} in IsLocalRef check, reason '{}'", Ref.FileHandle, Ec.message());
+				return false;
+			}
+			LocalRefFile.Append(std::filesystem::absolute(FilePath));
 			Path8 = LocalRefFile.ToUtf8();
 		}
 		FileNameMap.insert_or_assign(Ref.FileHandle, Path8);