From ce72e32cfc640af8029194578fe1ecf4766c8d49 Mon Sep 17 00:00:00 2001 From: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> Date: Tue, 14 Sep 2021 19:21:29 -0500 Subject: [PATCH] Fixed issue where two Paths could compare equal to each other, but hash differently (#4126) * Fixed issue where two Paths could compare equal to each other, but hash differently This issue is caused by the Path comparison logic using the path separator of the left path in a comparison of two paths(left and right) to determine whether the PathComparison is case-sensitive or not. The logic has been updated to only perform a non-case-sensitive path comparison if both paths are using the WindowsPathSeperator of `\` Also fixed issue with the Hashing algorihtm of the Path class to always hash the root directory as if it is `/`. This allows a path of "C:\foo" and "C:/foo" to hash to the equivalent value. Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> * MS Build Tools 14.29 workaround around suppressing warnings using the external header feature Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> --- Code/Framework/AzCore/AzCore/IO/Path/Path.h | 4 +- Code/Framework/AzCore/AzCore/IO/Path/Path.inl | 55 +++++--------- .../AzCore/AzCore/IO/Path/PathParser.inl | 56 ++++++++++++-- .../AzCore/Tests/IO/Path/PathTests.cpp | 76 +++++++++++++++++++ 4 files changed, 148 insertions(+), 43 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/IO/Path/Path.h b/Code/Framework/AzCore/AzCore/IO/Path/Path.h index c0c4b1c974..24f26daa51 100644 --- a/Code/Framework/AzCore/AzCore/IO/Path/Path.h +++ b/Code/Framework/AzCore/AzCore/IO/Path/Path.h @@ -273,7 +273,7 @@ namespace AZ::IO // If the path input = 'D:bar', then the new PathIterable parts = [D:, 'bar' ] static constexpr void AppendNormalPathParts(PathIterable& pathIterableResult, const AZ::IO::PathView& path) noexcept; - constexpr int compare_string_view(AZStd::string_view other) const; + constexpr int ComparePathView(const PathView& other) const; constexpr AZStd::string_view root_name_view() const; constexpr AZStd::string_view root_directory_view() const; constexpr AZStd::string_view root_path_raw_view() const; @@ -480,6 +480,8 @@ namespace AZ::IO // compare //! Performs a compare of each of the path parts for equivalence //! Each part of the path is compare using string comparison + //! If both *this path and the input path uses the WindowsPathSeparator + //! then a non-case sensitive compare is performed //! Ex: Comparing "test/foo" against "test/fop" returns -1; //! Path separators of the contained path string aren't compared //! Ex. Comparing "C:/test\foo" against C:\test/foo" returns 0; diff --git a/Code/Framework/AzCore/AzCore/IO/Path/Path.inl b/Code/Framework/AzCore/AzCore/IO/Path/Path.inl index 40cbf6f46b..0147ad3356 100644 --- a/Code/Framework/AzCore/AzCore/IO/Path/Path.inl +++ b/Code/Framework/AzCore/AzCore/IO/Path/Path.inl @@ -224,15 +224,15 @@ namespace AZ::IO // compare constexpr int PathView::Compare(const PathView& other) const noexcept { - return compare_string_view(other.m_path); + return ComparePathView(other); } constexpr int PathView::Compare(AZStd::string_view pathView) const noexcept { - return compare_string_view(pathView); + return ComparePathView(PathView(pathView, m_preferred_separator)); } constexpr int PathView::Compare(const value_type* path) const noexcept { - return compare_string_view(path); + return ComparePathView(PathView(path, m_preferred_separator)); } constexpr AZStd::fixed_string PathView::FixedMaxPathString() const noexcept @@ -398,10 +398,10 @@ namespace AZ::IO return true; } - constexpr int PathView::compare_string_view(AZStd::string_view pathView) const + constexpr int PathView::ComparePathView(const PathView& other) const { auto lhsPathParser = parser::PathParser::CreateBegin(m_path, m_preferred_separator); - auto rhsPathParser = parser::PathParser::CreateBegin(pathView, m_preferred_separator); + auto rhsPathParser = parser::PathParser::CreateBegin(other.m_path, other.m_preferred_separator); if (int res = CompareRootName(&lhsPathParser, &rhsPathParser); res != 0) { @@ -476,6 +476,8 @@ namespace AZ::IO template constexpr void PathView::MakeRelativeTo(PathResultType& pathResult, const AZ::IO::PathView& path, const AZ::IO::PathView& base) { + const bool exactCaseCompare = path.m_preferred_separator == PosixPathSeparator + || base.m_preferred_separator == PosixPathSeparator; { // perform root-name/root-directory mismatch checks auto pathParser = parser::PathParser::CreateBegin(path.m_path, path.m_preferred_separator); @@ -487,7 +489,7 @@ namespace AZ::IO }; if (pathParser.InRootName() && pathParserBase.InRootName()) { - if (int res = Internal::ComparePathSegment(*pathParser, *pathParserBase, pathParser.m_preferred_separator); + if (int res = Internal::ComparePathSegment(*pathParser, *pathParserBase, exactCaseCompare); res != 0) { pathResult.m_path = AZStd::string_view{}; @@ -519,7 +521,7 @@ namespace AZ::IO auto pathParser = parser::PathParser::CreateBegin(path.m_path, path.m_preferred_separator); auto pathParserBase = parser::PathParser::CreateBegin(base.m_path, base.m_preferred_separator); while (pathParser && pathParserBase && pathParser.m_parser_state == pathParserBase.m_parser_state && - Internal::ComparePathSegment(*pathParser, *pathParserBase, pathParser.m_preferred_separator) == 0) + Internal::ComparePathSegment(*pathParser, *pathParserBase, exactCaseCompare) == 0) { ++pathParser; ++pathParserBase; @@ -1080,25 +1082,25 @@ namespace AZ::IO template constexpr int BasicPath::Compare(const PathView& other) const noexcept { - return static_cast(*this).compare_string_view(other.m_path); + return static_cast(*this).ComparePathView(other); } template constexpr int BasicPath::Compare(const string_type& pathString) const { - return static_cast(*this).compare_string_view(pathString); + return static_cast(*this).ComparePathView(PathView(pathString, m_preferred_separator)); } template constexpr int BasicPath::Compare(AZStd::string_view pathView) const noexcept { - return static_cast(*this).compare_string_view(pathView); + return static_cast(*this).ComparePathView(pathView); } template constexpr int BasicPath::Compare(const value_type* pathString) const noexcept { - return static_cast(*this).compare_string_view(pathString); + return static_cast(*this).ComparePathView(pathString); } // decomposition @@ -1330,10 +1332,12 @@ namespace AZ::IO // PathView::LexicallyRelative is not being used as it returns a FixedMaxPath // which has a limitation that it requires the relative path to fit within // an AZ::IO::MaxPathLength buffer - auto ComparePathPart = [pathSeparator = m_preferred_separator]( + const bool exactCaseCompare = m_preferred_separator == PosixPathSeparator + || base.m_preferred_separator == PosixPathSeparator; + auto ComparePathPart = [exactCaseCompare]( const PathIterable::PartKindPair& left, const PathIterable::PartKindPair& right) -> bool { - return Internal::ComparePathSegment(left.first, right.first, pathSeparator) == 0; + return Internal::ComparePathSegment(left.first, right.first, exactCaseCompare) == 0; }; const PathIterable thisPathParts = GetNormalPathParts(*this); @@ -1471,37 +1475,16 @@ namespace AZStd template <> struct hash { - /// Path is using FNV-1a algorithm 64 bit version. - static size_t hash_path(AZStd::string_view pathSegment, const char pathSeparator) - { - size_t hash = 14695981039346656037ULL; - constexpr size_t fnvPrime = 1099511628211ULL; - - for (const char first : pathSegment) - { - hash ^= static_cast((pathSeparator == AZ::IO::PosixPathSeparator) - ? first : tolower(first)); - hash *= fnvPrime; - } - return hash; - } - size_t operator()(const AZ::IO::PathView& pathToHash) noexcept { auto pathParser = AZ::IO::parser::PathParser::CreateBegin(pathToHash.Native(), pathToHash.m_preferred_separator); - size_t hash_value = 0; - while (pathParser) - { - AZStd::hash_combine(hash_value, hash_path(*pathParser, pathToHash.m_preferred_separator)); - ++pathParser; - } - return hash_value; + return AZ::IO::parser::HashPath(pathParser); } }; template struct hash> { - const size_t operator()(const AZ::IO::BasicPath& pathToHash) noexcept + size_t operator()(const AZ::IO::BasicPath& pathToHash) noexcept { return AZStd::hash{}(pathToHash); } diff --git a/Code/Framework/AzCore/AzCore/IO/Path/PathParser.inl b/Code/Framework/AzCore/AzCore/IO/Path/PathParser.inl index 3ab2c4376c..b19c518ff9 100644 --- a/Code/Framework/AzCore/AzCore/IO/Path/PathParser.inl +++ b/Code/Framework/AzCore/AzCore/IO/Path/PathParser.inl @@ -183,13 +183,12 @@ namespace AZ::IO::Internal return IsAbsolute(pathView.begin(), pathView.end(), preferredSeparator); } - // Compares path segments using either Posix or Windows path rules based on the path separator in use - // Posix paths perform a case-sensitive comparison, while Windows paths perform a case-insensitive comparison - static int ComparePathSegment(AZStd::string_view left, AZStd::string_view right, char pathSeparator) + // Compares path segments using either Posix or Windows path rules based on the exactCaseCompare option + static int ComparePathSegment(AZStd::string_view left, AZStd::string_view right, bool exactCaseCompare) { const size_t maxCharsToCompare = (AZStd::min)(left.size(), right.size()); - int charCompareResult = pathSeparator == PosixPathSeparator + int charCompareResult = exactCaseCompare ? maxCharsToCompare ? strncmp(left.data(), right.data(), maxCharsToCompare) : 0 : maxCharsToCompare ? azstrnicmp(left.data(), right.data(), maxCharsToCompare) : 0; return charCompareResult == 0 @@ -594,7 +593,10 @@ namespace AZ::IO::parser { return pathParser->InRootName() ? **pathParser : ""; }; - int res = Internal::ComparePathSegment(GetRootName(lhsPathParser), GetRootName(rhsPathParser), lhsPathParser->m_preferred_separator); + + const bool exactCaseCompare = lhsPathParser->m_preferred_separator == PosixPathSeparator + || rhsPathParser->m_preferred_separator == PosixPathSeparator; + int res = Internal::ComparePathSegment(GetRootName(lhsPathParser), GetRootName(rhsPathParser), exactCaseCompare); ConsumeRootName(lhsPathParser); ConsumeRootName(rhsPathParser); return res; @@ -621,9 +623,11 @@ namespace AZ::IO::parser auto& lhsPathParser = *lhsPathParserPtr; auto& rhsPathParser = *rhsPathParserPtr; + const bool exactCaseCompare = lhsPathParser.m_preferred_separator == PosixPathSeparator + || rhsPathParser.m_preferred_separator == PosixPathSeparator; while (lhsPathParser && rhsPathParser) { - if (int res = Internal::ComparePathSegment(*lhsPathParser, *rhsPathParser, lhsPathParser.m_preferred_separator); + if (int res = Internal::ComparePathSegment(*lhsPathParser, *rhsPathParser, exactCaseCompare); res != 0) { return res; @@ -646,6 +650,46 @@ namespace AZ::IO::parser return 0; } + //path.hash + /// Path is using FNV-1a algorithm 64 bit version. + inline size_t HashSegment(AZStd::string_view pathSegment, bool hashExactPath) + { + size_t hash = 14695981039346656037ULL; + constexpr size_t fnvPrime = 1099511628211ULL; + + for (const char first : pathSegment) + { + hash ^= static_cast(hashExactPath ? first : tolower(first)); + hash *= fnvPrime; + } + return hash; + } + constexpr size_t HashPath(PathParser& pathParser) + { + size_t hash_value = 0; + const bool hashExactPath = pathParser.m_preferred_separator == AZ::IO::PosixPathSeparator; + while (pathParser) + { + switch (pathParser.m_parser_state) + { + case PS_InRootName: + case PS_InFilenames: + AZStd::hash_combine(hash_value, HashSegment(*pathParser, hashExactPath)); + break; + case PS_InRootDir: + // Only hash the PosixPathSeparator when a root directory is seen + // This makes the hash consistent for root directories path of C:\ and C:/ + AZStd::hash_combine(hash_value, HashSegment("/", hashExactPath)); + break; + default: + // The BeforeBegin and AtEnd states contain no segments to hash + break; + } + ++pathParser; + } + return hash_value; + } + constexpr int DetermineLexicalElementCount(PathParser pathParser) { int count = 0; diff --git a/Code/Framework/AzCore/Tests/IO/Path/PathTests.cpp b/Code/Framework/AzCore/Tests/IO/Path/PathTests.cpp index dbdb4fee78..cf212aa34c 100644 --- a/Code/Framework/AzCore/Tests/IO/Path/PathTests.cpp +++ b/Code/Framework/AzCore/Tests/IO/Path/PathTests.cpp @@ -213,6 +213,82 @@ namespace UnitTest AZStd::tuple(R"(foO/Bar)", "foo/bar") )); + + struct PathHashCompareParams + { + AZ::IO::PathView m_testPath{}; + ::testing::Matcher m_compareMatcher; + ::testing::Matcher m_hashMatcher; + }; + + class PathHashCompareFixture + : public ScopedAllocatorSetupFixture + , public ::testing::WithParamInterface + {}; + + // Verifies that two paths that compare equal has their hash value compare equal + TEST_P(PathHashCompareFixture, PathsWhichCompareEqual_HashesToSameValue_Succeeds) + { + auto&& [testPath1, compareMatcher, hashMatcher] = GetParam(); + + // Compare path using parameterized Matcher + EXPECT_THAT(testPath1, compareMatcher); + // Compare hash using parameterized Matcher + const size_t testPath1Hash = AZStd::hash{}(testPath1); +AZ_PUSH_DISABLE_WARNING(4296, "-Wunknown-warning-option") + EXPECT_THAT(testPath1Hash, hashMatcher); +AZ_POP_DISABLE_WARNING + } + + INSTANTIATE_TEST_CASE_P( + HashPathCompareValidation, + PathHashCompareFixture, + ::testing::Values( + PathHashCompareParams{ AZ::IO::PathView("C:/test/foo", AZ::IO::WindowsPathSeparator), + testing::Eq(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator)), + testing::Eq(AZStd::hash{}(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/foo", AZ::IO::WindowsPathSeparator), + testing::Eq(AZ::IO::PathView(R"(/test/FOO)", AZ::IO::WindowsPathSeparator)), + testing::Eq(AZStd::hash{}(AZ::IO::PathView(R"(/test/FOO)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("C:/test/foo", AZ::IO::WindowsPathSeparator), + testing::Eq(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator)), + testing::Eq(AZStd::hash{}(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("C:/test/foo", AZ::IO::PosixPathSeparator), + testing::Ne(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView(R"(C:\test\foo)", AZ::IO::WindowsPathSeparator), + testing::Ne(AZ::IO::PathView(R"(c:/test/foo)", AZ::IO::PosixPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(c:/test/foo)", AZ::IO::PosixPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::WindowsPathSeparator), + testing::Eq(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::WindowsPathSeparator)), + testing::Eq(AZStd::hash{}(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::PosixPathSeparator), + testing::Gt(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::WindowsPathSeparator)), + testing::Eq(AZStd::hash{}(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::WindowsPathSeparator), + testing::Gt(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::PosixPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::PosixPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/AOO", AZ::IO::PosixPathSeparator), + testing::Lt(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/AOO", AZ::IO::WindowsPathSeparator), + testing::Lt(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::PosixPathSeparator)), + testing::Eq(AZStd::hash{}(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::PosixPathSeparator))) }, + // Paths with different character values, comparison based on path separator + PathHashCompareParams{ AZ::IO::PathView("/test/BOO", AZ::IO::PosixPathSeparator), + testing::Le(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/BOO", AZ::IO::WindowsPathSeparator), + testing::Ge(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::WindowsPathSeparator), + testing::Le(AZ::IO::PathView(R"(/test/Boo)", AZ::IO::WindowsPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(/test/Boo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::PosixPathSeparator), + testing::Ge(AZ::IO::PathView(R"(/test/Boo)", AZ::IO::WindowsPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(/test/Boo)", AZ::IO::WindowsPathSeparator))) } + )); + class PathSingleParamFixture : public ScopedAllocatorSetupFixture , public ::testing::WithParamInterface>