Addendum #2 to 2ab6fe0 (Sync up to code review fixes from master)

Author: Dutchman101

Client/loader/CInstallManager.cpp

@@ -315,8 +315,20 @@ void CInstallManager::SetMTASAPathSource(const SString& strCommandLineIn)
         // The override must be installed before UpdateSettingsForReportLog runs, because that helper
         // reaches GetInstallPathForLauncher / GetMTASAPath through UpdateMTAVersionApplicationSetting.
         const SString strRealInstallRoot = m_pSequencer->GetVariable(INSTALL_ROOT);
-        if (!strRealInstallRoot.empty() && IsUsableMtasaInstallRoot(strRealInstallRoot) && !IsTemporaryUpdateLaunchPath(strRealInstallRoot))
+        if (strRealInstallRoot.empty())
+        {
+            AddReportLog(1063, "CInstallManager::SetMTASAPathSource: no sequencer install root, falling back to registry");
+        }
+        else if (SharedUtil::IsUsableMtasaInstallRoot(strRealInstallRoot) && !SharedUtil::IsTemporaryUpdateLaunchPath(strRealInstallRoot))
+        {
+            AddReportLog(1063, SString("CInstallManager::SetMTASAPathSource: honoring sequencer install root '%s'", strRealInstallRoot.c_str()));
             SharedUtil::SetMTASABaseDirOverride(strRealInstallRoot);
+        }
+        else
+        {
+            AddReportLog(1063, SString("CInstallManager::SetMTASAPathSource: rejecting unusable sequencer install root '%s', falling back to registry",
+                                       strRealInstallRoot.c_str()));
+        }
 
         ::SetMTASAPathSource(true);
     }

Client/loader/Main.cpp

@@ -150,7 +150,7 @@ namespace
 
         // A temp updater shouldnt load the install root's core.dll before RunInstall;
         // the localization module stays loaded and would block replacement.
-        if (!IsTemporaryUpdateLaunchPath(GetLaunchPath()))
+        if (!SharedUtil::IsTemporaryUpdateLaunchPath(GetLaunchPath()))
             InitLocalization(false);
 
         // Handle commands from the installer

Client/loader/Utils.cpp

@@ -579,7 +579,8 @@ namespace
         DWORD valueType = REG_SZ;
         DWORD valueSize = 0;
         LONG  result = RegQueryValueExW(hKey, wstrValueName.c_str(), nullptr, &valueType, nullptr, &valueSize);
-        if (result != ERROR_SUCCESS || (valueType != REG_SZ && valueType != REG_EXPAND_SZ) || valueSize == 0 || valueSize > kMaxRegistryValueBytes)
+        if (result != ERROR_SUCCESS || (valueType != REG_SZ && valueType != REG_EXPAND_SZ) || valueSize == 0 || valueSize > kMaxRegistryValueBytes ||
+            (valueSize % sizeof(wchar_t)) != 0)
         {
             RegCloseKey(hKey);
             return "";
@@ -589,13 +590,11 @@ namespace
         result = RegQueryValueExW(hKey, wstrValueName.c_str(), nullptr, &valueType, reinterpret_cast<LPBYTE>(buffer.data()), &valueSize);
         RegCloseKey(hKey);
 
-        if (result != ERROR_SUCCESS || (valueType != REG_SZ && valueType != REG_EXPAND_SZ) || valueSize > kMaxRegistryValueBytes)
+        if (result != ERROR_SUCCESS || (valueType != REG_SZ && valueType != REG_EXPAND_SZ) || valueSize > kMaxRegistryValueBytes ||
+            (valueSize % sizeof(wchar_t)) != 0)
             return "";
 
-        if (valueSize >= sizeof(wchar_t))
-            buffer[(valueSize / sizeof(wchar_t)) - 1u] = L'\0';
-        else
-            buffer[0] = L'\0';
+        buffer[valueSize / sizeof(wchar_t)] = L'\0';
 
         // Expand environment variable references for REG_EXPAND_SZ values so callers see a real
         // filesystem path. Without this, a value like "%ProgramFiles%\..." would fail validation.
@@ -620,7 +619,7 @@ namespace
 SString GetInstallPathForLauncher()
 {
     const SString strLaunchPath = GetLaunchPath();
-    if (!IsTemporaryUpdateLaunchPath(strLaunchPath))
+    if (!SharedUtil::IsTemporaryUpdateLaunchPath(strLaunchPath))
         return strLaunchPath;
 
     // Prefer the resolved base dir over the registry when one was already established. In the
@@ -633,7 +632,7 @@ SString GetInstallPathForLauncher()
     // back into GetInstallPathForLauncher() and would recurse for any temp launcher whose g_strMTASAPath
     // has not yet been populated by the far-update sequencer path. Empty string falls through to the
     // per-view registry loop below, preserving the temp-launcher-without-far-update fallback.
-    if (!g_strMTASAPath.empty() && IsUsableMtasaInstallRoot(g_strMTASAPath) && !IsTemporaryUpdateLaunchPath(g_strMTASAPath) &&
+    if (!g_strMTASAPath.empty() && SharedUtil::IsUsableMtasaInstallRoot(g_strMTASAPath) && !SharedUtil::IsTemporaryUpdateLaunchPath(g_strMTASAPath) &&
         !g_strMTASAPath.CompareI(strLaunchPath))
         return g_strMTASAPath;
 
@@ -651,7 +650,7 @@ SString GetInstallPathForLauncher()
     for (int i = 0; i < viewCount; ++i)
     {
         const SString strSavedInstallPath = ReadInstallRootRegistryView(viewFlags[i]);
-        if (IsUsableMtasaInstallRoot(strSavedInstallPath) && !IsTemporaryUpdateLaunchPath(strSavedInstallPath))
+        if (SharedUtil::IsUsableMtasaInstallRoot(strSavedInstallPath) && !SharedUtil::IsTemporaryUpdateLaunchPath(strSavedInstallPath))
             return strSavedInstallPath;
     }
 
@@ -661,30 +660,6 @@ SString GetInstallPathForLauncher()
     return SString();
 }
 
-bool IsUsableMtasaInstallRoot(const SString& strPath)
-{
-    if (strPath.empty())
-        return false;
-
-    return FileExists(PathJoin(strPath, "Multi Theft Auto.exe")) || FileExists(PathJoin(strPath, "Multi Theft Auto_d.exe")) ||
-           FileExists(PathJoin(strPath, "mta", "core.dll")) || FileExists(PathJoin(strPath, "MTA", "core.dll")) ||
-           FileExists(PathJoin(strPath, "mta", "core_d.dll")) || FileExists(PathJoin(strPath, "MTA", "core_d.dll"));
-}
-
-bool IsTemporaryUpdateLaunchPath(const SString& strLaunchPath)
-{
-    if (strLaunchPath.empty())
-        return false;
-
-    if (!strLaunchPath.ContainsI("\\upcache\\"))
-        return false;
-
-    // The auto-update flow creates the extraction directory in Install.cpp::CheckOnRestartCommand
-    // as MakeUniquePath("...\\upcache\\_<archiveName>_tmp_"), so the leaf always begins with '_'.
-    const SString strLeaf = ExtractFilename(strLaunchPath);
-    return strLeaf.BeginsWith("_") && strLeaf.ContainsI("_tmp_");
-}
-
 void SetMTASAPathSource(bool bReadFromRegistry)
 {
     if (bReadFromRegistry)
@@ -718,7 +693,7 @@ void SetMTASAPathSource(bool bReadFromRegistry)
         // values. A temp launcher started without the far-update sequencer state would otherwise
         // contaminate Last Run Location with an upcache\_*_tmp_* path that gets deleted later by
         // CleanDownloadCache, leaving a stale registry pointer that produces U01 on the next launch.
-        if (IsTemporaryUpdateLaunchPath(strLaunchPath))
+        if (SharedUtil::IsTemporaryUpdateLaunchPath(strLaunchPath))
         {
             AddReportLog(1063, SString("SetMTASAPathSource: refusing to record temp launch path '%s' in registry", strLaunchPath.c_str()));
             g_strMTASAPath = strLaunchPath;
@@ -1391,7 +1366,7 @@ void UpdateMTAVersionApplicationSetting(bool bQuiet)
     bool    bFreeModule = false;
 
     const SString strInstallPath = GetInstallPathForLauncher();
-    if (IsUsableMtasaInstallRoot(strInstallPath))
+    if (SharedUtil::IsUsableMtasaInstallRoot(strInstallPath))
     {
         hModule = LoadVersionModule(strInstallPath, dwLastError);
         bFreeModule = hModule != NULL;
@@ -1404,7 +1379,7 @@ void UpdateMTAVersionApplicationSetting(bool bQuiet)
     if (!hModule)
     {
         const SString strBaseDirPath = GetMTASAPath();
-        if (IsUsableMtasaInstallRoot(strBaseDirPath) && !strBaseDirPath.CompareI(strInstallPath))
+        if (SharedUtil::IsUsableMtasaInstallRoot(strBaseDirPath) && !strBaseDirPath.CompareI(strInstallPath))
         {
             hModule = LoadVersionModule(strBaseDirPath, dwLastError);
             bFreeModule = hModule != NULL;

Client/loader/Utils.h

@@ -79,8 +79,6 @@ auto    GetGameBaseDirectory() -> std::filesystem::path;
 auto    GetGameLaunchDirectory() -> std::filesystem::path;
 auto    GetGameExecutablePath() -> std::filesystem::path;
 SString GetInstallPathForLauncher();
-bool    IsUsableMtasaInstallRoot(const SString& strPath);
-bool    IsTemporaryUpdateLaunchPath(const SString& strLaunchPath);
 
 void            SetMTASAPathSource(bool bReadFromRegistry);
 SString         GetMTASAPath();

Shared/sdk/SharedUtil.Misc.h

@@ -123,6 +123,9 @@ namespace SharedUtil
     //
     void SetMTASABaseDirOverride(const SString& strPath);
 
+    bool IsUsableMtasaInstallRoot(const SString& strPath);
+    bool IsTemporaryUpdateLaunchPath(const SString& strLaunchPath);
+
     //
     // Get startup directory as saved in the registry by the launcher
     // Used in the Win32 Client only

Shared/sdk/SharedUtil.Misc.hpp

@@ -396,7 +396,7 @@ void SharedUtil::SetMTASABaseDirOverride(const SString& strPath)
     strInstallRootOverride = strPath;
 }
 
-static bool IsUsableMtasaInstallRoot(const SString& strPath)
+bool SharedUtil::IsUsableMtasaInstallRoot(const SString& strPath)
 {
     if (strPath.empty())
         return false;
@@ -406,22 +406,23 @@ static bool IsUsableMtasaInstallRoot(const SString& strPath)
            FileExists(PathJoin(strPath, "mta", "core_d.dll")) || FileExists(PathJoin(strPath, "MTA", "core_d.dll"));
 }
 
-// A path inside upcache\_<archive>_tmp_<n> is a transient extraction directory used by the auto-update
-// flow. Such a path can satisfy IsUsableMtasaInstallRoot while it still exists, but it must never be
-// accepted as the durable MTA install root. The launcher-side helper in Client/loader/Utils.cpp uses
-// the same shape; if either copy changes, update both.
-static bool IsTemporaryUpdateLaunchPath(const SString& strLaunchPath)
+// A path inside upcache\_<archive>_tmp_<n> is an auto-update extraction directory.
+// It can look like a usable install root while it exists, but it must not be kept as the real install root.
+bool SharedUtil::IsTemporaryUpdateLaunchPath(const SString& strLaunchPath)
 {
     if (strLaunchPath.empty())
         return false;
 
-    if (!strLaunchPath.ContainsI("\\upcache\\"))
+    const SString strNormalizedLaunchPath = PathConform(strLaunchPath).TrimEnd("\\");
+    if (strNormalizedLaunchPath.empty())
         return false;
 
-    // The auto-update flow creates the extraction directory in Install.cpp::CheckOnRestartCommand
-    // as MakeUniquePath("...\\upcache\\_<archiveName>_tmp_"), so the leaf always begins with '_'.
-    const SString strLeaf = ExtractFilename(strLaunchPath);
-    return strLeaf.BeginsWith("_") && strLeaf.ContainsI("_tmp_");
+    const SString strLeaf = ExtractFilename(strNormalizedLaunchPath);
+    if (!strLeaf.BeginsWith("_") || !strLeaf.ContainsI("_tmp_"))
+        return false;
+
+    const SString strParent = ExtractFilename(ExtractPath(strNormalizedLaunchPath));
+    return strParent.CompareI("upcache");
 }
 
 // Read Last Run Location from a specific registry view. viewFlag should be one of
@@ -443,7 +444,8 @@ static SString ReadInstallRootRegistryValueView(REGSAM viewFlag)
     DWORD dwType = REG_SZ;
     DWORD dwSize = 0;
     LONG  result = RegQueryValueExW(hkTemp, wstrValue, NULL, &dwType, NULL, &dwSize);
-    if (result != ERROR_SUCCESS || (dwType != REG_SZ && dwType != REG_EXPAND_SZ) || dwSize == 0 || dwSize > kMaxRegistryValueBytes)
+    if (result != ERROR_SUCCESS || (dwType != REG_SZ && dwType != REG_EXPAND_SZ) || dwSize == 0 || dwSize > kMaxRegistryValueBytes ||
+        (dwSize % sizeof(wchar_t)) != 0)
     {
         RegCloseKey(hkTemp);
         return "";
@@ -453,13 +455,11 @@ static SString ReadInstallRootRegistryValueView(REGSAM viewFlag)
     result = RegQueryValueExW(hkTemp, wstrValue, NULL, &dwType, reinterpret_cast<LPBYTE>(buffer.data()), &dwSize);
     RegCloseKey(hkTemp);
 
-    if (result != ERROR_SUCCESS || (dwType != REG_SZ && dwType != REG_EXPAND_SZ) || dwSize > kMaxRegistryValueBytes)
+    if (result != ERROR_SUCCESS || (dwType != REG_SZ && dwType != REG_EXPAND_SZ) || dwSize > kMaxRegistryValueBytes ||
+        (dwSize % sizeof(wchar_t)) != 0)
         return "";
 
-    if (dwSize >= sizeof(wchar_t))
-        buffer[(dwSize / sizeof(wchar_t)) - 1u] = L'\0';
-    else
-        buffer[0] = L'\0';
+    buffer[dwSize / sizeof(wchar_t)] = L'\0';
 
     // Expand environment variable references for REG_EXPAND_SZ values so callers see a real
     // filesystem path. Without this, a value like "%ProgramFiles%\..." would fail validation.
@@ -499,7 +499,7 @@ SString SharedUtil::GetMTASABaseDir()
             // install-root checks used elsewhere and reject temp update paths so the parent-process
             // branch cannot point this process at a directory that is about to be deleted.
             SString strParentDir = ExtractPath(GetParentProcessPathFilename(GetCurrentProcessId()));
-            if (IsUsableMtasaInstallRoot(strParentDir) && !IsTemporaryUpdateLaunchPath(strParentDir))
+            if (SharedUtil::IsUsableMtasaInstallRoot(strParentDir) && !SharedUtil::IsTemporaryUpdateLaunchPath(strParentDir))
             {
                 strInstallRoot = strParentDir;
                 strInstallRootSource = "parent process";
@@ -543,7 +543,7 @@ SString SharedUtil::GetMTASABaseDir()
                     continue;
                 if (strFirstNonEmptyRegistryValue.empty())
                     strFirstNonEmptyRegistryValue = strCandidate;
-                if (IsUsableMtasaInstallRoot(strCandidate) && !IsTemporaryUpdateLaunchPath(strCandidate))
+                if (SharedUtil::IsUsableMtasaInstallRoot(strCandidate) && !SharedUtil::IsTemporaryUpdateLaunchPath(strCandidate))
                 {
                     strInstallRoot = strCandidate;
                     strInstallRootSource = (viewFlags[i] == 0) ? "registry" :