Lookup Installed Version in the Clients Registry

If an installer is abandoned or an app is modified by an external source
the version in the updater prefs may be out of sync with the installed
version. To prevent these values from getting out of sync lookup the
product version in the `Clients` registry, and, if its a valid version,
override the version in the updater prefs.

This fix is not necessary on macOS because `LookupVersion` is already
implemented on macOS to lookup the version in the app's registered
plist.

Bug: 370543468
Change-Id: Ia39daf0f427adb763e2b1debcffcf358a639ab3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5909796
Commit-Queue: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1365696}

chrome/updater/installer.cc

@@ -19,6 +19,7 @@
 #include "base/threading/scoped_blocking_call.h"
 #include "base/time/time.h"
 #include "base/values.h"
+#include "base/version.h"
 #include "build/build_config.h"
 #include "chrome/updater/action_handler.h"
 #include "chrome/updater/constants.h"
@@ -47,7 +48,8 @@ AppInfo MakeAppInfo(UpdaterScope scope,
                     const base::FilePath& brand_path,
                     const std::string& brand_key,
                     const base::FilePath& ec_path) {
-  const base::Version pv_lookup = LookupVersion(pv_path, pv_key, pv);
+  const base::Version pv_lookup =
+      LookupVersion(scope, app_id, pv_path, pv_key, pv);
   return AppInfo(scope, app_id, LookupString(ap_path, ap_key, ap),
                  LookupString(brand_path, brand_key, brand),
                  pv_lookup.IsValid() ? pv_lookup : base::Version(kNullVersion),

chrome/updater/installer.h

@@ -70,8 +70,13 @@ InstallerResult RunApplicationInstaller(
 std::string LookupString(const base::FilePath& path,
                          const std::string& keyname,
                          const std::string& default_value);
-base::Version LookupVersion(const base::FilePath& path,
-                            const std::string& keyname,
+
+// Retrieves the version of the installed application. If the version cannot
+// be determined the `default_value` is returned.
+base::Version LookupVersion(UpdaterScope scope,
+                            const std::string& app_id,
+                            const base::FilePath& version_path,
+                            const std::string& version_key,
                             const base::Version& default_value);
 
 // Manages the install of one application. Some of the functions of this

chrome/updater/installer_linux.cc

@@ -5,13 +5,17 @@
 #include "chrome/updater/installer.h"
 
 #include <optional>
+#include <string>
 
+#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/process/launch.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
+#include "base/version.h"
 #include "chrome/updater/constants.h"
+#include "chrome/updater/updater_scope.h"
 
 namespace updater {
 
@@ -65,8 +69,10 @@ std::string LookupString(const base::FilePath& path,
   return default_value;
 }
 
-base::Version LookupVersion(const base::FilePath& path,
-                            const std::string& keyname,
+base::Version LookupVersion(UpdaterScope scope,
+                            const std::string& app_id,
+                            const base::FilePath& version_path,
+                            const std::string& version_key,
                             const base::Version& default_value) {
   return default_value;
 }

chrome/updater/installer_mac.cc

@@ -5,14 +5,17 @@
 #include "chrome/updater/installer.h"
 
 #include <optional>
+#include <string>
 
 #include "base/files/file_path.h"
 #include "base/functional/callback.h"
 #include "base/logging.h"
 #include "base/strings/strcat.h"
 #include "base/time/time.h"
+#include "base/version.h"
 #include "chrome/updater/constants.h"
 #include "chrome/updater/mac/install_from_archive.h"
+#include "chrome/updater/updater_scope.h"
 #include "chrome/updater/util/mac_util.h"
 
 namespace updater {
@@ -48,10 +51,13 @@ std::string LookupString(const base::FilePath& path,
   return value ? *value : default_value;
 }
 
-base::Version LookupVersion(const base::FilePath& path,
-                            const std::string& keyname,
+base::Version LookupVersion(UpdaterScope scope,
+                            const std::string& app_id,
+                            const base::FilePath& version_path,
+                            const std::string& version_key,
                             const base::Version& default_value) {
-  std::optional<std::string> value = ReadValueFromPlist(path, keyname);
+  std::optional<std::string> value =
+      ReadValueFromPlist(version_path, version_key);
   if (value) {
     base::Version value_version(*value);
     return value_version.IsValid() ? value_version : default_value;

chrome/updater/win/installer_api.cc

@@ -24,6 +24,7 @@
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "base/timer/elapsed_timer.h"
+#include "base/version.h"
 #include "base/win/registry.h"
 #include "chrome/updater/constants.h"
 #include "chrome/updater/enum_traits.h"
@@ -544,9 +545,18 @@ std::string LookupString(const base::FilePath& path,
   return default_value;
 }
 
-base::Version LookupVersion(const base::FilePath& path,
-                            const std::string& keyname,
+base::Version LookupVersion(UpdaterScope scope,
+                            const std::string& app_id,
+                            const base::FilePath& version_path,
+                            const std::string& version_key,
                             const base::Version& default_value) {
+  std::wstring pv;
+  if (base::win::RegKey(UpdaterScopeToHKeyRoot(scope),
+                        GetAppClientsKey(app_id).c_str(), Wow6432(KEY_READ))
+          .ReadValue(kRegValuePV, &pv) == ERROR_SUCCESS) {
+    base::Version value_version = base::Version(base::WideToUTF8(pv));
+    return value_version.IsValid() ? value_version : default_value;
+  }
   return default_value;
 }
 

chrome/updater/win/installer_api_unittest.cc

@@ -7,8 +7,10 @@
 #include <optional>
 #include <string>
 
+#include "base/files/file_path.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/test_reg_util_win.h"
+#include "base/version.h"
 #include "base/win/registry.h"
 #include "chrome/updater/constants.h"
 #include "chrome/updater/updater_scope.h"
@@ -260,4 +262,52 @@ TEST_P(InstallerAPITest, ClientStateAppKeyOpen) {
   EXPECT_TRUE(ClientStateAppKeyOpen(updater_scope_, kAppId, KEY_READ));
 }
 
+TEST_P(InstallerAPITest, LookupVersionMissing) {
+  ASSERT_NO_FATAL_FAILURE(registry_override_.OverrideRegistry(
+      UpdaterScopeToHKeyRoot(updater_scope_)));
+  base::Version default_version = base::Version("1.1.1.1");
+  base::Version version =
+      LookupVersion(updater_scope_, "{4e346bdc-c3d1-460e-83d7-31555eef96c7}",
+                    base::FilePath(), "", default_version);
+  EXPECT_EQ(default_version, version);
+}
+
+TEST_P(InstallerAPITest, LookupVersionInvalid) {
+  HKEY root = UpdaterScopeToHKeyRoot(updater_scope_);
+  ASSERT_NO_FATAL_FAILURE(registry_override_.OverrideRegistry(root));
+
+  base::win::RegKey key;
+  ASSERT_EQ(
+      key.Create(root,
+                 UPDATER_KEY L"Clients\\{4e346bdc-c3d1-460e-83d7-31555eef96c7}",
+                 Wow6432(KEY_WRITE)),
+      ERROR_SUCCESS);
+  ASSERT_EQ(key.WriteValue(kRegValuePV, L"invalid"), ERROR_SUCCESS);
+
+  base::Version default_version = base::Version("1.1.1.1");
+  base::Version version =
+      LookupVersion(updater_scope_, "{4e346bdc-c3d1-460e-83d7-31555eef96c7}",
+                    base::FilePath(), "", default_version);
+  EXPECT_EQ(default_version, version);
+}
+
+TEST_P(InstallerAPITest, LookupVersionValid) {
+  HKEY root = UpdaterScopeToHKeyRoot(updater_scope_);
+  ASSERT_NO_FATAL_FAILURE(registry_override_.OverrideRegistry(root));
+
+  base::win::RegKey key;
+  ASSERT_EQ(
+      key.Create(root,
+                 UPDATER_KEY L"Clients\\{4e346bdc-c3d1-460e-83d7-31555eef96c7}",
+                 Wow6432(KEY_WRITE)),
+      ERROR_SUCCESS);
+  ASSERT_EQ(key.WriteValue(kRegValuePV, L"1.1.1.2"), ERROR_SUCCESS);
+
+  base::Version default_version = base::Version("1.1.1.1");
+  base::Version version =
+      LookupVersion(updater_scope_, "{4e346bdc-c3d1-460e-83d7-31555eef96c7}",
+                    base::FilePath(), "", default_version);
+  EXPECT_EQ(version, base::Version("1.1.1.2"));
+}
+
 }  // namespace updater

docs/updater/functional_spec.md

@@ -1323,6 +1323,14 @@ POSIX platforms, they will additionally lchown the existence checker path
 registered by the application to be owned by the root user. User-scope updaters
 use this as a signal that the application is managed by a system-scope updater.
 
+#### Windows
+
+Application installers are expected to register with the updater by setting
+[HKCU or HKLM]\SOFTWARE\{Company}\Update\Clients\{AppID} → pv to the installed
+version of the application. If pv is present and valid in the app's Clients
+key it will be used by the updater as the source of truth for the registered
+version.
+
 For backwards compatibility with third party software, on Windows, after a
 successful registration and on each update, the updater will set
 [HKCU or HKLM]\SOFTWARE\{Company}\Update\ClientState\{AppID} → pv to the