[bug#53676] [PATCH 0/5] *** PulseAudio service improvements ***

[Maxim Cournoyer]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

> Hi,
>
> Am Montag, dem 07.02.2022 um 17:29 -0500 schrieb Maxim Cournoyer:
>> Thanks for this!  I wasn't aware of the history; I tried it and it
>> failed the same.  The following fix I attempted in webkitgtk did not
>> seem to do anything:
>> 
>> --8<---------------cut here---------------start------------->8---
>> modified  
>> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
>> @@ -24,6 +24,7 @@
>>  #include <fcntl.h>
>>  #include <glib.h>
>>  #include <seccomp.h>
>> +#include <string.h>
>>  #include <sys/ioctl.h>
>>  #include <sys/mman.h>
>>  #include <unistd.h>
>> @@ -337,7 +338,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.
>> +    char canonicalPath[PATH_MAX];
>> +    if (!realpath(path, canonicalPath)) {
>> +        if (strlen(path) + 1 > PATH_MAX)
>> +            return;                  // too long of a path
>> +        strcpy(path, canonicalPath); // no-op
>> +    }
>> +    args.appendVector(Vector<CString>({ bindType, canonicalPath,
>> path }));
>>  }
> Apart from raw char arrays and string.h looking funny (and wrong) in
> C++, what is strcpy supposed to do here?  Would it work if we mapped
> canonicalPath to path (i.e. `ls path' in the container would be `ls
> canonicalPath' under the hood)?

I first went the C++ solution, which is std::filesystem::canonical, but
was suggested in #webkitgtk (on the GNOME IRC server) to use the POSIX
realpath, already in use in that file, upon finding out that their build
system is configured to disallow the use of exceptions
(-fno-exceptions).  I refined the experiment as:

--8<---------------cut here---------------start------------->8---
diff --git a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp 
b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
index 0d5dd4f6986d..1512b73a985d 100644
--- a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
+++ b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
@@ -325,6 +325,18 @@ enum class BindFlags {
     Device,
 };
 
+static void bindSymlinksRealPath(Vector<CString>& args, const char* path,
+                                 const char* bindOption = "--ro-bind")
+{
+    char realPath[PATH_MAX];
+
+    if (realpath(path, realPath) && strcmp(path, realPath)) {
+        args.appendVector(Vector<CString>({
+            bindOption, realPath, realPath,
+        }));
+    }
+}
+
 static void bindIfExists(Vector<CString>& args, const char* path, BindFlags 
bindFlags = BindFlags::ReadOnly)
 {
     if (!path || path[0] == '\0')
@@ -337,6 +349,10 @@ static void bindIfExists(Vector<CString>& args, const 
char* path, BindFlags bind
         bindType = "--ro-bind-try";
     else
         bindType = "--bind-try";
+
+    // Canonicalize the source path, otherwise a symbolic link could
+    // point to a location outside of the namespace.
+    bindSymlinksRealPath(args, path, bindType);
     args.appendVector(Vector<CString>({ bindType, path, path }));
 }
 
@@ -615,17 +631,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
 --8<---------------cut here---------------end--------------->8---

Which produced the intended bwrap arguments, but unfortunately that'd
still fail.  The issue seems to be related to attempt to bind
/etc/pulse/client.conf over something already existing there; it can be
simply reproduced with:

--8<---------------cut here---------------start------------->8---
$ guix shell bubblewrap -- bwrap --ro-bind /gnu /gnu \
    --ro-bind /etc /etc \
    --ro-bind /etc/pulse/client.conf /etc/pulse/client.conf \
    /gnu/store/4y5m9lb8k3qkb1y9m02sw9w9a6hacd16-bash-minimal-5.1.8/bin/bash
bwrap: Can't create file at /etc/pulse/client.conf: No such file or directory
--8<---------------cut here---------------end--------------->8---

One thing to try would be to not bind mount client.conf; /etc/ is
already bind mounted as a whole.  If the resolved paths are all bind
mounted (which they are since we share the whole of /gnu), we should be
OK.

Alternatively we could try to bind only the resolved paths, and rewrite
the environment variables such as PULSE_CLIENTCONFIG at run time in
webkitgtk such that it points to the resolved destination.

To be continued...

Maxim