Potential Memory Corruption in Release Build (#3559)

* Potential Memory Corruption in Release Build Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> * @lumberyard-employee-dm suggested code Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> * warnings as errors found in VS2022 Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> * simplifying some strucutres used and fixing a bug Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> * some unused fixes for VS2022 Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> * fix for other platforms Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com> * fixes check used in unit tests to be case-insensitive fixes memory leaks/invalid memory operations in AWSCore tests Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
2021-08-26 15:59:40 -07:00
parent 9f55212a19
commit 1be95a1e76
20 changed files with 93 additions and 108 deletions
@@ -419,9 +419,9 @@ namespace AZ::IO::ZipDir
        }

        // defining file attributes for opening files using constants to avoid the need to include windows headers
-        constexpr int FileFlagNoBufferinf = 0x20000000;
+        constexpr int FileFlagNoBuffering = 0x20000000;
        constexpr int FileAttributeNormal = 0x00000080;
-        if (m_unbufferedFile.Open(filename, AZ::IO::SystemFile::OpenMode::SF_OPEN_READ_ONLY, FileAttributeNormal | FileAttributeNormal))
+        if (m_unbufferedFile.Open(filename, AZ::IO::SystemFile::OpenMode::SF_OPEN_READ_ONLY, FileFlagNoBuffering | FileAttributeNormal))
        {
            m_nSize = aznumeric_cast<int64_t>(m_unbufferedFile.Length());
            return true;
@@ -14,6 +14,7 @@
 #include <AzCore/Casting/lossy_cast.h>
 #include <AzCore/std/functional.h>
 #include <AzCore/std/string/conversions.h>
+#include <AzCore/std/string/string_view.h>
 #include <AzCore/StringFunc/StringFunc.h>
 #include <cctype>

@@ -283,11 +284,18 @@ namespace AZ
        void LocalFileIO::CheckInvalidWrite([[maybe_unused]] const char* path)
        {
 #if defined(AZ_ENABLE_TRACING)
-            const char* assetsAlias = GetAlias("@assets@");
-            if (path && assetsAlias && AZ::IO::PathView(path).IsRelativeTo(assetsAlias))
+            const char* assetAliasPath = GetAlias("@assets@");
+            if (path && assetAliasPath)
            {
-                AZ_Error("FileIO", false, "You may not alter data inside the asset cache.  Please check the call stack and consider writing into the source asset folder instead.\n"
-                    "Attempted write location: %s", path);
+                AZStd::string assetsAlias(assetAliasPath);
+                AZStd::string pathString = path;
+                AZStd::to_lower(assetsAlias.begin(), assetsAlias.end());
+                AZStd::to_lower(pathString.begin(), pathString.end());
+                if (AZ::IO::PathView(pathString.c_str()).IsRelativeTo(assetsAlias.c_str()))
+                {
+                    AZ_Error("FileIO", false, "You may not alter data inside the asset cache.  Please check the call stack and consider writing into the source asset folder instead.\n"
+                        "Attempted write location: %s", path);
+                }
            }
 #endif
        }
@@ -543,28 +551,12 @@ namespace AZ
            char fullPath[AZ_MAX_PATH_LEN];
            ConvertToAbsolutePath(path, fullPath, AZ_MAX_PATH_LEN);

-            const auto it = AZStd::find_if(m_aliases.begin(), m_aliases.end(), [key](const AliasType& alias)
-            {
-                return alias.first.compare(key) == 0;
-            });
-
-            if (it != m_aliases.end())
-            {
-                it->second = fullPath;
-            }
-            else
-            {
-                m_aliases.emplace_back(key, fullPath);
-            }
+            m_aliases[key] = fullPath;
        }

        const char* LocalFileIO::GetAlias(const char* key) const
        {
-            const auto it = AZStd::find_if(m_aliases.begin(), m_aliases.end(), [key](const AliasType& alias)
-                    {
-                        return alias.first.compare(key) == 0;
-                    });
-
+            const auto it = m_aliases.find(key);
            if (it != m_aliases.end())
            {
                return it->second.c_str();
@@ -574,10 +566,7 @@ namespace AZ

        void LocalFileIO::ClearAlias(const char* key)
        {
-            m_aliases.erase(AZStd::remove_if(m_aliases.begin(), m_aliases.end(), [key](const AliasType& alias)
-                    {
-                        return alias.first.compare(key) == 0;
-                    }), m_aliases.end());
+            m_aliases.erase(key);
        }

        AZStd::optional<AZ::u64> LocalFileIO::ConvertToAliasBuffer(char* outBuffer, AZ::u64 outBufferLength, AZStd::string_view inBuffer) const
@@ -682,55 +671,67 @@ namespace AZ

        bool LocalFileIO::ResolveAliases(const char* path, char* resolvedPath, AZ::u64 resolvedPathSize) const
        {
-            AZ_Assert(path != resolvedPath && resolvedPathSize > strlen(path), "Resolved path is incorrect");
            AZ_Assert(path && path[0] != '%', "%% is deprecated, @ is the only valid alias token");
-
+            AZStd::string_view pathView(path);
+            AZStd::string_view aliasKey;
+            AZStd::string_view aliasValue;
+            for (const auto& alias : m_aliases)
+            {
+                AZStd::string_view key{ alias.first };
+                if (AZ::StringFunc::StartsWith(pathView, key)) // we only support aliases at the front of the path
+                {
+                    aliasKey = key;
+                    aliasValue = alias.second;
+                    break;
+                }
+            }
+            size_t requiredResolvedPathSize = pathView.size() - aliasKey.size() + aliasValue.size() + 1;
+            AZ_Assert(path != resolvedPath && resolvedPathSize >= requiredResolvedPathSize, "Resolved path is incorrect");
            // we assert above, but we also need to properly handle the case when the resolvedPath buffer size
            // is too small to copy the source into.
-            size_t pathLen = strlen(path) + 1; // account for null
-            if (path == resolvedPath || (resolvedPathSize < pathLen))
+            if (path == resolvedPath || (resolvedPathSize < requiredResolvedPathSize))
            {
                return false;
            }

-            azstrncpy(resolvedPath, resolvedPathSize, path, pathLen);
-            for (const auto& alias : m_aliases)
+            // Skip past the alias key in the pathView
+            // must ensure that we are replacing the entire folder name, not a partial (e.g. @GAME01@/ vs @GAME0@/)
+            if (AZStd::string_view postAliasView = pathView.substr(aliasKey.size());
+                !aliasKey.empty() && (postAliasView.empty() || postAliasView.starts_with('/') || postAliasView.starts_with('\\')))
            {
-                const char* key = alias.first.c_str();
-                size_t keyLen = alias.first.length();
-                if (azstrnicmp(resolvedPath, key, keyLen) == 0) // we only support aliases at the front of the path
+                // Copy over resolved alias path first
+                size_t resolvedPathLen = 0;
+                aliasValue.copy(resolvedPath, aliasValue.size());
+                resolvedPathLen += aliasValue.size();
+                // Append the post alias path next
+                postAliasView.copy(resolvedPath + resolvedPathLen, postAliasView.size());
+                resolvedPathLen += postAliasView.size();
+                // Null-Terminated the resolved path
+                resolvedPath[resolvedPathLen] = '\0';
+                // If the path started with one of the "asset cache" path aliases, lowercase the path
+                const char* assetAliasPath = GetAlias("@assets@");
+                const char* rootAliasPath = GetAlias("@root@");
+                const char* projectPlatformCacheAliasPath = GetAlias("@projectplatformcache@");
+                const bool lowercasePath = (assetAliasPath != nullptr && AZ::StringFunc::StartsWith(resolvedPath, assetAliasPath)) ||
+                    (rootAliasPath != nullptr && AZ::StringFunc::StartsWith(resolvedPath, rootAliasPath)) ||
+                    (projectPlatformCacheAliasPath != nullptr && AZ::StringFunc::StartsWith(resolvedPath, projectPlatformCacheAliasPath));
+                if (lowercasePath)
                {
-                    [[maybe_unused]] bool lowercasePath = LowerIfBeginsWith(resolvedPath, resolvedPathSize, "@assets@")
-                        || LowerIfBeginsWith(resolvedPath, resolvedPathSize, "@root@")
-                        || LowerIfBeginsWith(resolvedPath, resolvedPathSize, "@projectplatformcache@");
-
-                    const char* dest = alias.second.c_str();
-                    size_t destLen = alias.second.length();
-                    char* afterKey = resolvedPath + keyLen;
-                    size_t afterKeyLen = pathLen - keyLen;
-                    // must ensure that we are replacing the entire folder name, not a partial (e.g. @GAME01@/ vs @GAME0@/)
-                    if (*afterKey == '/' || *afterKey == '\\' || *afterKey == 0)
-                    {
-                        if (afterKeyLen + destLen + 1 < resolvedPathSize)//if after replacing the alias the length is greater than the max path size than skip
-                        {
-                            // scoot the right hand side of the replacement over to make room
-                            memmove(resolvedPath + destLen, afterKey, afterKeyLen + 1); // make sure null is copied
-                            memcpy(resolvedPath, dest, destLen); // insert replacement
-                            pathLen -= keyLen;
-                            pathLen += destLen;
-
-                            AZStd::replace(resolvedPath, resolvedPath + resolvedPathSize, '\\', '/');
-                            return true;
-                        }
-                    }
+                    AZStd::to_lower(resolvedPath, resolvedPath + resolvedPathLen);
                }
+                // Replace any backslashes with posix slashes
+                AZStd::replace(resolvedPath, resolvedPath + resolvedPathLen, AZ::IO::WindowsPathSeparator, AZ::IO::PosixPathSeparator);
+                return true;
+            }
+            else
+            {
+                // The input path doesn't start with an available, copy it directly to the resolved path
+                pathView.copy(resolvedPath, pathView.size());
+                // Null-Terminated the resolved path
+                resolvedPath[pathView.size()] = '\0';
            }
-
            // warn on failing to resolve an alias
-            AZ_Warning(
-                "LocalFileIO::ResolveAlias", path && path[0] != '@',
-                "Failed to resolve an alias: %s", path ? path : "(null)");
-
+            AZ_Warning("LocalFileIO::ResolveAlias", path && path[0] != '@', "Failed to resolve an alias: %s", path ? path : "(null)");
            return false;
        }

@@ -803,7 +804,7 @@ namespace AZ
            return false;
        }

-        AZ::OSString LocalFileIO::RemoveTrailingSlash(const AZ::OSString& pathStr)
+        AZStd::string LocalFileIO::RemoveTrailingSlash(const AZStd::string& pathStr)
        {
            if (pathStr.empty() || (pathStr[pathStr.length() - 1] != '/' && pathStr[pathStr.length() - 1] != '\\'))
            {
@@ -813,7 +814,7 @@ namespace AZ
            return pathStr.substr(0, pathStr.length() - 1);
        }

-        AZ::OSString LocalFileIO::CheckForTrailingSlash(const AZ::OSString& pathStr)
+        AZStd::string LocalFileIO::CheckForTrailingSlash(const AZStd::string& pathStr)
        {
            if (pathStr.empty() || pathStr[pathStr.length() - 1] == '/')
            {
@@ -8,16 +8,12 @@
 #pragma once

 #include <AzCore/base.h>
-#include <AzCore/std/containers/vector.h>
-#include <AzCore/std/containers/map.h>
-#include <AzCore/std/utils.h>
-#include <AzCore/std/string/osstring.h>
+#include <AzCore/std/containers/unordered_map.h>
 #include <AzCore/std/parallel/mutex.h>
 #include <AzCore/std/parallel/atomic.h>
-#include <AzCore/std/parallel/lock.h>
 #include <AzCore/IO/FileIO.h>
-#include <AzCore/Memory/OSAllocator.h>
 #include <AzCore/RTTI/RTTI.h>
+#include <AzCore/Memory/Memory.h>

 // This header file and CPP handles the platform specific implementation of code as defined by the FileIOBase interface class.
 // In order to make your code portable and functional with both this and the RemoteFileIO class, use the interface to access
@@ -33,7 +29,7 @@ namespace AZ
        {
        public:
            AZ_RTTI(LocalFileIO, "{87A8D32B-F695-4105-9A4D-D99BE15DFD50}", FileIOBase);
-            AZ_CLASS_ALLOCATOR(LocalFileIO, OSAllocator, 0);
+            AZ_CLASS_ALLOCATOR(LocalFileIO, SystemAllocator, 0);

            LocalFileIO();
            ~LocalFileIO();
@@ -77,8 +73,6 @@ namespace AZ
            bool ConvertToAbsolutePath(const char* path, char* absolutePath, AZ::u64 maxLength) const;
            
        private:
-            typedef AZStd::pair<AZ::OSString, AZ::OSString> AliasType;
-
            SystemFile* GetFilePointerFromHandle(HandleType fileHandle);

            HandleType GetNextHandle();
@@ -90,13 +84,13 @@ namespace AZ
            bool LowerIfBeginsWith(char* inOutBuffer, AZ::u64 bufferLen, const char* alias) const;

        private:
-            static AZ::OSString RemoveTrailingSlash(const AZ::OSString& pathStr);
-            static AZ::OSString CheckForTrailingSlash(const AZ::OSString& pathStr);
+            static AZStd::string RemoveTrailingSlash(const AZStd::string& pathStr);
+            static AZStd::string CheckForTrailingSlash(const AZStd::string& pathStr);

            mutable AZStd::recursive_mutex m_openFileGuard;
            AZStd::atomic<HandleType> m_nextHandle;
-            AZStd::map<HandleType, SystemFile, AZStd::less<HandleType>, AZ::OSStdAllocator> m_openFiles;
-            AZStd::vector<AliasType, AZ::OSStdAllocator> m_aliases;
+            AZStd::unordered_map<HandleType, SystemFile> m_openFiles;
+            AZStd::unordered_map<AZStd::string, AZStd::string> m_aliases;

            void CheckInvalidWrite(const char* path);
        };
@@ -100,7 +100,7 @@ namespace AZ
            char resolvedPath[AZ_MAX_PATH_LEN];
            ResolvePath(filePath, resolvedPath, AZ_MAX_PATH_LEN);

-            AZ::OSString pathWithoutSlash = RemoveTrailingSlash(resolvedPath);
+            AZStd::string pathWithoutSlash = RemoveTrailingSlash(resolvedPath);
            bool isInAPK = AZ::Android::Utils::IsApkPath(pathWithoutSlash.c_str());

            if (isInAPK)
@@ -115,7 +115,7 @@ namespace AZ
                        // Skip over the current and parent directory paths
                        if (filenameView != "." && filenameView != ".." && NameMatchesFilter(name, filter))
                        {
-                            AZ::OSString foundFilePath = CheckForTrailingSlash(resolvedPath);
+                            AZStd::string foundFilePath = CheckForTrailingSlash(resolvedPath);
                            foundFilePath += name;
                            // if aliased, de-alias!
                            azstrcpy(tempBuffer, AZ_MAX_PATH_LEN, foundFilePath.c_str());
@@ -150,7 +150,7 @@ namespace AZ
                        // Skip over the current and parent directory paths
                        if (filenameView != "." && filenameView != ".." && NameMatchesFilter(entry->d_name, filter))
                        {
-                            AZ::OSString foundFilePath = CheckForTrailingSlash(resolvedPath);
+                            AZStd::string foundFilePath = CheckForTrailingSlash(resolvedPath);
                            foundFilePath += entry->d_name;
                            // if aliased, de-alias!
                            azstrcpy(tempBuffer, AZ_MAX_PATH_LEN, foundFilePath.c_str());
@@ -199,7 +199,7 @@ namespace AZ
            }

            // make directories from bottom to top.
-            AZ::OSString pathBuffer;
+            AZStd::string pathBuffer;
            size_t pathLength = strlen(resolvedPath);
            pathBuffer.reserve(pathLength);
            for (size_t pathPos = 0; pathPos < pathLength; ++pathPos)
@@ -61,7 +61,7 @@ namespace AZ
            char resolvedPath[AZ_MAX_PATH_LEN] = {0};
            ResolvePath(filePath, resolvedPath, AZ_MAX_PATH_LEN);

-            AZ::OSString withoutSlash = RemoveTrailingSlash(resolvedPath);
+            AZStd::string withoutSlash = RemoveTrailingSlash(resolvedPath);
            DIR* dir = opendir(withoutSlash.c_str());

            if (dir != nullptr)
@@ -80,7 +80,7 @@ namespace AZ
                    // Skip over the current and parent directory paths
                    if (filenameView != "." && filenameView != ".." && NameMatchesFilter(entry->d_name, filter))
                    {
-                        AZ::OSString foundFilePath = CheckForTrailingSlash(resolvedPath);
+                        AZStd::string foundFilePath = CheckForTrailingSlash(resolvedPath);
                        foundFilePath += entry->d_name;
                        // if aliased, dealias!
                        azstrcpy(tempBuffer, AZ_MAX_PATH_LEN, foundFilePath.c_str());
@@ -116,7 +116,7 @@ namespace AZ
            }

            // make directories from bottom to top.
-            AZ::OSString buf;
+            AZStd::string buf;
            size_t pathLength = strlen(resolvedPath);
            buf.reserve(pathLength);
            for (size_t pos = 0; pos < pathLength; ++pos)
@@ -128,7 +128,7 @@ namespace AZ
            }

            // make directories from bottom to top.
-            AZ::OSString buf;
+            AZStd::string buf;
            size_t pathLength = strlen(resolvedPath);
            buf.reserve(pathLength);
            for (size_t pos = 0; pos < pathLength; ++pos)
@@ -234,7 +234,7 @@ namespace AzFramework
            GetRawInputData((HRAWINPUT)lParam, RID_INPUT, NULL, &rawInputSize, rawInputHeaderSize);

            LPBYTE rawInputBytes = new BYTE[rawInputSize];
-            const UINT bytesCopied = GetRawInputData((HRAWINPUT)lParam, RID_INPUT, rawInputBytes, &rawInputSize, rawInputHeaderSize);
+            GetRawInputData((HRAWINPUT)lParam, RID_INPUT, rawInputBytes, &rawInputSize, rawInputHeaderSize);

            RAWINPUT* rawInput = (RAWINPUT*)rawInputBytes;
            AzFramework::RawInputNotificationBusWindows::Broadcast(
@@ -752,7 +752,9 @@ namespace UnitTest
            // Test that sending in a too small output path fails,
            // if the output buffer is too small to hold the resolved path
            size_t SMALLER_THAN_FINAL_RESOLVED_PATH = expectedResolvedPath.length() - 1;
+            AZ_TEST_START_TRACE_SUPPRESSION;
            resolveDidWork = local.ResolvePath(aliasTestPath, aliasResolvedPath, SMALLER_THAN_FINAL_RESOLVED_PATH);
+            AZ_TEST_STOP_TRACE_SUPPRESSION(1);
            AZ_TEST_ASSERT(!resolveDidWork);

            // test clearing an alias