Bug 239682 – [GTK] Bubblewrap sandbox lacks support for pipewire

• Component: WebKitGTK
• Status: NEW

[Haelwenn (lanodan) Monnier]

Created attachment 458194 [details]
Patch: BubblewrapLauncher.cpp: Add paths required for pipewire

Tested with playback with pipewire-alsa being used instead of pulse or jack.

[Michael Catanzaro]

Comment on attachment 458194 [details]
Patch: BubblewrapLauncher.cpp: Add paths required for pipewire

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

Hi, can you please add a changelog entry using Tools/Scripts/prepare-ChangeLog? Then set the r? and cq? flags to request review and commit, respectively. Thanks for contributing to WebKit!

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:308
> +    if(!pwRemote) {

if (!pwRemote) {

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:316
> +    GUniquePtr<char> pwRuntimeFile(g_build_filename(pwRuntimeDir, pwRemote, nullptr));
> +    bindIfExists(args, pwRuntimeFile.get(), BindFlags::ReadWrite);

Well this can't be right, because pwRuntimeDir will generally be nullptr. We should only attempt to bind this location if pwRuntimeDir is set. But if it is, then we probably want to use it *instead* of xdgRuntimeDir, right?

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:319
> +    GUniquePtr<char> xdgRuntimeFile(g_build_filename(xdgRuntimeDir, pwRemote, nullptr));
> +    bindIfExists(args, xdgRuntimeFile.get(), BindFlags::ReadWrite);
> +
> +    GUniquePtr<char> pwRuntimeFile(g_build_filename(pwRuntimeDir, pwRemote, nullptr));
> +    bindIfExists(args, pwRuntimeFile.get(), BindFlags::ReadWrite);
> +
> +    GUniquePtr<char> sysRuntimeFile(g_build_filename("/run/pipewire", pwRemote, nullptr));
> +    bindIfExists(args, sysRuntimeFile.get(), BindFlags::ReadWrite);

And /run/pipewire doesn't exist at all on my system. When does pipewire actually use this? There is always a valid XDG runtime dir even if the environment variable is unset, so I'm not sure why pipewire would ever fall back to this location.

Anyway, I think what you're trying to do is "bind the pipewire socket, wherever it exists," right? Probably we can implement the same lookup strategy used by pipewire itself? I notice that we can't copy from flatpak, because flatpak doesn't actually grant access to the pipewire socket. I suppose flatpak expects apps to go through the pulseaudio socket instead?

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:778
>          // FIXME: We should move to Pipewire as soon as viable, Pulse doesn't restrict clients atm.
>          bindPulse(sandboxArgs);

Hi Patrick, is it possible to not do this if binding Pipewire is successful? Or do we need to check if Pipewire is running?

[Patrick Griffis]

(In reply to Michael Catanzaro from comment #1)
> Hi Patrick, is it possible to not do this if binding Pipewire is successful?
> Or do we need to check if Pipewire is running?

Pipewire is socket activated so `pipewire-0` will probably always exist on a system with pipewire installed.

My intuition is to just expect pipewire to work and avoid pulseaudio at that point, but its probably entirely possible that pipewire fails and pulseaudio works. That would be a system bug IMO.

[Patrick Griffis]

(In reply to Michael Catanzaro from comment #1)
> I notice that we can't copy from flatpak,
> because flatpak doesn't actually grant access to the pipewire socket. I
> suppose flatpak expects apps to go through the pulseaudio socket instead?

I think this omission is not on purpose. Its just nobody has been working on it.

I would guess flatpak should grow a `--socket=fallback-pulseaudio` permission that only binds pulseaudio when pipewire is unavailable
and also grow a `--socket=pipewire` permission.

I glanced at the pipewire code and it checks for `/.flatpak-info` on the client pid talking to it to determine if it should have somewhat limited permissions.

[Patrick Griffis]

(In reply to Patrick Griffis from comment #3)
> I glanced at the pipewire code and it checks for `/.flatpak-info` on the
> client pid talking to it to determine if it should have somewhat limited
> permissions.

Actually this was incorrect. The pulse module from pipewire checks this. I didn't see anywhere else it checks it. I'm not familiar enough with the details to know if thats a problem.

[Haelwenn (lanodan) Monnier]

(In reply to Michael Catanzaro from comment #1)
> Comment on attachment 458194 [details]
> Patch: BubblewrapLauncher.cpp: Add paths required for pipewire
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458194&action=review
> 
> > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:308
> > +    if(!pwRemote) {
> 
> if (!pwRemote) {
> 
> > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:316
> > +    GUniquePtr<char> pwRuntimeFile(g_build_filename(pwRuntimeDir, pwRemote, nullptr));
> > +    bindIfExists(args, pwRuntimeFile.get(), BindFlags::ReadWrite);
> 
> Well this can't be right, because pwRuntimeDir will generally be nullptr. We
> should only attempt to bind this location if pwRuntimeDir is set. But if it
> is, then we probably want to use it *instead* of xdgRuntimeDir, right?

Right.

> > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:319
> > +    GUniquePtr<char> xdgRuntimeFile(g_build_filename(xdgRuntimeDir, pwRemote, nullptr));
> > +    bindIfExists(args, xdgRuntimeFile.get(), BindFlags::ReadWrite);
> > +
> > +    GUniquePtr<char> pwRuntimeFile(g_build_filename(pwRuntimeDir, pwRemote, nullptr));
> > +    bindIfExists(args, pwRuntimeFile.get(), BindFlags::ReadWrite);
> > +
> > +    GUniquePtr<char> sysRuntimeFile(g_build_filename("/run/pipewire", pwRemote, nullptr));
> > +    bindIfExists(args, sysRuntimeFile.get(), BindFlags::ReadWrite);
> 
> And /run/pipewire doesn't exist at all on my system. When does pipewire
> actually use this? There is always a valid XDG runtime dir even if the
> environment variable is unset, so I'm not sure why pipewire would ever fall
> back to this location.

/run/pipewire is for when pipewire is started by the system to be used by everyone instead of being bound to your user.
Pipewire clients try to connect to it when it failed to connect to the runtime-dir based one.

> Anyway, I think what you're trying to do is "bind the pipewire socket,
> wherever it exists," right? Probably we can implement the same lookup
> strategy used by pipewire itself? I notice that we can't copy from flatpak,
> because flatpak doesn't actually grant access to the pipewire socket. I
> suppose flatpak expects apps to go through the pulseaudio socket instead?

I haven't looked on the flatpak side of things, I'm not really familiar with it.

Within pipewire itself, the relevant code seems to be at
https://gitlab.freedesktop.org/pipewire/pipewire/-/blob/master/src/modules/module-protocol-native/local-socket.c
and it is setting up a namespace rather than a proxy so using the exact same strategy probably wouldn't work.

[Haelwenn (lanodan) Monnier]

Created attachment 458223 [details]
[GTK] BubblewrapLauncher.cpp: Add paths required for pipewire

[EWS]

[e-mail] does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json.

Rejecting attachment 458223 [details] from commit queue.

[Michael Catanzaro]

Comment on attachment 458223 [details]
[GTK] BubblewrapLauncher.cpp: Add paths required for pipewire

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

OK, the code changes look fine to me. We really do need to disable Pulse when enabling Pipewire, but that can be a future improvement.

You want to set r? and cq?, not r+ and cq+. Adrian already fixed it for you this time, but that's why the commit bot is angry.

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:310
> +    if (!pwRemote) {
> +        pwRemote = "pipewire-0";
> +    }

Style nit: remove the braces. That's why style bot is angry.

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:780
>          // FIXME: We should move to Pipewire as soon as viable, Pulse doesn't restrict clients atm.
>          bindPulse(sandboxArgs);

Can you update this comment please?

// FIXME: Disable PulseAudio access when Pipewire is available.

Bonus points available if you fix it. Should be just checking for sockets.