Hello Jack,

Thanks a lot for this work.

Jack Hill <jackhill@jackhill.us> writes:

> Some additional observations:
>
> With my patched webkitgtk, if I set:
>
> PULSE_CLIENTCONFIG=/gnu/store/zc4dsmvdabi00nvisrjhi9w00ff4igs7-client.conf
>
> it does work, which is an improvement compared to without the patch.

Great.  I have attached a patch for Guix that stops using /etc for these
variables.

> I notice that Nix [0] has a similar patch:
>
> """
> diff -ru old/webkitgtk-2.26.0/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp webkitgtk-2.26.0/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
> --- old/webkitgtk-2.26.0/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp	2019-09-09 04:47:07.000000000 -0400
> +++ webkitgtk-2.26.0/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp	2019-09-20 21:14:10.537921173 -0400
> @@ -585,7 +585,7 @@
>           { SCMP_SYS(keyctl), nullptr },
>           { SCMP_SYS(request_key), nullptr },
>
> -        // Scary VM/NUMA ops 
> +        // Scary VM/NUMA ops
>           { SCMP_SYS(move_pages), nullptr },
>           { SCMP_SYS(mbind), nullptr },
>           { SCMP_SYS(get_mempolicy), nullptr },
> @@ -724,6 +724,10 @@
>           "--ro-bind-try", "/usr/local/lib64", "/usr/local/lib64",
>
>           "--ro-bind-try", PKGLIBEXECDIR, PKGLIBEXECDIR,
> +
> +        // Nix Directories
> +        "--ro-bind", "@storeDir@", "@storeDir@",
> +        "--ro-bind", "/run/current-system", "/run/current-system",
>       };
>       // We would have to parse ld config files for more info.
>       bindPathVar(sandboxArgs, "LD_LIBRARY_PATH");
> """
>
> [0] https://github.com/NixOS/nixpkgs/blob/465566948393cf533e3617704d1c4ccc34cf3753/pkgs/development/libraries/webkitgtk/fix-bubblewrap-paths.patch
>
> so I wonder if I didn't do the mounts in the right place and or if it is 
> becasue I missed /run/current-system.
>
> I'm going to try to adapt the Nix patch to see if that helps.

Were you able to verify whether /run/current-system is required inside
the sandbox?

I cleaned up your patch a bit and rebased it on the latest master
branch, available as patch 2/2 below.  Currently building it on
'core-updates' to verify that it works.  It takes a while on my dinky
quad-core server though.  :-)

It does not bind /run/current-system, and I think we should avoid it if
possible.  Ideally we would only mount the store paths required by the
consumers instead of all of /gnu/store, but not sure how to achieve
that.