Implement StringFromGUID2 natively in Chromium

This change implements StringFromGUID2 natively in Chromium in order to
avoid calling into ole32.dll. This is necessary to support the effort to
delayload user32.dll and gdi32.dll because StringFromGUID2 has some
callers in the renderer process and delayloading ole32.dll for this
method call will cause the process to crash.

Bug: 948829
Change-Id: I00a2d5735984268813ecccc43aa3cb464d21c62c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1575869
Commit-Queue: Cliff Smolinsky <cliffsmo@microsoft.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Tommi <tommi@chromium.org>
Reviewed-by: Misha Efimov <mef@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#654978}

PRESUBMIT.py

@@ -728,6 +728,28 @@
       True,
       (),
     ),
+    (
+      'StringFromGUID2',
+      (
+        'StringFromGUID2 introduces an unnecessary dependency on ole32.dll.',
+        'Use base::win::String16FromGUID instead.'
+      ),
+      True,
+      (
+        r'/base/win/win_util_unittest.cc'
+      ),
+    ),
+    (
+      'StringFromCLSID',
+      (
+        'StringFromCLSID introduces an unnecessary dependency on ole32.dll.',
+        'Use base::win::String16FromGUID instead.'
+      ),
+      True,
+      (
+        r'/base/win/win_util_unittest.cc'
+      ),
+    ),
 )
 
 

base/sampling_heap_profiler/module_cache_win.cc

@@ -15,6 +15,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/win/pe_image.h"
 #include "base/win/scoped_handle.h"
+#include "base/win/win_util.h"
 
 namespace base {
 
@@ -52,12 +53,7 @@ void GetDebugInfoForModule(HMODULE module_handle,
     return;
   *pdb_name = FilePath(std::move(pdb_filename)).BaseName();
 
-  const int kGUIDSize = 39;
-  string16 buffer;
-  int result = ::StringFromGUID2(
-      guid, as_writable_wcstr(WriteInto(&buffer, kGUIDSize)), kGUIDSize);
-  if (result != kGUIDSize)
-    return;
+  auto buffer = win::String16FromGUID(guid);
   RemoveChars(buffer, STRING16_LITERAL("{}-"), &buffer);
   buffer.append(NumberToString16(age));
   *build_id = UTF16ToUTF8(buffer);

base/win/win_util.cc

@@ -22,6 +22,7 @@
 #include <signal.h>
 #include <stddef.h>
 #include <stdlib.h>
+#include <strsafe.h>
 #include <tchar.h>  // Must be before tpcshrd.h or for any use of _T macro
 #include <tpcshrd.h>
 #include <uiviewsettingsinterop.h>
@@ -71,13 +72,12 @@ bool SetPropVariantValueForPropertyStore(
   if (SUCCEEDED(result))
     return true;
 #if DCHECK_IS_ON()
-  ScopedCoMem<OLECHAR> guidString;
-  ::StringFromCLSID(property_key.fmtid, &guidString);
   if (HRESULT_FACILITY(result) == FACILITY_WIN32)
     ::SetLastError(HRESULT_CODE(result));
   // See third_party/perl/c/i686-w64-mingw32/include/propkey.h for GUID and
   // PID definitions.
-  DPLOG(ERROR) << "Failed to set property with GUID " << guidString << " PID "
+  DPLOG(ERROR) << "Failed to set property with GUID "
+               << String16FromGUID(property_key.fmtid) << " PID "
                << property_key.pid;
 #endif
   return false;
@@ -721,6 +721,21 @@ void EnableHighDPISupport() {
   }
 }
 
+string16 String16FromGUID(REFGUID rguid) {
+  // This constant counts the number of characters in the formatted string,
+  // including the null termination character.
+  constexpr int kGuidStringCharacters =
+      1 + 8 + 1 + 4 + 1 + 4 + 1 + 4 + 1 + 12 + 1 + 1;
+  wchar_t guid_string[kGuidStringCharacters];
+  CHECK(SUCCEEDED(StringCchPrintfW(
+      guid_string, kGuidStringCharacters,
+      L"{%08lX-%04X-%04X-%02X%02X-%02X%02X%02X%02X%02X%02X}", rguid.Data1,
+      rguid.Data2, rguid.Data3, rguid.Data4[0], rguid.Data4[1], rguid.Data4[2],
+      rguid.Data4[3], rguid.Data4[4], rguid.Data4[5], rguid.Data4[6],
+      rguid.Data4[7])));
+  return string16(as_u16cstr(guid_string), kGuidStringCharacters - 1);
+}
+
 ScopedDomainStateForTesting::ScopedDomainStateForTesting(bool state)
     : initial_state_(IsEnrolledToDomain()) {
   *GetDomainEnrollmentStateStorage() = state;

base/win/win_util.h

@@ -184,6 +184,9 @@ BASE_EXPORT bool IsProcessPerMonitorDpiAware();
 // Enable high-DPI support for the current process.
 BASE_EXPORT void EnableHighDPISupport();
 
+// Returns a string representation of |rguid|.
+BASE_EXPORT string16 String16FromGUID(REFGUID rguid);
+
 // Allows changing the domain enrolled state for the life time of the object.
 // The original state is restored upon destruction.
 class BASE_EXPORT ScopedDomainStateForTesting {

base/win/win_util_unittest.cc

@@ -4,12 +4,15 @@
 
 #include "base/win/win_util.h"
 
+#include <objbase.h>
+
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/scoped_native_library.h"
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_util.h"
+#include "base/win/scoped_co_mem.h"
 #include "base/win/win_client_metrics.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -80,5 +83,22 @@ TEST(BaseWinUtilTest, TestUint32ToInvalidHandle) {
   EXPECT_EQ(INVALID_HANDLE_VALUE, base::win::Uint32ToHandle(invalid_handle));
 }
 
+TEST(BaseWinUtilTest, String16FromGUID) {
+  const GUID kGuid = {0x7698f759,
+                      0xf5b0,
+                      0x4328,
+                      {0x92, 0x38, 0xbd, 0x70, 0x8a, 0x6d, 0xc9, 0x63}};
+  const base::StringPiece16 kGuidStr(
+      STRING16_LITERAL("{7698F759-F5B0-4328-9238-BD708A6DC963}"));
+  auto guid_string16 = String16FromGUID(kGuid);
+  EXPECT_EQ(guid_string16, kGuidStr);
+  wchar_t guid_wchar[39];
+  ::StringFromGUID2(kGuid, guid_wchar, base::size(guid_wchar));
+  EXPECT_STREQ(as_wcstr(guid_string16), guid_wchar);
+  ScopedCoMem<OLECHAR> clsid_string;
+  ::StringFromCLSID(kGuid, &clsid_string);
+  EXPECT_STREQ(as_wcstr(guid_string16), clsid_string.get());
+}
+
 }  // namespace win
 }  // namespace base

base/win/windows_types.h

@@ -68,6 +68,11 @@ typedef LONG_PTR SSIZE_T, *PSSIZE_T;
 typedef DWORD ACCESS_MASK;
 typedef ACCESS_MASK REGSAM;
 
+// As defined in guiddef.h.
+#ifndef _REFGUID_DEFINED
+#define _REFGUID_DEFINED
+#define REFGUID const GUID&
+#endif
 
 // Forward declare Windows compatible handles.
 

chrome/browser/google/google_update_win.cc

@@ -33,6 +33,7 @@
 #include "base/version.h"
 #include "base/win/atl.h"
 #include "base/win/scoped_bstr.h"
+#include "base/win/win_util.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/install_static/install_util.h"
@@ -137,11 +138,10 @@ HRESULT CoGetClassObjectAsAdmin(gfx::AcceleratedWidget hwnd,
 
   // For Vista+, need to instantiate the class factory via the elevation
   // moniker. This ensures that the UAC dialog shows up.
-  wchar_t class_id_as_string[MAX_PATH] = {};
-  StringFromGUID2(class_id, class_id_as_string, base::size(class_id_as_string));
+  auto class_id_as_string = base::win::String16FromGUID(class_id);
 
   base::string16 elevation_moniker_name = base::StringPrintf(
-      L"Elevation:Administrator!clsid:%ls", class_id_as_string);
+      L"Elevation:Administrator!clsid:%ls", class_id_as_string.c_str());
 
   BIND_OPTS3 bind_opts;
   // An explicit memset is needed rather than relying on value initialization

chrome/chrome_cleaner/components/system_report_component.cc

@@ -41,6 +41,7 @@
 #include "base/win/scoped_bstr.h"
 #include "base/win/scoped_handle.h"
 #include "base/win/scoped_variant.h"
+#include "base/win/win_util.h"
 #include "base/win/windows_version.h"
 #include "chrome/chrome_cleaner/chrome_utils/chrome_util.h"
 #include "chrome/chrome_cleaner/chrome_utils/extension_file_logger.h"
@@ -593,9 +594,7 @@ void ReportLayeredServiceProviders() {
       const std::set<GUID, GUIDLess>& guids = provider->second;
       for (std::set<GUID, GUIDLess>::const_iterator guid = guids.begin();
            guid != guids.end(); ++guid) {
-        base::string16 guid_str;
-        GUIDToString(*guid, &guid_str);
-        logged_guids.push_back(guid_str);
+        logged_guids.push_back(base::win::String16FromGUID(*guid));
       }
       LoggingServiceAPI::GetInstance()->AddLayeredServiceProvider(
           logged_guids, file_information);

chrome/chrome_cleaner/os/disk_util.cc

@@ -29,6 +29,7 @@
 #include "base/win/current_module.h"
 #include "base/win/registry.h"
 #include "base/win/shortcut.h"
+#include "base/win/win_util.h"
 #include "base/win/windows_version.h"
 #include "chrome/chrome_cleaner/constants/version.h"
 #include "chrome/chrome_cleaner/os/file_path_sanitization.h"
@@ -658,9 +659,9 @@ void GetLayeredServiceProviders(const LayeredServiceProviderAPI& lsp_api,
           providers->emplace(base::FilePath(path), std::set<GUID, GUIDLess>());
       inserted.first->second.insert(service_providers[i].ProviderId);
     } else {
-      base::string16 guid;
-      GUIDToString(service_providers[i].ProviderId, &guid);
-      LOG(ERROR) << "Couldn't get path for provider: " << guid;
+      LOG(ERROR) << "Couldn't get path for provider: "
+                 << base::win::String16FromGUID(
+                        service_providers[i].ProviderId);
     }
   }
 }

chrome/chrome_cleaner/os/system_util.cc

@@ -72,14 +72,6 @@ bool GetMediumIntegrityToken(base::win::ScopedHandle* medium_integrity_token) {
   return true;
 }
 
-void GUIDToString(const GUID& guid, base::string16* output) {
-  DCHECK(output);
-  static const size_t kGUIDStringSize = 39;
-  int result = ::StringFromGUID2(guid, base::WriteInto(output, kGUIDStringSize),
-                                 kGUIDStringSize);
-  DCHECK(result == kGUIDStringSize);
-}
-
 void SetBackgroundMode() {
   // Get the process working set size and flags, so that we can reset them
   // after setting the process to background mode. For some reason Windows sets

chrome/chrome_cleaner/os/system_util.h

@@ -53,10 +53,6 @@ struct ServiceStatus {
 // running on a Windows version before Vista.
 bool GetMediumIntegrityToken(base::win::ScopedHandle* medium_integrity_token);
 
-// Convert a GUID to its textual representation. |output| receives the textual
-// representation.
-void GUIDToString(const GUID&, base::string16* output);
-
 // Set the current process to background mode.
 void SetBackgroundMode();
 

chrome/chrome_cleaner/os/system_util_unittest.cc

@@ -34,16 +34,4 @@ TEST(SystemUtilTests, GetMediumIntegrityToken) {
   EXPECT_LT(integrity_level, SECURITY_MANDATORY_HIGH_RID);
 }
 
-TEST(SystemUtilTests, GUIDToString) {
-  base::string16 provider1;
-  base::string16 provider2;
-  base::string16 provider3;
-  GUIDToString(kGUID1, &provider1);
-  GUIDToString(kGUID2, &provider2);
-  GUIDToString(kGUID3, &provider3);
-  EXPECT_STREQ(provider1.c_str(), kGUID1Str);
-  EXPECT_STREQ(provider2.c_str(), kGUID2Str);
-  EXPECT_STREQ(provider3.c_str(), kGUID3Str);
-}
-
 }  // namespace chrome_cleaner

chrome/credential_provider/gaiacp/gaia_credential_provider_module.cc

@@ -8,6 +8,8 @@
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/strings/string_util.h"
+#include "base/win/win_util.h"
 #include "base/win/windows_version.h"
 #include "chrome/common/chrome_version.h"
 #include "chrome/credential_provider/eventlog/gcp_eventlog_messages.h"
@@ -55,17 +57,15 @@ CGaiaCredentialProviderModule::UpdateRegistryAppId(BOOL do_register) throw() {
   eventlog_path =
       eventlog_path.Append(FILE_PATH_LITERAL("gcp_eventlog_provider.dll"));
 
-  wchar_t provider_guid_in_wchar[64];
-  StringFromGUID2(CLSID_GaiaCredentialProvider, provider_guid_in_wchar,
-                  base::size(provider_guid_in_wchar));
+  auto provider_guid_string =
+      base::win::String16FromGUID(CLSID_GaiaCredentialProvider);
 
-  wchar_t filter_guid_in_wchar[64];
-  StringFromGUID2(__uuidof(CGaiaCredentialProviderFilter), filter_guid_in_wchar,
-                  base::size(filter_guid_in_wchar));
+  auto filter_guid_string =
+      base::win::String16FromGUID(__uuidof(CGaiaCredentialProviderFilter));
 
   ATL::_ATL_REGMAP_ENTRY regmap[] = {
-      {L"CP_CLASS_GUID", provider_guid_in_wchar},
-      {L"CP_FILTER_CLASS_GUID", filter_guid_in_wchar},
+      {L"CP_CLASS_GUID", base::as_wcstr(provider_guid_string.c_str())},
+      {L"CP_FILTER_CLASS_GUID", base::as_wcstr(filter_guid_string.c_str())},
       {L"VERSION", TEXT(CHROME_VERSION_STRING)},
       {L"EVENTLOG_PATH", eventlog_path.value().c_str()},
       {nullptr, nullptr},

chrome/credential_provider/gaiacp/gaia_credential_provider_unittests.cc

@@ -11,6 +11,7 @@
 
 #include "base/synchronization/waitable_event.h"
 #include "base/win/registry.h"
+#include "base/win/win_util.h"
 #include "chrome/credential_provider/common/gcp_strings.h"
 #include "chrome/credential_provider/gaiacp/associated_user_validator.h"
 #include "chrome/credential_provider/gaiacp/auth_utils.h"
@@ -931,15 +932,14 @@ TEST_P(GcpCredentialProviderAvailableCredentialsTest, AvailableCredentials) {
 
     // In the case that a real CReauthCredential is created, we expect that this
     // credential will set the default credential provider for the user tile.
-    wchar_t guid_in_wchar[64];
-    ::StringFromGUID2(CLSID_GaiaCredentialProvider, guid_in_wchar,
-                      base::size(guid_in_wchar));
+    auto guid_string =
+        base::win::String16FromGUID(CLSID_GaiaCredentialProvider);
 
     wchar_t guid_in_registry[64];
     ULONG length = base::size(guid_in_registry);
     EXPECT_EQ(S_OK, GetMachineRegString(kLogonUiUserTileRegKey, sid,
                                         guid_in_registry, &length));
-    EXPECT_EQ(base::string16(guid_in_wchar), base::string16(guid_in_registry));
+    EXPECT_EQ(guid_string, base::string16(guid_in_registry));
     ::CoTaskMemFree(sid);
   }
 

chrome/credential_provider/gaiacp/reg_utils.cc

@@ -11,6 +11,7 @@
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/win/registry.h"
+#include "base/win/win_util.h"
 #include "chrome/credential_provider/common/gcp_strings.h"
 
 namespace credential_provider {
@@ -317,10 +318,8 @@ HRESULT SetUserWinlogonUserListEntry(const base::string16& username,
 }
 
 HRESULT SetLogonUiUserTileEntry(const base::string16& sid, CLSID cp_guid) {
-  wchar_t guid_in_wchar[64];
-  ::StringFromGUID2(cp_guid, guid_in_wchar, base::size(guid_in_wchar));
-  return SetMachineRegString(kLogonUiUserTileRegKey, sid.c_str(),
-                             guid_in_wchar);
+  return SetMachineRegString(kLogonUiUserTileRegKey, sid,
+                             base::win::String16FromGUID(cp_guid));
 }
 
 }  // namespace credential_provider

chrome/credential_provider/test/gcp_setup_unittests.cc

@@ -27,6 +27,7 @@
 #include "base/test/scoped_path_override.h"
 #include "base/test/test_reg_util_win.h"
 #include "base/win/registry.h"
+#include "base/win/win_util.h"
 #include "build/build_config.h"
 #include "chrome/credential_provider/common/gcp_strings.h"
 #include "chrome/credential_provider/gaiacp/gaia_credential_provider.h"
@@ -128,13 +129,11 @@ void GcpSetupTest::ExpectAllFilesToExist(
 void GcpSetupTest::ExpectCredentialProviderToBeRegistered(
     bool registered,
     const base::string16& product_version) {
-  wchar_t guid_in_wchar[64];
-  StringFromGUID2(CLSID_GaiaCredentialProvider, guid_in_wchar,
-                  base::size(guid_in_wchar));
+  auto guid_string = base::win::String16FromGUID(CLSID_GaiaCredentialProvider);
 
   // Make sure COM object is registered.
   base::string16 register_key_path =
-      base::StringPrintf(L"CLSID\\%ls\\InprocServer32", guid_in_wchar);
+      base::StringPrintf(L"CLSID\\%ls\\InprocServer32", guid_string.c_str());
   base::win::RegKey clsid_key(HKEY_CLASSES_ROOT, register_key_path.c_str(),
                               KEY_READ);
   EXPECT_EQ(registered, clsid_key.Valid());
@@ -149,7 +148,8 @@ void GcpSetupTest::ExpectCredentialProviderToBeRegistered(
 
   base::string16 cp_key_path = base::StringPrintf(
       L"SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\"
-      L"Authentication\\Credential Providers\\%ls", guid_in_wchar);
+      L"Authentication\\Credential Providers\\%ls",
+      guid_string.c_str());
 
   // Make sure credential provider is registered.
   base::win::RegKey cp_key(HKEY_LOCAL_MACHINE, cp_key_path.c_str(), KEY_READ);