From 9d195cb5d885d32433a7d79547c6a70c16c0959f Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Sun, 9 Oct 2022 06:41:53 -0700 Subject: [PATCH] REGRESSION(254232@main): Causes process launching to use fork + exec instead of posix_spawn https://bugs.webkit.org/show_bug.cgi?id=245784 Reviewed by Carlos Garcia Campos. 254232@main removed use of G_SUBPROCESS_FLAGS_INHERIT_FDS since we actually do not ever want to inherit fds into our child processes, except for those that we explicitly pass along using g_subprocess_launcher_take_fd(). This was a good change since it makes WebKit more robust to file descriptor leaks, but there is an unfortunate side effect: it prevents gspawn from using posix_spawn() to launch subprocesses, so we wind up falling back to fork() and exec() instead. I had left an insufficient warning comment about this, but it only mentioned one of the many things that could cause us to fall back to fork()/exec(). So: * Revert 254232@main * Avoid reintroducing bug #221489 by setting CLOEXEC on the client end of the IPC socket * Improve the warning comments to indicate there is more than one way to cause us to fall back to fork()/exec() * Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp: (WebKit::connectionOptions): (WebKit::ProcessLauncher::launchProcess): * Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.cpp: (WebKit::XDGDBusProxy::launch): Canonical link: https://commits.webkit.org/255325@main --- .../Launcher/glib/ProcessLauncherGLib.cpp | 40 +++++++++++++------ .../UIProcess/Launcher/glib/XDGDBusProxy.cpp | 14 +++++-- 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp b/Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp index 9d5b4a3201b6..6e4d2dd6b14b 100644 --- a/Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp +++ b/Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp @@ -73,9 +73,25 @@ static bool isFlatpakSpawnUsable() } #endif +static int connectionOptions() +{ +#if USE(LIBWPE) && !ENABLE(BUBBLEWRAP_SANDBOX) + // When using the WPE process launcher API, we cannot use CLOEXEC for the client socket because + // we need to leak it to the child process. + if (ProcessProviderLibWPE::singleton().isEnabled()) + return IPC::PlatformConnectionOptions::SetCloexecOnServer; +#endif + + // We use CLOEXEC for the client socket here even though we need to leak it to the child, + // because we don't want it leaking to xdg-dbus-proxy. If the IPC socket is unexpectedly open in + // an extra subprocess, WebKit won't notice when its child process crashes. We can ensure it + // gets leaked into only the correct subprocess by using g_subprocess_launcher_take_fd() later. + return IPC::PlatformConnectionOptions::SetCloexecOnClient | IPC::PlatformConnectionOptions::SetCloexecOnServer; +} + void ProcessLauncher::launchProcess() { - IPC::SocketPair socketPair = IPC::createPlatformConnection(IPC::PlatformConnectionOptions::SetCloexecOnServer); + IPC::SocketPair socketPair = IPC::createPlatformConnection(connectionOptions()); GUniquePtr processIdentifier(g_strdup_printf("%" PRIu64, m_launchOptions.processIdentifier.toUInt64())); GUniquePtr webkitSocket(g_strdup_printf("%d", socketPair.client)); @@ -93,10 +109,6 @@ void ProcessLauncher::launchProcess() if (m_processIdentifier <= -1) g_error("Unable to spawn a new child process"); - // Don't expose the parent socket to potential future children. - if (!setCloseOnExec(socketPair.client)) - RELEASE_ASSERT_NOT_REACHED(); - // We've finished launching the process, message back to the main run loop. RunLoop::main().dispatch([protectedThis = Ref { *this }, this, serverSocket = socketPair.server] { didFinishLaunchingProcess(m_processIdentifier, IPC::Connection::Identifier { serverSocket }); @@ -159,10 +171,16 @@ void ProcessLauncher::launchProcess() #endif argv[i++] = nullptr; - // Warning: do not set a child setup function, because we want GIO to be able to spawn with - // posix_spawn() rather than fork()/exec(), in order to better accommodate applications that use - // a huge amount of memory or address space in the UI process, like Eclipse. - GRefPtr launcher = adoptGRef(g_subprocess_launcher_new(G_SUBPROCESS_FLAGS_NONE)); + // Warning: we want GIO to be able to spawn with posix_spawn() rather than fork()/exec(), in + // order to better accommodate applications that use a huge amount of memory or address space + // in the UI process, like Eclipse. This means we must use GSubprocess in a manner that follows + // the rules documented in g_spawn_async_with_pipes_and_fds() for choosing between posix_spawn() + // (optimized/ideal codepath) vs. fork()/exec() (fallback codepath). As of GLib 2.74, the rules + // relevant to GSubprocess are (a) must inherit fds, (b) must not search path from envp, and (c) + // must not use a child setup fuction. + // + // Please keep this comment in sync with the duplicate comment in XDGDBusProxy::launch. + GRefPtr launcher = adoptGRef(g_subprocess_launcher_new(G_SUBPROCESS_FLAGS_INHERIT_FDS)); g_subprocess_launcher_take_fd(launcher.get(), socketPair.client, socketPair.client); GUniqueOutPtr error; @@ -202,10 +220,6 @@ void ProcessLauncher::launchProcess() m_processIdentifier = g_ascii_strtoll(processIdStr, nullptr, 0); RELEASE_ASSERT(m_processIdentifier); - // Don't expose the parent socket to potential future children. - if (!setCloseOnExec(socketPair.client)) - RELEASE_ASSERT_NOT_REACHED(); - // We've finished launching the process, message back to the main run loop. RunLoop::main().dispatch([protectedThis = Ref { *this }, this, serverSocket = socketPair.server] { didFinishLaunchingProcess(m_processIdentifier, IPC::Connection::Identifier { serverSocket }); diff --git a/Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.cpp b/Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.cpp index 376862827cd0..bd9a9c486f52 100644 --- a/Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.cpp +++ b/Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.cpp @@ -170,10 +170,16 @@ bool XDGDBusProxy::launch() argv[i++] = const_cast(arg.data()); argv[i] = nullptr; - // Warning: do not set a child setup function, because we want GIO to be able to spawn with - // posix_spawn() rather than fork()/exec(), in order to better accomodate applications that use - // a huge amount of memory or address space in the UI process, like Eclipse. - GRefPtr launcher = adoptGRef(g_subprocess_launcher_new(G_SUBPROCESS_FLAGS_NONE)); + // Warning: we want GIO to be able to spawn with posix_spawn() rather than fork()/exec(), in + // order to better accommodate applications that use a huge amount of memory or address space + // in the UI process, like Eclipse. This means we must use GSubprocess in a manner that follows + // the rules documented in g_spawn_async_with_pipes_and_fds() for choosing between posix_spawn() + // (optimized/ideal codepath) vs. fork()/exec() (fallback codepath). As of GLib 2.74, the rules + // relevant to GSubprocess are (a) must inherit fds, (b) must not search path from envp, and (c) + // must not use a child setup fuction. + // + // Please keep this comment in sync with the duplicate comment in ProcessLauncher::launchProcess. + GRefPtr launcher = adoptGRef(g_subprocess_launcher_new(G_SUBPROCESS_FLAGS_INHERIT_FDS)); g_subprocess_launcher_take_fd(launcher.get(), proxyFd, proxyFd); g_subprocess_launcher_take_fd(launcher.get(), syncFds[1], syncFds[1]); m_syncFD = UnixFileDescriptor(syncFds[0], UnixFileDescriptor::Adopt);