30_fix_broker_alive_mutex.patch (4748B)
1 # HG changeset patch 2 # User Yannis Juglaret <yjuglaret@mozilla.com> 3 # Date 1704300086 -3600 4 # Wed Jan 03 17:41:26 2024 +0100 5 # Node ID c08c4330fa141f5c7f8fa646ce179969e98cfd60 6 # Parent 4a7e58fdaac86befe272259c2f603e2229080a5b 7 Bug 1851889 - Create the broker alive mutex during sandbox initialization. r=bobowen 8 9 The sandbox IPC client/server communication protocol relies on a mutex 10 that clients can use to check if the broker process is still alive; e.g. 11 when a response takes more than one second to come. This mutex is owned 12 by a thread of the broker process and will be marked as abandoned when 13 that thread dies. 14 15 Clients assume that the broker alive mutex being abandoned means that 16 the whole broker process crashed. Therefore it is necessary that the 17 thread that owns the broker alive mutex lives as long as the whole 18 broker process, since clients cannot distinguish between the death of 19 this thread and the death of the whole broker process. 20 21 In upstream code, the broker alive mutex gets created during the first 22 call to SpawnTarget, which means that it is implicitly required that 23 this call occurs from a thread that lives as long as the broker process 24 will. Since we call SpawnTarget from the IPC launcher thread, which dies 25 during XPCOM shutdown, we are breaking this implicit requirement. 26 27 Therefore, this patch makes us create the broker alive mutex from the 28 main thread, during sandbox initialization. This ensures that clients 29 will not get disturbed by the death of the IPC launcher thread anymore. 30 31 Differential Revision: https://phabricator.services.mozilla.com/D197423 32 33 diff --git a/sandbox/win/src/broker_services.cc b/sandbox/win/src/broker_services.cc 34 --- a/sandbox/win/src/broker_services.cc 35 +++ b/sandbox/win/src/broker_services.cc 36 @@ -293,6 +293,9 @@ ResultCode BrokerServicesBase::Init() { 37 return SBOX_ERROR_CANNOT_INIT_BROKERSERVICES; 38 } 39 40 + if (!SharedMemIPCServer::CreateBrokerAliveMutex()) 41 + return SBOX_ERROR_CANNOT_INIT_BROKERSERVICES; 42 + 43 params.release(); 44 return SBOX_ALL_OK; 45 } 46 diff --git a/sandbox/win/src/sharedmem_ipc_server.cc b/sandbox/win/src/sharedmem_ipc_server.cc 47 --- a/sandbox/win/src/sharedmem_ipc_server.cc 48 +++ b/sandbox/win/src/sharedmem_ipc_server.cc 49 @@ -24,6 +24,18 @@ volatile HANDLE g_alive_mutex = nullptr; 50 51 namespace sandbox { 52 53 +/* static */ bool SharedMemIPCServer::CreateBrokerAliveMutex() { 54 + DCHECK(!g_alive_mutex); 55 + // We create a initially owned mutex. If the server dies unexpectedly, 56 + // the thread that owns it will fail to release the lock and windows will 57 + // report to the target (when it tries to acquire it) that the wait was 58 + // abandoned. Note: We purposely leak the local handle because we want it to 59 + // be closed by Windows itself so it is properly marked as abandoned if the 60 + // server dies. 61 + g_alive_mutex = ::CreateMutexW(nullptr, true, nullptr); 62 + return static_cast<bool>(g_alive_mutex); 63 +} 64 + 65 SharedMemIPCServer::ServerControl::ServerControl() {} 66 67 SharedMemIPCServer::ServerControl::~ServerControl() {} 68 @@ -37,19 +49,7 @@ SharedMemIPCServer::SharedMemIPCServer(H 69 target_process_(target_process), 70 target_process_id_(target_process_id), 71 call_dispatcher_(dispatcher) { 72 - // We create a initially owned mutex. If the server dies unexpectedly, 73 - // the thread that owns it will fail to release the lock and windows will 74 - // report to the target (when it tries to acquire it) that the wait was 75 - // abandoned. Note: We purposely leak the local handle because we want it to 76 - // be closed by Windows itself so it is properly marked as abandoned if the 77 - // server dies. 78 - if (!g_alive_mutex) { 79 - HANDLE mutex = ::CreateMutexW(nullptr, true, nullptr); 80 - if (::InterlockedCompareExchangePointer(&g_alive_mutex, mutex, nullptr)) { 81 - // We lost the race to create the mutex. 82 - ::CloseHandle(mutex); 83 - } 84 - } 85 + DCHECK(g_alive_mutex); 86 } 87 88 SharedMemIPCServer::~SharedMemIPCServer() { 89 diff --git a/sandbox/win/src/sharedmem_ipc_server.h b/sandbox/win/src/sharedmem_ipc_server.h 90 --- a/sandbox/win/src/sharedmem_ipc_server.h 91 +++ b/sandbox/win/src/sharedmem_ipc_server.h 92 @@ -60,6 +60,12 @@ class SharedMemIPCServer { 93 // creates the kernels events used to signal the IPC. 94 bool Init(void* shared_mem, uint32_t shared_size, uint32_t channel_size); 95 96 + // Create the mutex used by clients to check if the broker process crashed. 97 + // This function must be called only once, from a thread that will live as 98 + // long as the whole broker process will. This call must occur prior to any 99 + // SharedMemIPCServer creation. 100 + static bool CreateBrokerAliveMutex(); 101 + 102 private: 103 // Allow tests to be marked DISABLED_. Note that FLAKY_ and FAILS_ prefixes 104 // do not work with sandbox tests.