Use the lto_visibility_public tag on the BrokerServices class

This is necessary because the BrokerServices class is used across
module boundaries and without this the linker (with ThinLTO enabled)
could devirtualize the BrokerServices functions and cause the sandbox
implementation code to be called from chrome.dll instead of chrome.exe.

This also adds a DCHECK in BrokerServicesBase::SpawnTarget to catch this
issue in DCHECK builds.

See crbug.com/1177001#c22 for all the details


Bug: 1177001
Change-Id: I50968274990051d2ff9edd71e4d9ccdd0d5e541f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3010473
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#899832}

sandbox/win/src/broker_services.cc

@@ -29,6 +29,10 @@
 #include "sandbox/win/src/threadpool.h"
 #include "sandbox/win/src/win_utils.h"
 
+#if DCHECK_IS_ON()
+#include "base/win/current_module.h"
+#endif
+
 namespace {
 
 // Utility function to associate a completion port to a job object.
@@ -437,6 +441,14 @@ ResultCode BrokerServicesBase::SpawnTarget(const wchar_t* exe_path,
                                            ResultCode* last_warning,
                                            DWORD* last_error,
                                            PROCESS_INFORMATION* target_info) {
+#if DCHECK_IS_ON()
+  // This code should only be called from the exe, ensure that this is always
+  // the case.
+  HMODULE exe_module = nullptr;
+  CHECK(::GetModuleHandleEx(NULL, exe_path, &exe_module));
+  DCHECK_EQ(CURRENT_MODULE(), exe_module);
+#endif
+
   if (!exe_path)
     return SBOX_ERROR_BAD_PARAMS;
 

sandbox/win/src/sandbox.h

@@ -55,7 +55,10 @@ class TargetServices;
 //  // -- later you can call:
 //  broker->WaitForAllTargets(option);
 //
-class BrokerServices {
+// We need [[clang::lto_visibility_public]] because instances of this class are
+// passed across module boundaries. This means different modules must have
+// compatible definitions of the class even when LTO is enabled.
+class [[clang::lto_visibility_public]] BrokerServices {
  public:
   // Initializes the broker. Must be called before any other on this class.
   // returns ALL_OK if successful. All other return values imply failure.