Subject: bug#54135: closed (Re: [bug#54135] [PATCH] gnu: webkitgtk: Adjust BubbleWrap wrapper.)

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 24 Feb 2022 14:26:02 +0000

Your bug report

#54135: [PATCH] gnu: webkitgtk: Adjust BubbleWrap wrapper.

which was filed against the guix-patches package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
54135: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=54135
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: [bug#54135] [PATCH] gnu: webkitgtk: Adjust BubbleWrap wrapper.
From: Maxim Cournoyer <[email-masked]>
To: Jack Hill <[email-masked]>
Cc: [email-masked]
Date: Thu, 24 Feb 2022 09:25:48 -0500
List-Id: <debbugs-submit.debbugs.gnu.org>

Hi Jack,

Jack Hill <[email-masked]> 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

Subject: [PATCH] gnu: webkitgtk: Adjust BubbleWrap wrapper.
From: Maxim Cournoyer <[email-masked]>
To: [email-masked]
Cc: Maxim Cournoyer <[email-masked]>
Date: Wed, 23 Feb 2022 21:29:11 -0500
List-Id: <debbugs-submit.debbugs.gnu.org>

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 <[email-masked]>
-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 <[email-masked]>
----
-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 <[email-masked]>
 ;;; Copyright © 2018 Pierre Neidhardt <[email-masked]>
 ;;; Copyright © 2019 Marius Bakke <[email-masked]>
-;;; Copyright © 2021 Maxim Cournoyer <[email-masked]>
+;;; Copyright © 2021, 2022 Maxim Cournoyer <[email-masked]>
 ;;;
 ;;; 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