Ensure StrCat means StrCat

undefine StrCat from StrCatW after including windows headers that redefine it Add preprocessor checks StrCat is not defined to something else. Add presubmit check headers that include shlwapi.h that redefines StrCat are wrapped Bug: chromium:856536, chromium:817738 Change-Id: Ief9303cbf6fa4f55f671861e49fb1fc747ed59aa Reviewed-on: https://chromium-review.googlesource.com/1117180 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Reviewed-by: Bruce Dawson <brucedawson@chromium.org> Commit-Queue: Danil Chapovalov <danilchap@chromium.org> Cr-Commit-Position: refs/heads/master@{#582449}
marcosholgado · Aug 11, 2018 · 3518f36 · 3518f36
1 parent 856d335
commit 3518f36
Show file tree

Hide file tree

Showing 60 changed files with 234 additions and 100 deletions.
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
@@ -813,6 +813,28 @@ def _CheckNoIOStreamInHeaders(input_api, output_api):
         files) ]
   return []
 
+def _CheckNoStrCatRedefines(input_api, output_api):
+  """Checks no windows headers with StrCat redefined are included directly."""
+  files = []
+  pattern_deny = input_api.re.compile(
+      r'^#include\s*[<"](shlwapi|atlbase|propvarutil|sphelper).h[">]',
+      input_api.re.MULTILINE)
+  pattern_allow = input_api.re.compile(
+      r'^#include\s"base/win/windows_defines.inc"',
+      input_api.re.MULTILINE)
+  for f in input_api.AffectedSourceFiles(input_api.FilterSourceFile):
+    contents = input_api.ReadFile(f)
+    if pattern_deny.search(contents) and not pattern_allow.search(contents):
+      files.append(f.LocalPath())
+
+  if len(files):
+    return [output_api.PresubmitError(
+        'Do not #include shlwapi.h, atlbase.h, propvarutil.h or sphelper.h '
+        'directly since they pollute code with StrCat macro. Instead, '
+        'include matching header from base/win. See http://crbug.com/856536',
+        files) ]
+  return []
+
 
 def _CheckNoUNIT_TESTInSourceFiles(input_api, output_api):
   """Checks to make sure no source files use UNIT_TEST."""

diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py
@@ -1858,6 +1858,42 @@ def testFalsePositives(self):
     results = PRESUBMIT._CheckUniquePtr(mock_input_api, MockOutputApi())
     self.assertEqual(0, len(results))
 
+class CheckNoDirectIncludesHeadersWhichRedefineStrCat(unittest.TestCase):
+  def testBlocksDirectIncludes(self):
+    mock_input_api = MockInputApi()
+    mock_input_api.files = [
+      MockFile('dir/foo_win.cc', ['#include "shlwapi.h"']),
+      MockFile('dir/bar.h', ['#include <propvarutil.h>']),
+      MockFile('dir/baz.h', ['#include <atlbase.h>']),
+      MockFile('dir/jumbo.h', ['#include "sphelper.h"']),
+    ]
+    results = PRESUBMIT._CheckNoStrCatRedefines(mock_input_api, MockOutputApi())
+    self.assertEquals(1, len(results))
+    self.assertEquals(4, len(results[0].items))
+    self.assertTrue('StrCat' in results[0].message)
+    self.assertTrue('foo_win.cc' in results[0].items[0])
+    self.assertTrue('bar.h' in results[0].items[1])
+    self.assertTrue('baz.h' in results[0].items[2])
+    self.assertTrue('jumbo.h' in results[0].items[3])
+
+  def testAllowsToIncludeWrapper(self):
+    mock_input_api = MockInputApi()
+    mock_input_api.files = [
+      MockFile('dir/baz_win.cc', ['#include "base/win/shlwapi.h"']),
+      MockFile('dir/baz-win.h', ['#include "base/win/atl.h"']),
+    ]
+    results = PRESUBMIT._CheckNoStrCatRedefines(mock_input_api, MockOutputApi())
+    self.assertEquals(0, len(results))
+
+  def testAllowsToCreateWrapper(self):
+    mock_input_api = MockInputApi()
+    mock_input_api.files = [
+      MockFile('base/win/shlwapi.h', [
+        '#include <shlwapi.h>',
+        '#include "base/win/windows_defines.inc"']),
+    ]
+    results = PRESUBMIT._CheckNoStrCatRedefines(mock_input_api, MockOutputApi())
+    self.assertEquals(0, len(results))
 
 class TranslationScreenshotsTest(unittest.TestCase):
   # An empty grd file.

diff --git a/base/BUILD.gn b/base/BUILD.gn
@@ -1060,6 +1060,7 @@ jumbo_component("base") {
     "vlog.cc",
     "vlog.h",
     "win/async_operation.h",
+    "win/atl.h",
     "win/com_init_check_hook.cc",
     "win/com_init_check_hook.h",
     "win/com_init_util.cc",
@@ -1088,6 +1089,7 @@ jumbo_component("base") {
     "win/patch_util.h",
     "win/process_startup_helper.cc",
     "win/process_startup_helper.h",
+    "win/propvarutil.h",
     "win/reference.h",
     "win/registry.cc",
     "win/registry.h",
@@ -1116,8 +1118,10 @@ jumbo_component("base") {
     "win/scoped_windows_thread_environment.h",
     "win/scoped_winrt_initializer.cc",
     "win/scoped_winrt_initializer.h",
+    "win/shlwapi.h",
     "win/shortcut.cc",
     "win/shortcut.h",
+    "win/sphelper.h",
     "win/startup_information.cc",
     "win/startup_information.h",
     "win/typed_event_handler.h",
@@ -1127,6 +1131,9 @@ jumbo_component("base") {
     "win/wait_chain.h",
     "win/win_util.cc",
     "win/win_util.h",
+    "win/windows_defines.inc",
+    "win/windows_types.h",
+    "win/windows_undefines.inc",
     "win/windows_version.cc",
     "win/windows_version.h",
     "win/winrt_storage_util.cc",

diff --git a/base/files/file_enumerator_win.cc b/base/files/file_enumerator_win.cc
@@ -4,12 +4,12 @@
 
 #include "base/files/file_enumerator.h"
 
-#include <shlwapi.h>
 #include <stdint.h>
 #include <string.h>
 
 #include "base/logging.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/win/shlwapi.h"
 
 namespace base {
 

diff --git a/base/strings/strcat.h b/base/strings/strcat.h
@@ -14,8 +14,9 @@
 #include "build/build_config.h"
 
 #if defined(OS_WIN)
-// To resolve a conflict with Win32 API StrCat macro.
-#include "base/win/windows_types.h"
+// Guard against conflict with Win32 API StrCat macro:
+// check StrCat wasn't and will not be redefined.
+#define StrCat StrCat
 #endif
 
 namespace base {

diff --git a/base/test/test_file_util_win.cc b/base/test/test_file_util_win.cc
@@ -5,7 +5,6 @@
 #include "base/test/test_file_util.h"
 
 #include <aclapi.h>
-#include <shlwapi.h>
 #include <stddef.h>
 #include <wchar.h>
 #include <windows.h>
@@ -20,6 +19,7 @@
 #include "base/strings/string_split.h"
 #include "base/threading/platform_thread.h"
 #include "base/win/scoped_handle.h"
+#include "base/win/shlwapi.h"
 
 namespace base {
 

diff --git a/base/win/atl.h b/base/win/atl.h
@@ -0,0 +1,28 @@
+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+#ifndef BASE_WIN_ATL_H_
+#define BASE_WIN_ATL_H_
+
+// Check no prior poisonous defines were made.
+#include "base/win/windows_defines.inc"
+// Undefine before windows header will make the poisonous defines
+#include "base/win/windows_undefines.inc"
+
+#ifndef _ATL_NO_EXCEPTIONS
+#define _ATL_NO_EXCEPTIONS
+#endif
+
+#include <atlbase.h>
+#include <atlcom.h>
+#include <atlctl.h>
+#include <atlhost.h>
+#include <atlsecurity.h>
+#include <atlwin.h>
+
+// Undefine the poisonous defines
+#include "base/win/windows_undefines.inc"
+// Check no poisonous defines follow this include
+#include "base/win/windows_defines.inc"
+
+#endif  // BASE_WIN_ATL_H_
diff --git a/base/win/propvarutil.h b/base/win/propvarutil.h
@@ -0,0 +1,19 @@
+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+#ifndef BASE_WIN_PROPVARUTIL_H_
+#define BASE_WIN_PROPVARUTIL_H_
+
+// Check no prior poisonous defines were made.
+#include "base/win/windows_defines.inc"
+// Undefine before windows header will make the poisonous defines
+#include "base/win/windows_undefines.inc"
+
+#include <propvarutil.h>
+
+// Undefine the poisonous defines
+#include "base/win/windows_undefines.inc"
+// Check no poisonous defines follow this include
+#include "base/win/windows_defines.inc"
+
+#endif  // BASE_WIN_PROPVARUTIL_H_
diff --git a/base/win/registry.cc b/base/win/registry.cc
@@ -4,14 +4,14 @@
 
 #include "base/win/registry.h"
 
-#include <shlwapi.h>
 #include <stddef.h>
 #include <algorithm>
 
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/win/shlwapi.h"
 #include "base/win/windows_version.h"
 
 namespace base {

diff --git a/base/win/shlwapi.h b/base/win/shlwapi.h
@@ -0,0 +1,19 @@
+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+#ifndef BASE_WIN_SHLWAPI_H_
+#define BASE_WIN_SHLWAPI_H_
+
+// Check no prior poisonous defines were made.
+#include "base/win/windows_defines.inc"
+// Undefine before windows header will make the poisonous defines
+#include "base/win/windows_undefines.inc"
+
+#include <shlwapi.h>
+
+// Undefine the poisonous defines
+#include "base/win/windows_undefines.inc"
+// Check no poisonous defines follow this include
+#include "base/win/windows_defines.inc"
+
+#endif  // BASE_WIN_SHLWAPI_H_
diff --git a/base/win/sphelper.h b/base/win/sphelper.h
@@ -0,0 +1,19 @@
+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+#ifndef BASE_WIN_SPHELPER_H_
+#define BASE_WIN_SPHELPER_H_
+
+// Check no prior poisonous defines were made.
+#include "base/win/windows_defines.inc"
+// Undefine before windows header will make the poisonous defines
+#include "base/win/windows_undefines.inc"
+
+#include <sphelper.h>
+
+// Undefine the poisonous defines
+#include "base/win/windows_undefines.inc"
+// Check no poisonous defines follow this include
+#include "base/win/windows_defines.inc"
+
+#endif  // BASE_WIN_SPHELPER_H_
diff --git a/base/win/win_util.cc b/base/win/win_util.cc
@@ -14,13 +14,11 @@
 #include <mdmregistration.h>
 #include <objbase.h>
 #include <propkey.h>
-#include <propvarutil.h>
 #include <psapi.h>
 #include <roapi.h>
 #include <sddl.h>
 #include <setupapi.h>
 #include <shellscalingapi.h>
-#include <shlwapi.h>
 #include <signal.h>
 #include <stddef.h>
 #include <stdlib.h>
@@ -45,11 +43,13 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/win/core_winrt_util.h"
+#include "base/win/propvarutil.h"
 #include "base/win/registry.h"
 #include "base/win/scoped_co_mem.h"
 #include "base/win/scoped_handle.h"
 #include "base/win/scoped_hstring.h"
 #include "base/win/scoped_propvariant.h"
+#include "base/win/shlwapi.h"
 #include "base/win/win_client_metrics.h"
 #include "base/win/windows_version.h"
 

diff --git a/base/win/windows_defines.inc b/base/win/windows_defines.inc
@@ -0,0 +1,12 @@
+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// This file verifies no poisonous defines from windows headers are present.
+// If you add more macros here, add them to windows_undefines.inc too
+
+// This will generate an error if it was defined to something different, or
+// if it is defined to something different after.
+// Preprocessor will also nicely point to header that defined it differently.
+#define StrCat StrCat
+
diff --git a/base/win/windows_types.h b/base/win/windows_types.h
@@ -248,6 +248,5 @@ WINBASEAPI VOID WINAPI SetLastError(_In_ DWORD dwErrCode);
 #define SendMessageCallback SendMessageCallbackW
 #define SetCurrentDirectory SetCurrentDirectoryW
 #define StartService StartServiceW
-#define StrCat StrCatW
 
 #endif  // BASE_WIN_WINDOWS_TYPES_H
diff --git a/base/win/windows_undefines.inc b/base/win/windows_undefines.inc
@@ -0,0 +1,9 @@
+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// This file undefines poisonous defines from windows headers.
+// If you add more macros here, add them to windows_defines.inc too
+
+#undef StrCat
+
diff --git a/chrome/app/chrome_main_delegate.cc b/chrome/app/chrome_main_delegate.cc
@@ -65,12 +65,12 @@
 #include "ui/base/ui_base_switches.h"
 
 #if defined(OS_WIN)
-#include <atlbase.h>
 #include <malloc.h>
 
 #include <algorithm>
 
 #include "base/debug/close_handle_hook_win.h"
+#include "base/win/atl.h"
 #include "chrome/browser/downgrade/user_data_downgrade.h"
 #include "chrome/child/v8_breakpad_support_win.h"
 #include "chrome/common/child_process_logging.h"

diff --git a/chrome/app/main_dll_loader_win.cc b/chrome/app/main_dll_loader_win.cc
@@ -5,7 +5,6 @@
 #include "chrome/app/main_dll_loader_win.h"
 
 #include <windows.h>  // NOLINT
-#include <shlwapi.h>  // NOLINT
 #include <stddef.h>
 #include <stdint.h>
 #include <userenv.h>  // NOLINT
@@ -28,6 +27,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/trace_event/trace_event.h"
 #include "base/win/scoped_handle.h"
+#include "base/win/shlwapi.h"
 #include "base/win/windows_version.h"
 #include "chrome/app/chrome_watcher_client_win.h"
 #include "chrome/app/chrome_watcher_command_line_win.h"

diff --git a/chrome/browser/conflicts/uninstall_application_win.cc b/chrome/browser/conflicts/uninstall_application_win.cc
@@ -4,7 +4,6 @@
 
 #include "chrome/browser/conflicts/uninstall_application_win.h"
 
-#include <atlbase.h>
 #include <wrl/client.h>
 
 #include <memory>
@@ -22,6 +21,7 @@
 #include "base/strings/string_util.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/sequenced_task_runner_handle.h"
+#include "base/win/atl.h"
 #include "base/win/scoped_variant.h"
 #include "chrome/browser/win/automation_controller.h"
 #include "chrome/browser/win/ui_automation_util.h"

diff --git a/chrome/browser/google/google_update_win.cc b/chrome/browser/google/google_update_win.cc
@@ -4,8 +4,6 @@
 
 #include "chrome/browser/google/google_update_win.h"
 
-#include <atlbase.h>
-#include <atlcom.h>
 #include <objbase.h>
 #include <stdint.h>
 #include <string.h>
@@ -33,6 +31,7 @@
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/version.h"
+#include "base/win/atl.h"
 #include "base/win/scoped_bstr.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/generated_resources.h"

diff --git a/chrome/browser/google/google_update_win_unittest.cc b/chrome/browser/google/google_update_win_unittest.cc
@@ -5,8 +5,7 @@
 #include "chrome/browser/google/google_update_win.h"
 
 #include <windows.h>
-#include <atlbase.h>
-#include <atlcom.h>
+
 #include <wrl/client.h>
 
 #include <memory>
@@ -24,6 +23,7 @@
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/version.h"
+#include "base/win/atl.h"
 #include "base/win/registry.h"
 #include "chrome/common/chrome_version.h"
 #include "chrome/install_static/test/scoped_install_details.h"