[Webkit-unassigned] [Bug 191360] [GTK][WPE] Bubblewrap launcher should not depend on memfd

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=191360

--- Comment #2 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 354090
  --> https://bugs.webkit.org/attachment.cgi?id=354090
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354090&action=review

> Source/WebCore/ChangeLog:3
> +        [GTK][WPE] Bubblewrap launcher should not depend on memfd

It's a little unclear to me why this is desirable given that kernel 3.17 is positively ancient now, but OK.

> Source/WebCore/platform/glib/FileSystemGlib.cpp:42
> +#if HAVE(LINUX_MEMFD_H)

Here's what really confuses me. A grep of the entire codebase shows no other uses of LINUX_MEMFD_H. And your patch doesn't appear to add it. So how does this check work? Is it always false?

It's not the ideal check, either. The original code is designed to use memfd even if memfd.h doesn't exist, as long as the kernel is new enough. I guess it's perfectly fine to change this, since we have fallback in place, but it's not clear to me that the change here was intentional.

> Source/WebCore/platform/glib/FileSystemGlib.cpp:43
> +#include <linux/memfd.h>

Looks like this is unused? You only use sycall() directly and don't seem to use anything declared in this header.

> Source/WebCore/platform/glib/FileSystemGlib.cpp:47
> +
> +

Too much space here

> Source/WebCore/platform/glib/FileSystemGlib.cpp:477
> +            fileDescriptor = syscall(__NR_memfd_create, name, 0);

Since you're inside an #if HAVE(LINUX_MEMFD_H) guard, you know linux/memfd.h is available, and can use it instead of syscall(). We were using syscall() to ensure this works when memfd.h is not available.

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:-94
> -    if (fcntl(fd, F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE | F_SEAL_SEAL) == -1) {
> -        g_warning("Failed to seal memfd: %s", g_strerror(errno));
> -        close(fd);
> -        return -1;
> -    }

Well we lost the fd sealing code. I'm not sure how important this is, or why it's there in the first place. Are other processes somehow able to modify the fd and mess with the args passed to bwap without the sealing? Patrick, can you comment on this?

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:47
> -static int
> -argsToFd(const Vector<CString>& args, const char *name)
> +static RefPtr<SharedMemory> argsToFd(const Vector<CString>& args, const char *name)

Oops. Also fix: const char* name

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:708
> +    auto flatpakInfoMemory = createFlatpakInfo();
> +    std::optional<int> flatpakInfoFd;
> +    if (flatpakInfoMemory) {

How about: if (auto flatpakInfoMemory = createFlatpakInfo()) {

-- 
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/20181108/69eab2bf/attachment.html>