[Webkit-unassigned] [Bug 188568] [GTK][WPE] Implement subprocess sandboxing
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Sep 18 04:54:06 PDT 2018
https://bugs.webkit.org/show_bug.cgi?id=188568
--- Comment #49 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 349888
--> https://bugs.webkit.org/attachment.cgi?id=349888
[GTK][WPE] Implement subprocess sandboxing
View in context: https://bugs.webkit.org/attachment.cgi?id=349888&action=review
> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1158
> + * This method **must be called before any web process has been created**,
> + * as early as possible in your application. Calling it later is a fatal error.
Maybe we could make it a construct only property then, because it doesn't make sense to change it either.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:31
> +#include "PlatformDisplay.h"
WebCore headers should be included as <WebCore/Header.h> from WebKit layer.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:66
> + return syscall(__NR_memfd_create, name, flags);
Is this always available? doesn't it depend on a kernel version or libc?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:139
> + int sync_fds[2] = { -1 };
sync_fds -> syncFds
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:197
> + if (!g_spawn_async(NULL, argv, NULL, G_SPAWN_LEAVE_DESCRIPTORS_OPEN, NULL, NULL, &pid, &error.outPtr()))
Use nullptr instead of NULL.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:220
> + char* proxySocketTemplate = g_build_filename(appRunDir, "dbus-proxy-XXXXXX", nullptr);
Use GUniquePtr here, and you don't need to free in case of early return
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:223
> + g_free(proxySocketTemplate);
Like this one.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:293
> + const char* sessionAddress = g_getenv("DBUS_SESSION_BUS_ADDRESS");
> + if (!sessionAddress)
> + return;
> +
> + GUniquePtr<char> sessionPath = dbusAddressToPath(sessionAddress);
> + if (!sessionPath.get())
> + return;
This is going to be the same for any received proxy. I think it would be simpler if DBusProxy class received only the address and the type and called dbusAddressToPath itself. DBusProxy::setAddress() instead of setSocket.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:296
> + proxy.setSocket(sessionAddress, sessionPath.get());
> + proxy.launch();
These always go together, so maybe launch could simply receive the address and type instead of set + launch.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:301
> + args.appendVector(Vector<CString>({
> + "--bind", proxy.proxyPath(), proxy.path(),
> + }));
So, I would leave this function to only add the argument, like all other bind methods, and the caller, who created the proxy can call launch().
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:456
> + GUniquePtr<char> a11yPath = dbusAddressToPath(a11yAddress.get(), DBusAddressType::Abstract);
> + if (!a11yAddress.get())
> + return;
I would do the same here, I would move the code to get the address to its own function.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:668
> +static std::tuple<Vector<CString>, std::optional<int>, std::optional<int>> getBwrapArgs(ProcessLauncher::ProcessType processType, const Vector<String>& extraPaths)
We don't use get unless return values are out parameters. Also avoid using abbreviations in function and variables names in general.
> Source/cmake/OptionsGTK.cmake:215
> + find_program(BWRAP_EXECUTABLE bwrap)
> + if (NOT BWRAP_EXECUTABLE)
> + message(FATAL_ERROR "bwrap executable is needed for ENABLE_BUBBLEWRAP_SANDBOX")
> + endif ()
> + add_definitions(-DBWRAP_EXECUTABLE="${BWRAP_EXECUTABLE}")
hmm, this executable is not actually a build dep, but a runtime one, no? Wouldn't it be possible to not have the executable at build time, for example when cross-compiling?
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180918/94a49ee3/attachment.html>
More information about the webkit-unassigned
mailing list