[GStreamer] plugin scanner not available in bwrap sandbox #40320

philn · 2025-02-09T20:05:16Z

`526ec66`

[GStreamer] plugin scanner not available in bwrap sandbox
https://bugs.webkit.org/show_bug.cgi?id=287368

Reviewed by Michael Catanzaro.

On UsrMerge systems where /lib64 is a symlink to /usr/lib64 and where the GStreamer registry
attempts to load the plugin-scanner from /lib64/ the path needs to be allow-listed in the sandbox.

* Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:
(WebKit::bindGStreamerData):

Canonical link: https://commits.webkit.org/296779@main

b6d2cdc

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2025-02-09T20:05:22Z

EWS run on previous version of this PR (hash e1f1ed3)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

mcatanzaro

I'm going to click Approve, but I do wonder if this is really significantly easier than just adding some more symlinks to sandboxArgs in bubblewrapSpawn to fix things for more than just GStreamer?

We could test if /lib64 is a symlink on the host, and if so, then do "--symlink", "/lib64", "/usr/lib64", and that should probably fix this? Then we don't need to wonder what else besides GStreamer might be broken.

I checked what flatpak does. flatpak_run_setup_usr_links() creates symlinks for abs_usrmerged_dirs which is defined in flatpak-exports.c to be /bin, /lib, /lib32, /lib64, and /sbin. That's not so many locations; doesn't seem too hard?

mcatanzaro · 2025-02-10T16:31:51Z

CC @TingPing

philn · 2025-02-10T16:35:38Z

Yeah, I suppose we can use the same approach as flatpak. I can rework this, thanks for the review/feedback.

webkit-early-warning-system · 2025-02-10T20:12:06Z

EWS run on previous version of this PR (hash 25b0e6d)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	❌ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

philn · 2025-02-10T20:12:27Z

I tried this, but ... bwrap: execvp /usr/bin/xdg-dbus-proxy: No such file or directory.

mcatanzaro · 2025-02-10T22:26:57Z

Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp

+            auto symLink = makeString("/usr"_s, directory, '/');
+            args.appendVector(Vector<CString> { "--symlink", directory.utf8(), symLink.utf8() });


I think this is backwards, which would explain why /usr/bin/xdg-dbus-proxy is missing:

Suggested change

auto symLink = makeString("/usr"_s, directory, '/');

args.appendVector(Vector<CString> { "--symlink", directory.utf8(), symLink.utf8() });

auto linkTarget = makeString("/usr"_s, directory, '/');

args.appendVector(Vector<CString> { "--symlink", linkTarget.utf8(), directory.utf8() });

I tried both ways, didn't notice an improvement either way.

mcatanzaro · 2025-02-10T22:36:02Z

Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp

+            auto symLink = makeString("/usr"_s, directory);
+            args.appendVector(Vector<CString> { "--ro-bind-try", symLink.utf8(), symLink.utf8() });


This fails on non-usrmerged systems, e.g. if /bin is not a symlink then it won't be bound.

What I would do is bind normally here instead:

Suggested change

auto symLink = makeString("/usr"_s, directory);

args.appendVector(Vector<CString> { "--ro-bind-try", symLink.utf8(), symLink.utf8() });

args.appendVector(Vector<CString> { "--ro-bind", directory.utf8(), directory.utf8() });

That will take care of binding any paths not under /usr, like /bin or /lib. But we also need to bind the paths under /usr, like /usr/bin or /usr/lib. We could do that above, in the if (fileType == FileSystem::FileType::SymbolicLink) condition, but that would fail if the symlinks ever get removed (you never know, it could happen 50 years from now!), so probably best to just restore the use of --ro-bind-try that you removed for the locations under /usr?

The reason the current wip patch is like this is that --ro-bind* and --symlink don't seem to play well together. If I keep both I get this bwrap: Can't make symlink at /bin: destination exists and is not a symlink.

I think this needs the attention of someone actually familiar with the bwrap options.

The options do exactly what you tell them to. It's either create a symlink or mount a directory. The error message indicates exactly what is wrong: this is a usrmerged system and /bin is supposed to be a symlink to /usr/bin, so BubblewrapLauncher.cpp shouldn't be trying to --ro-bind the host /bin. But we do need to --ro-bind it on non-usrmerged systems. We need to do one of create symlink or bind host directory -- never both -- for each of these locations. That error message indicates your code is doing both. It's probably a simple mistake?

Also: I notice the existing sandbox intentionally skips binding /bin except in the enableDebugPermissions case. We should probably maintain that behavior and just altogether remove /bin and /sbin from this function. We could create the symlinks in the enableDebugPermissions codepath if you want, though.

You realize you contradict yourself then? You said "so probably best to just restore the use of --ro-bind-try that you removed for the locations under /usr?" and now "We need to do one of create symlink or bind host directory -- never both -- for each of these locations".

Use --ro-bind-try in the non-usrmerge condition, when /bin is not a symlink.

webkit-early-warning-system · 2025-06-29T08:48:47Z

EWS run on current version of this PR (hash b6d2cdc)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

philn · 2025-06-29T08:49:27Z

I'm landing the initial version of this patch, which is simple and less invasive than the broken WIP I couldn't make work :(

https://bugs.webkit.org/show_bug.cgi?id=287368 Reviewed by Michael Catanzaro. On UsrMerge systems where /lib64 is a symlink to /usr/lib64 and where the GStreamer registry attempts to load the plugin-scanner from /lib64/ the path needs to be allow-listed in the sandbox. * Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp: (WebKit::bindGStreamerData): Canonical link: https://commits.webkit.org/296779@main

webkit-commit-queue · 2025-06-29T09:59:45Z

Committed 296779@main (526ec66): https://commits.webkit.org/296779@main

Reviewed commits have been landed. Closing PR #40320 and removing active labels.

philn requested a review from a team as a code owner February 9, 2025 20:05

philn self-assigned this Feb 9, 2025

philn added the Platform Portability improvements and other general platform improvements not driven directly by site bugs. label Feb 9, 2025

mcatanzaro approved these changes Feb 10, 2025

View reviewed changes

philn force-pushed the eng/287368 branch from e1f1ed3 to 25b0e6d Compare February 10, 2025 20:12

philn marked this pull request as draft February 10, 2025 20:40

mcatanzaro reviewed Feb 10, 2025

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 11, 2025

philn removed the merging-blocked Applied to prevent a change from being merged label Jun 29, 2025

philn force-pushed the eng/287368 branch from 25b0e6d to b6d2cdc Compare June 29, 2025 08:48

philn marked this pull request as ready for review June 29, 2025 08:49

philn added the merge-queue Applied to send a pull request to merge-queue label Jun 29, 2025

webkit-commit-queue force-pushed the eng/287368 branch from b6d2cdc to 526ec66 Compare June 29, 2025 09:59

webkit-commit-queue merged commit 526ec66 into WebKit:main Jun 29, 2025

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 29, 2025

philn deleted the eng/287368 branch June 29, 2025 10:04

		auto symLink = makeString("/usr"_s, directory, '/');
		args.appendVector(Vector<CString> { "--symlink", directory.utf8(), symLink.utf8() });

		auto symLink = makeString("/usr"_s, directory);
		args.appendVector(Vector<CString> { "--ro-bind-try", symLink.utf8(), symLink.utf8() });

[GStreamer] plugin scanner not available in bwrap sandbox #40320

[GStreamer] plugin scanner not available in bwrap sandbox #40320

Conversation

philn commented Feb 9, 2025 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcatanzaro left a comment

Choose a reason for hiding this comment

Uh oh!

mcatanzaro commented Feb 10, 2025

Uh oh!

philn commented Feb 10, 2025

Uh oh!

webkit-early-warning-system commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philn commented Feb 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Jun 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philn commented Jun 29, 2025

Uh oh!

webkit-commit-queue commented Jun 29, 2025

Uh oh!

Uh oh!

philn commented Feb 9, 2025 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Feb 9, 2025 •

edited

Loading

webkit-early-warning-system commented Feb 10, 2025 •

edited

Loading

webkit-early-warning-system commented Jun 29, 2025 •

edited

Loading