From 3af2148ea43bed383e16aca16db93464fe68d1ba Mon Sep 17 00:00:00 2001 From: James Clarke Date: Mon, 15 Feb 2021 10:46:34 -0800 Subject: [PATCH 1/6] Fix fml::GetFullHandlePath for WINUWP target --- fml/platform/win/file_win.cc | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/fml/platform/win/file_win.cc b/fml/platform/win/file_win.cc index 2815584ae4b82..096f998d26d2f 100644 --- a/fml/platform/win/file_win.cc +++ b/fml/platform/win/file_win.cc @@ -7,6 +7,10 @@ #include #include #include +#include +#include +#include +#include #include #include @@ -20,7 +24,24 @@ namespace fml { +#ifdef WINUWP +static std::map file_map; +#endif + static std::string GetFullHandlePath(const fml::UniqueFD& handle) { + // Although the documentation claims that GetFinalPathNameByHandle is + // supported for UWP apps, turns out it does not work correctly hence the need + // to workaround it by maintaining a map of file handles to absolute paths + // populated by fml::OpenDirectory. +#ifdef WINUWP + if (file_map.count(handle.get()) > 0) { + auto found = file_map[handle.get()]; + return WideStringToString(found); + } else { + return std::string(); + } + +#else wchar_t buffer[MAX_PATH] = {0}; const DWORD buffer_size = ::GetFinalPathNameByHandle( handle.get(), buffer, MAX_PATH, FILE_NAME_NORMALIZED); @@ -30,6 +51,7 @@ static std::string GetFullHandlePath(const fml::UniqueFD& handle) { return {}; } return WideStringToString({buffer, buffer_size}); +#endif } static std::string GetAbsolutePath(const fml::UniqueFD& base_directory, @@ -223,6 +245,10 @@ fml::UniqueFD OpenDirectory(const char* path, return {}; } +#ifdef WINUWP + file_map[handle] = file_name; +#endif + return fml::UniqueFD{handle}; } From 4759397ecee03d7bdf4fd559f5060d01e23fefbe Mon Sep 17 00:00:00 2001 From: James Clarke Date: Mon, 15 Feb 2021 21:14:35 -0800 Subject: [PATCH 2/6] CR feedback --- fml/platform/win/file_win.cc | 40 +++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/fml/platform/win/file_win.cc b/fml/platform/win/file_win.cc index 096f998d26d2f..24c55fd31953c 100644 --- a/fml/platform/win/file_win.cc +++ b/fml/platform/win/file_win.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -25,7 +26,12 @@ namespace fml { #ifdef WINUWP -static std::map file_map; +struct DirCacheEntry { + std::wstring filename; + FILE_ID_128 id; +}; +std::mutex file_map_mutex; +static std::map file_map; #endif static std::string GetFullHandlePath(const fml::UniqueFD& handle) { @@ -34,9 +40,26 @@ static std::string GetFullHandlePath(const fml::UniqueFD& handle) { // to workaround it by maintaining a map of file handles to absolute paths // populated by fml::OpenDirectory. #ifdef WINUWP + const std::lock_guard lock(file_map_mutex); + if (file_map.count(handle.get()) > 0) { + FILE_ID_INFO info; + + auto result = GetFileInformationByHandleEx( + handle.get(), FILE_INFO_BY_HANDLE_CLASS::FileIdInfo, &info, + sizeof(FILE_ID_INFO)); + auto found = file_map[handle.get()]; - return WideStringToString(found); + + // The handle hasn't been reused if the file identifier is the same as when + // it was cached + if (memcmp(found.id.Identifier, info.FileId.Identifier, + sizeof(FILE_ID_INFO))) { + return WideStringToString(found.filename); + } else { + // The file handle was reused and cache entry is invalid. + return std::string(); + } } else { return std::string(); } @@ -246,7 +269,18 @@ fml::UniqueFD OpenDirectory(const char* path, } #ifdef WINUWP - file_map[handle] = file_name; + const std::lock_guard lock(file_map_mutex); + + FILE_ID_INFO info; + + auto result = GetFileInformationByHandleEx( + handle, FILE_INFO_BY_HANDLE_CLASS::FileIdInfo, &info, + sizeof(FILE_ID_INFO)); + + auto fc = DirCacheEntry{}; + fc.filename = file_name; + fc.id = info.FileId; + file_map[handle] = fc; #endif return fml::UniqueFD{handle}; From d0bbdf8b148ea391286bea33547073e8f71640ad Mon Sep 17 00:00:00 2001 From: James Clarke Date: Thu, 18 Feb 2021 20:38:47 -0800 Subject: [PATCH 3/6] CR feedback --- fml/platform/win/file_win.cc | 54 +++++++++++++++--------------------- fml/unique_fd.cc | 2 +- fml/unique_fd.h | 43 +++++++++++++++++++++++++++- 3 files changed, 65 insertions(+), 34 deletions(-) diff --git a/fml/platform/win/file_win.cc b/fml/platform/win/file_win.cc index 24c55fd31953c..85a71e89639a8 100644 --- a/fml/platform/win/file_win.cc +++ b/fml/platform/win/file_win.cc @@ -9,13 +9,12 @@ #include #include #include -#include -#include #include #include #include #include +#include #include #include "flutter/fml/build_config.h" @@ -25,45 +24,33 @@ namespace fml { -#ifdef WINUWP -struct DirCacheEntry { - std::wstring filename; - FILE_ID_128 id; -}; -std::mutex file_map_mutex; -static std::map file_map; -#endif - static std::string GetFullHandlePath(const fml::UniqueFD& handle) { // Although the documentation claims that GetFinalPathNameByHandle is - // supported for UWP apps, turns out it does not work correctly hence the need + // supported for UWP apps, turns out it returns ACCESS_DENIED in this case hence the need // to workaround it by maintaining a map of file handles to absolute paths // populated by fml::OpenDirectory. #ifdef WINUWP - const std::lock_guard lock(file_map_mutex); - - if (file_map.count(handle.get()) > 0) { + std::optional found = + fml::internal::os_win::UniqueFDTraits::GetCacheEntry(handle.get()); + + if (found) { FILE_ID_INFO info; - auto result = GetFileInformationByHandleEx( + BOOL result = GetFileInformationByHandleEx( handle.get(), FILE_INFO_BY_HANDLE_CLASS::FileIdInfo, &info, sizeof(FILE_ID_INFO)); - auto found = file_map[handle.get()]; - - // The handle hasn't been reused if the file identifier is the same as when + // Assuming it was possible to retrieve fileinfo, compare the id field. The handle hasn't been reused if the file identifier is the same as when // it was cached - if (memcmp(found.id.Identifier, info.FileId.Identifier, + if (result && memcmp(found.value().id.Identifier, info.FileId.Identifier, sizeof(FILE_ID_INFO))) { - return WideStringToString(found.filename); + return WideStringToString(found.value().filename); } else { - // The file handle was reused and cache entry is invalid. - return std::string(); + fml::internal::os_win::UniqueFDTraits::RemoveCacheEntry(handle.get()); } - } else { - return std::string(); } + return std::string(); #else wchar_t buffer[MAX_PATH] = {0}; const DWORD buffer_size = ::GetFinalPathNameByHandle( @@ -269,18 +256,21 @@ fml::UniqueFD OpenDirectory(const char* path, } #ifdef WINUWP - const std::lock_guard lock(file_map_mutex); - FILE_ID_INFO info; - auto result = GetFileInformationByHandleEx( + BOOL result = GetFileInformationByHandleEx( handle, FILE_INFO_BY_HANDLE_CLASS::FileIdInfo, &info, sizeof(FILE_ID_INFO)); - auto fc = DirCacheEntry{}; - fc.filename = file_name; - fc.id = info.FileId; - file_map[handle] = fc; + // Only cache if it is possible to get valid a fileinformation to extract the fileid to ensure correct handle versioning. + if (result) { + fml::internal::os_win::DirCacheEntry fc { + file_name, + info.FileId + }; + + fml::internal::os_win::UniqueFDTraits::StoreCacheEntry(handle, fc); + } #endif return fml::UniqueFD{handle}; diff --git a/fml/unique_fd.cc b/fml/unique_fd.cc index 2da6d938f9875..5a16c72d21663 100644 --- a/fml/unique_fd.cc +++ b/fml/unique_fd.cc @@ -13,7 +13,7 @@ namespace internal { namespace os_win { -void UniqueFDTraits::Free(HANDLE fd) { +void UniqueFDTraits::Free_Handle(HANDLE fd) { CloseHandle(fd); } diff --git a/fml/unique_fd.h b/fml/unique_fd.h index 5b74073f426a4..5d4be0f35e0ab 100644 --- a/fml/unique_fd.h +++ b/fml/unique_fd.h @@ -10,6 +10,9 @@ #if OS_WIN #include +#include +#include +#include #else // OS_WIN #include #include @@ -22,10 +25,48 @@ namespace internal { namespace os_win { +struct DirCacheEntry { + std::wstring filename; + FILE_ID_128 id; +}; + +// The order of these is important. Must come before UniqueFDTraits struct +// else linker error. Embedding in struct also causes linker error. + struct UniqueFDTraits { + inline static std::mutex file_map_mutex; + inline static std::map file_map; + static HANDLE InvalidValue() { return INVALID_HANDLE_VALUE; } static bool IsValid(HANDLE value) { return value != InvalidValue(); } - static void Free(HANDLE fd); + static void Free_Handle(HANDLE fd); + + static void Free(HANDLE fd) { + RemoveCacheEntry(fd); + + UniqueFDTraits::Free_Handle(fd); + } + + static void RemoveCacheEntry(HANDLE fd) { + const std::lock_guard lock(file_map_mutex); + + if (file_map.count(fd) > 0) { + file_map.erase(fd); + } + } + + static void StoreCacheEntry(HANDLE fd, DirCacheEntry state) { + const std::lock_guard lock(file_map_mutex); + file_map[fd] = state; + } + + static std::optional GetCacheEntry(HANDLE fd) { + const std::lock_guard lock(file_map_mutex); + if (file_map.count(fd) > 0) { + return file_map[fd]; + } + return {}; + } }; } // namespace os_win From c53a1ca6c891f9e1dd1958797c26382a8128dbb7 Mon Sep 17 00:00:00 2001 From: James Clarke Date: Thu, 18 Feb 2021 20:43:49 -0800 Subject: [PATCH 4/6] Format --- fml/platform/win/file_win.cc | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/fml/platform/win/file_win.cc b/fml/platform/win/file_win.cc index 85a71e89639a8..7b0e785f40c0e 100644 --- a/fml/platform/win/file_win.cc +++ b/fml/platform/win/file_win.cc @@ -26,13 +26,13 @@ namespace fml { static std::string GetFullHandlePath(const fml::UniqueFD& handle) { // Although the documentation claims that GetFinalPathNameByHandle is - // supported for UWP apps, turns out it returns ACCESS_DENIED in this case hence the need - // to workaround it by maintaining a map of file handles to absolute paths - // populated by fml::OpenDirectory. + // supported for UWP apps, turns out it returns ACCESS_DENIED in this case + // hence the need to workaround it by maintaining a map of file handles to + // absolute paths populated by fml::OpenDirectory. #ifdef WINUWP std::optional found = fml::internal::os_win::UniqueFDTraits::GetCacheEntry(handle.get()); - + if (found) { FILE_ID_INFO info; @@ -40,17 +40,18 @@ static std::string GetFullHandlePath(const fml::UniqueFD& handle) { handle.get(), FILE_INFO_BY_HANDLE_CLASS::FileIdInfo, &info, sizeof(FILE_ID_INFO)); - // Assuming it was possible to retrieve fileinfo, compare the id field. The handle hasn't been reused if the file identifier is the same as when - // it was cached + // Assuming it was possible to retrieve fileinfo, compare the id field. The + // handle hasn't been reused if the file identifier is the same as when it + // was cached if (result && memcmp(found.value().id.Identifier, info.FileId.Identifier, - sizeof(FILE_ID_INFO))) { + sizeof(FILE_ID_INFO))) { return WideStringToString(found.value().filename); } else { fml::internal::os_win::UniqueFDTraits::RemoveCacheEntry(handle.get()); } } - return std::string(); + return std::string(); #else wchar_t buffer[MAX_PATH] = {0}; const DWORD buffer_size = ::GetFinalPathNameByHandle( @@ -262,12 +263,10 @@ fml::UniqueFD OpenDirectory(const char* path, handle, FILE_INFO_BY_HANDLE_CLASS::FileIdInfo, &info, sizeof(FILE_ID_INFO)); - // Only cache if it is possible to get valid a fileinformation to extract the fileid to ensure correct handle versioning. + // Only cache if it is possible to get valid a fileinformation to extract the + // fileid to ensure correct handle versioning. if (result) { - fml::internal::os_win::DirCacheEntry fc { - file_name, - info.FileId - }; + fml::internal::os_win::DirCacheEntry fc{file_name, info.FileId}; fml::internal::os_win::UniqueFDTraits::StoreCacheEntry(handle, fc); } From c0aceed4e90f7731c87d4bc5e5f4bc014a546831 Mon Sep 17 00:00:00 2001 From: James Clarke Date: Fri, 19 Feb 2021 21:08:28 -0800 Subject: [PATCH 5/6] CR feedback --- fml/unique_fd.h | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/fml/unique_fd.h b/fml/unique_fd.h index 5d4be0f35e0ab..80db7d32326e6 100644 --- a/fml/unique_fd.h +++ b/fml/unique_fd.h @@ -50,9 +50,7 @@ struct UniqueFDTraits { static void RemoveCacheEntry(HANDLE fd) { const std::lock_guard lock(file_map_mutex); - if (file_map.count(fd) > 0) { - file_map.erase(fd); - } + file_map.erase(fd); } static void StoreCacheEntry(HANDLE fd, DirCacheEntry state) { @@ -62,10 +60,10 @@ struct UniqueFDTraits { static std::optional GetCacheEntry(HANDLE fd) { const std::lock_guard lock(file_map_mutex); - if (file_map.count(fd) > 0) { - return file_map[fd]; - } - return {}; + auto found = file_map.find(fd); + return found == file_map.end() + ? std::nullopt + : std::optional{found->second}; } }; From 641c0a78d30f6f76713c49423f30e9f545755db1 Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Tue, 16 Mar 2021 20:25:54 -0700 Subject: [PATCH 6/6] Define mutex, file_map The statics were declared in the header but never defined in a translation unit. --- fml/unique_fd.cc | 3 +++ fml/unique_fd.h | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/fml/unique_fd.cc b/fml/unique_fd.cc index 5a16c72d21663..5721b0f82958d 100644 --- a/fml/unique_fd.cc +++ b/fml/unique_fd.cc @@ -13,6 +13,9 @@ namespace internal { namespace os_win { +std::mutex UniqueFDTraits::file_map_mutex; +std::map UniqueFDTraits::file_map; + void UniqueFDTraits::Free_Handle(HANDLE fd) { CloseHandle(fd); } diff --git a/fml/unique_fd.h b/fml/unique_fd.h index 80db7d32326e6..c4b79dc3496e4 100644 --- a/fml/unique_fd.h +++ b/fml/unique_fd.h @@ -34,8 +34,8 @@ struct DirCacheEntry { // else linker error. Embedding in struct also causes linker error. struct UniqueFDTraits { - inline static std::mutex file_map_mutex; - inline static std::map file_map; + static std::mutex file_map_mutex; + static std::map file_map; static HANDLE InvalidValue() { return INVALID_HANDLE_VALUE; } static bool IsValid(HANDLE value) { return value != InvalidValue(); }