GNU bug report logs - #54135
[PATCH] gnu: webkitgtk: Adjust BubbleWrap wrapper.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 54135 in the body.
You can then email your comments to 54135 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: guix-patches <at> gnu.org
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH] gnu: webkitgtk: Adjust BubbleWrap wrapper.
Date: Wed, 23 Feb 2022 21:29:11 -0500

This revisits <https://issues.guix.gnu.org/40837> with a fix that doesn't
require to have PULSE_CLIENTCONFIG point to an absolute store location, which
will allow us to revert to have PULSE_CLIENTCONFIG point to a fixed location
under /etc.  This would alleviate the need to reboot to have changes to the
PulseAudio configuration effected.

* gnu/packages/patches/webkitgtk-share-store.patch: Delete file.
* gnu/packages/patches/webkitgtk-bubblewrap-paths.patch: Add file.
* gnu/packages/patches/webkitgtk-canonicalize-paths.patch: Likewise.
* gnu/local.mk (dist_patch_DATA): Update patches list.
* gnu/packages/webkit.scm (webkitgtk)[patches]: Adjust accordingly.
---
 gnu/local.mk                                  |  3 +-
 .../webkitgtk-adjust-bubblewrap-paths.patch   | 38 +++++++++++
 .../patches/webkitgtk-bind-all-fonts.patch    | 17 +++--
 .../webkitgtk-canonicalize-paths.patch        | 66 +++++++++++++++++++
 .../patches/webkitgtk-share-store.patch       | 19 ------
 gnu/packages/webkit.scm                       |  7 +-
 6 files changed, 118 insertions(+), 32 deletions(-)
 create mode 100644 gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
 create mode 100644 gnu/packages/patches/webkitgtk-canonicalize-paths.patch
 delete mode 100644 gnu/packages/patches/webkitgtk-share-store.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index dcee1611b2..c4869f538c 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -1925,8 +1925,9 @@ dist_patch_DATA =						\
   %D%/packages/patches/vte-CVE-2012-2738-pt2.patch			\
   %D%/packages/patches/vtk-fix-freetypetools-build-failure.patch	\
   %D%/packages/patches/warsow-qfusion-fix-bool-return-type.patch	\
-  %D%/packages/patches/webkitgtk-share-store.patch		\
   %D%/packages/patches/webkitgtk-bind-all-fonts.patch		\
+  %D%/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch	\
+  %D%/packages/patches/webkitgtk-canonicalize-paths.patch	\
   %D%/packages/patches/webrtc-audio-processing-big-endian.patch	\
   %D%/packages/patches/websocketpp-fix-for-cmake-3.15.patch	\
   %D%/packages/patches/wicd-bitrate-none-fix.patch		\
diff --git a/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch b/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
new file mode 100644
index 0000000000..18ddb645ad
--- /dev/null
+++ b/gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
@@ -0,0 +1,38 @@
+Share /gnu/store in the BubbleWrap container and remove FHS mounts.
+
+This is a Guix-specific patch not meant to be upstreamed.
+diff --git a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
+index f0a5e4b05dff..88b11f806968 100644
+--- a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
++++ b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
+@@ -854,27 +854,12 @@ GRefPtr<GSubprocess> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
+         "--ro-bind", "/sys/dev", "/sys/dev",
+         "--ro-bind", "/sys/devices", "/sys/devices",
+ 
+-        "--ro-bind-try", "/usr/share", "/usr/share",
+-        "--ro-bind-try", "/usr/local/share", "/usr/local/share",
+         "--ro-bind-try", DATADIR, DATADIR,
+-
+-        // We only grant access to the libdirs webkit is built with and
+-        // guess system libdirs. This will always have some edge cases.
+-        "--ro-bind-try", "/lib", "/lib",
+-        "--ro-bind-try", "/usr/lib", "/usr/lib",
+-        "--ro-bind-try", "/usr/local/lib", "/usr/local/lib",
+         "--ro-bind-try", LIBDIR, LIBDIR,
+-#if CPU(ADDRESS64)
+-        "--ro-bind-try", "/lib64", "/lib64",
+-        "--ro-bind-try", "/usr/lib64", "/usr/lib64",
+-        "--ro-bind-try", "/usr/local/lib64", "/usr/local/lib64",
+-#else
+-        "--ro-bind-try", "/lib32", "/lib32",
+-        "--ro-bind-try", "/usr/lib32", "/usr/lib32",
+-        "--ro-bind-try", "/usr/local/lib32", "/usr/local/lib32",
+-#endif
+-
+         "--ro-bind-try", PKGLIBEXECDIR, PKGLIBEXECDIR,
++
++        // Bind mount the store inside the WebKitGTK sandbox.
++        "--ro-bind", "@storedir@", "@storedir@",
+     };
+ 
+     if (launchOptions.processType == ProcessLauncher::ProcessType::DBusProxy) {
diff --git a/gnu/packages/patches/webkitgtk-bind-all-fonts.patch b/gnu/packages/patches/webkitgtk-bind-all-fonts.patch
index e7b06cc650..27013180c4 100644
--- a/gnu/packages/patches/webkitgtk-bind-all-fonts.patch
+++ b/gnu/packages/patches/webkitgtk-bind-all-fonts.patch
@@ -1,26 +1,25 @@
-Add fonts from all XDG_DATA_DIRS, not just XDG_DATA_HOME.
+Upstream commit: https://github.com/WebKit/WebKit/commit/31ac354cbeecf866f9a38f7b2f8f59f7975d3f6a
 
-See <http://bugs.gnu.org/41174>.
-Author: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
-Index: webkitgtk-2.28.2/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
-===================================================================
+diff --git a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
+index ecc804663784..8de174be3c0e 100644
 --- a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
 +++ b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
-@@ -387,6 +387,7 @@ static void bindFonts(Vector<CString>& args)
+@@ -288,6 +288,7 @@ static void bindFonts(Vector<CString>& args)
      const char* homeDir = g_get_home_dir();
      const char* dataDir = g_get_user_data_dir();
      const char* cacheDir = g_get_user_cache_dir();
 +    const char* const * dataDirs = g_get_system_data_dirs();
-
+ 
      // Configs can include custom dirs but then we have to parse them...
      GUniquePtr<char> fontConfig(g_build_filename(configDir, "fontconfig", nullptr));
-@@ -403,6 +404,10 @@ static void bindFonts(Vector<CString>& args)
+@@ -304,6 +305,10 @@ static void bindFonts(Vector<CString>& args)
      bindIfExists(args, fontHomeConfigDir.get());
      bindIfExists(args, fontData.get());
      bindIfExists(args, fontHomeData.get());
-+    for (auto dataDir = dataDirs; dataDir != nullptr && *dataDir != nullptr; dataDir++) {
++    for (auto* dataDir = dataDirs; dataDir && *dataDir; dataDir++) {
 +        GUniquePtr<char> fontDataDir(g_build_filename(*dataDir, "fonts", nullptr));
 +        bindIfExists(args, fontDataDir.get());
 +    }
      bindIfExists(args, "/var/cache/fontconfig"); // Used by Debian.
  }
+ 
diff --git a/gnu/packages/patches/webkitgtk-canonicalize-paths.patch b/gnu/packages/patches/webkitgtk-canonicalize-paths.patch
new file mode 100644
index 0000000000..741d534831
--- /dev/null
+++ b/gnu/packages/patches/webkitgtk-canonicalize-paths.patch
@@ -0,0 +1,66 @@
+Upstream commit: https://github.com/WebKit/WebKit/commit/6a87eb254ef57a986a1a6ce9a3a4b66928afeb65
+
+diff --git a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
+index ecc804663784..a2a1c9d7a4dd 100644
+--- a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
++++ b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
+@@ -27,7 +27,6 @@
+ #include <seccomp.h>
+ #include <sys/ioctl.h>
+ #include <sys/mman.h>
+-#include <unistd.h>
+ #include <wtf/FileSystem.h>
+ #include <wtf/UniStdExtras.h>
+ #include <wtf/glib/GRefPtr.h>
+@@ -165,6 +164,15 @@ enum class BindFlags {
+     Device,
+ };
+ 
++static void bindSymlinksRealPath(Vector<CString>& args, const char* path, const char* bindOption = "--ro-bind")
++{
++    WTF::String realPath = FileSystem::realPath(path);
++    if (path != realPath) {
++        CString rpath = realPath.utf8();
++        args.appendVector(Vector<CString>({ bindOption, rpath.data(), rpath.data() }));
++    }
++}
++
+ static void bindIfExists(Vector<CString>& args, const char* path, BindFlags bindFlags = BindFlags::ReadOnly)
+ {
+     if (!path || path[0] == '\0')
+@@ -177,7 +185,16 @@ static void bindIfExists(Vector<CString>& args, const char* path, BindFlags bind
+         bindType = "--ro-bind-try";
+     else
+         bindType = "--bind-try";
+-    args.appendVector(Vector<CString>({ bindType, path, path }));
++
++    // Canonicalize the source path, otherwise a symbolic link could
++    // point to a location outside of the namespace.
++    bindSymlinksRealPath(args, path, bindType);
++
++    // As /etc is exposed wholesale, do not layer extraneous bind
++    // directives on top, which could fail in the presence of symbolic
++    // links.
++    if (!g_str_has_prefix(path, "/etc/"))
++        args.appendVector(Vector<CString>({ bindType, path, path }));
+ }
+ 
+ static void bindDBusSession(Vector<CString>& args, bool allowPortals)
+@@ -410,17 +427,6 @@ static void bindV4l(Vector<CString>& args)
+     }));
+ }
+ 
+-static void bindSymlinksRealPath(Vector<CString>& args, const char* path)
+-{
+-    char realPath[PATH_MAX];
+-
+-    if (realpath(path, realPath) && strcmp(path, realPath)) {
+-        args.appendVector(Vector<CString>({
+-            "--ro-bind", realPath, realPath,
+-        }));
+-    }
+-}
+-
+ // Translate a libseccomp error code into an error message. libseccomp
+ // mostly returns negative errno values such as -ENOMEM, but some
+ // standard errno values are used for non-standard purposes where their
diff --git a/gnu/packages/patches/webkitgtk-share-store.patch b/gnu/packages/patches/webkitgtk-share-store.patch
deleted file mode 100644
index 053d86fcf4..0000000000
--- a/gnu/packages/patches/webkitgtk-share-store.patch
+++ /dev/null
@@ -1,19 +0,0 @@
-Tell bubblewrap to share the store.  Required for programs that use the
-sandboxing features such as Epiphany.
-
-See <https://bugs.gnu.org/40837>.
-Author: Jack Hill <jackhill <at> jackhill.us>
----
-diff --git a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
---- a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
-+++ b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
-@@ -737,6 +737,9 @@ GRefPtr<GSubprocess> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
-         "--ro-bind-try", "/usr/local/share", "/usr/local/share",
-         "--ro-bind-try", DATADIR, DATADIR,
- 
-+       // Bind mount the store inside the WebKitGTK sandbox.
-+       "--ro-bind", "@storedir@", "@storedir@",
-+
-         // We only grant access to the libdirs webkit is built with and
-         // guess system libdirs. This will always have some edge cases.
-         "--ro-bind-try", "/lib", "/lib",
diff --git a/gnu/packages/webkit.scm b/gnu/packages/webkit.scm
index 40537f5e0a..72f673b0ca 100644
--- a/gnu/packages/webkit.scm
+++ b/gnu/packages/webkit.scm
@@ -6,7 +6,7 @@
 ;;; Copyright © 2018–2021 Tobias Geerinckx-Rice <me <at> tobias.gr>
 ;;; Copyright © 2018 Pierre Neidhardt <mail <at> ambrevar.xyz>
 ;;; Copyright © 2019 Marius Bakke <mbakke <at> fastmail.com>
-;;; Copyright © 2021 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
+;;; Copyright © 2021, 2022 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -247,8 +247,9 @@ (define-public webkitgtk
               (sha256
                (base32
                 "1xn1hhd0qaxmjf6vy6664i4mmmjsw9zgrr4w8ni3415d981zvj3b"))
-              (patches (search-patches "webkitgtk-share-store.patch"
-                                       "webkitgtk-bind-all-fonts.patch"))))
+              (patches (search-patches "webkitgtk-bind-all-fonts.patch"
+                                       "webkitgtk-adjust-bubblewrap-paths.patch"
+                                       "webkitgtk-canonicalize-paths.patch"))))
     (build-system cmake-build-system)
     (outputs '("out" "doc" "debug"))
     (arguments
-- 
2.34.0

Message #8 received at 54135 <at> debbugs.gnu.org (full text, mbox):

From: Jack Hill <jackhill <at> jackhill.us>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 54135 <at> debbugs.gnu.org
Subject: Re: [bug#54135] [PATCH] gnu: webkitgtk: Adjust BubbleWrap wrapper.
Date: Wed, 23 Feb 2022 21:46:05 -0500 (EST)

On Wed, 23 Feb 2022, Maxim Cournoyer wrote:

> This revisits <https://issues.guix.gnu.org/40837> with a fix that doesn't
> require to have PULSE_CLIENTCONFIG point to an absolute store location, which
> will allow us to revert to have PULSE_CLIENTCONFIG point to a fixed location
> under /etc.  This would alleviate the need to reboot to have changes to the
> PulseAudio configuration effected.
>
> * gnu/packages/patches/webkitgtk-share-store.patch: Delete file.
> * gnu/packages/patches/webkitgtk-bubblewrap-paths.patch: Add file.
> * gnu/packages/patches/webkitgtk-canonicalize-paths.patch: Likewise.
> * gnu/local.mk (dist_patch_DATA): Update patches list.
> * gnu/packages/webkit.scm (webkitgtk)[patches]: Adjust accordingly.
> ---
> gnu/local.mk                                  |  3 +-
> .../webkitgtk-adjust-bubblewrap-paths.patch   | 38 +++++++++++
> .../patches/webkitgtk-bind-all-fonts.patch    | 17 +++--
> .../webkitgtk-canonicalize-paths.patch        | 66 +++++++++++++++++++
> .../patches/webkitgtk-share-store.patch       | 19 ------
> gnu/packages/webkit.scm                       |  7 +-
> 6 files changed, 118 insertions(+), 32 deletions(-)
> create mode 100644 gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
> create mode 100644 gnu/packages/patches/webkitgtk-canonicalize-paths.patch
> delete mode 100644 gnu/packages/patches/webkitgtk-share-store.patch

LGTM. Thanks for taking up that WebKit issue and getting it fixed 
upstream!

Best,
Jack

Message #15 received at 54135-done <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Jack Hill <jackhill <at> jackhill.us>
Cc: 54135-done <at> debbugs.gnu.org
Subject: Re: [bug#54135] [PATCH] gnu: webkitgtk: Adjust BubbleWrap wrapper.
Date: Thu, 24 Feb 2022 09:25:48 -0500

Hi Jack,

Jack Hill <jackhill <at> jackhill.us> writes:

> On Wed, 23 Feb 2022, Maxim Cournoyer wrote:
>
>> This revisits <https://issues.guix.gnu.org/40837> with a fix that doesn't
>> require to have PULSE_CLIENTCONFIG point to an absolute store location, which
>> will allow us to revert to have PULSE_CLIENTCONFIG point to a fixed location
>> under /etc.  This would alleviate the need to reboot to have changes to the
>> PulseAudio configuration effected.
>>
>> * gnu/packages/patches/webkitgtk-share-store.patch: Delete file.
>> * gnu/packages/patches/webkitgtk-bubblewrap-paths.patch: Add file.
>> * gnu/packages/patches/webkitgtk-canonicalize-paths.patch: Likewise.
>> * gnu/local.mk (dist_patch_DATA): Update patches list.
>> * gnu/packages/webkit.scm (webkitgtk)[patches]: Adjust accordingly.
>> ---
>> gnu/local.mk                                  |  3 +-
>> .../webkitgtk-adjust-bubblewrap-paths.patch   | 38 +++++++++++
>> .../patches/webkitgtk-bind-all-fonts.patch    | 17 +++--
>> .../webkitgtk-canonicalize-paths.patch        | 66 +++++++++++++++++++
>> .../patches/webkitgtk-share-store.patch       | 19 ------
>> gnu/packages/webkit.scm                       |  7 +-
>> 6 files changed, 118 insertions(+), 32 deletions(-)
>> create mode 100644 gnu/packages/patches/webkitgtk-adjust-bubblewrap-paths.patch
>> create mode 100644 gnu/packages/patches/webkitgtk-canonicalize-paths.patch
>> delete mode 100644 gnu/packages/patches/webkitgtk-share-store.patch
>
> LGTM. Thanks for taking up that WebKit issue and getting it fixed
> upstream!

Thanks for the quick review!

Applied as b9a4705f80.

Closing.

Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.