[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