Export parent directory of LD_PRELOAD modules
Another step towards solving steamrt/tasks#595, based on the refactoring in !783 (merged) and !784 (merged).
!780 should be rebased onto this, but I'm not there yet.
-
pv-wrap: Have a file descriptor for $HOME in the PvWrapContext
This will allow the LD_PRELOAD handling code to use a real or mock home directory, as appropriate.
-
wrap-setup test: Assert that paths passed to fixture_populate_dir are relative
-
wrap-setup test: Use a temporary variable for the path to create
No functional change.
-
wrap-setup test: Convert absolute paths to relative
This will make it clearer when writing tests where a LD_PRELOAD entry is very similar (or even identical) to the path that we need to create to represent it.
-
wrap-setup test: Move list of LD_PRELOAD entries to top level
This will let us make the tests that involve it more declarative, by grouping the various inputs and expectations related to one LD_PRELOAD entry into the struct associated with that entry.
-
wrap-setup test: Make setup of mock sysroot more declarative
-
wrap-setup test: Move expectations into same data structure as inputs
-
wrap-setup test: Stop asserting that "/" is marked as exported
This makes no sense: the comment above indicates that we didn't want this path to be exported, and if it was exported then we would be unable to have any control over the filesystem namespace.
In fact, it is not exported: the fact that it appears to be exported is an implementation quirk of flatpak_exports_path_get_mode(), which returns FLATPAK_FILESYSTEM_MODE_READ_WRITE because none of the zero remaining path segments is unmapped.
-
wrap-setup test: Store a pointer to the configuration in the fixture
-
wrap-setup test: Set a mock home directory
Otherwise, we can't test behaviours that depend on the home directory.
-
wrap-setup test: Make assertions about the FlatpakExports declarative
This keeps the inputs and their expected results together.
-
wrap-setup test: Clarify why it's correct to not export $HOME
steamrt/tasks#595
-
wrap-setup test: Systematically check ancestors of LD_PRELOADs
In two cases we were asserting that the grandparent directory is not exported, and the LD_PRELOAD module itself is, but making no assertion about the parent directory of the LD_PRELOAD module.
To make modules like MangoHud work more reliably, in fact we want to export the parent directory in most cases.
steamrt/tasks#595
-
wrap-setup test: Test export behaviour in a subdirectory of $HOME
Contrast this with the previous test coverage for a module directly inside $HOME: in that situation we don't want to overrule --unshare-home, but in this situation we do want to share a small subset of it.
steamrt/tasks#595
-
pv-wrap: Export parent dir of LD_PRELOAD modules, unless it's $HOME
LD_PRELOAD modules are an inherently fragile mechanism, and any symbols exported by either a LD_PRELOAD module or its direct dependencies will "interpose" in front of symbols of the same name from any other library. In some cases this can lead to crashes, for example when the Python ctypes module from Steam Runtime 2 'soldier' (which depends on libffi.so.6) gets its libffi references resolved to symbols from a more modern host system's libffi.so.7 or libffi.so.8 pulled in by the dependency chain:
libMangoHud.so -> libwayland-client.so.0 -> libffi.so.8
Authors of LD_PRELOAD modules like MangoHud can avoid this situation by making their LD_PRELOAD module only depend on highly long-term-stable projects (ideally only glibc), and then loading the implementation of its real functionality via
dlopen()
, usingRTLD_LOCAL|RTLD_DEEPBIND
to avoid symbol conflicts.For example, newer versions of MangoHud separate the module into libMangoHud_shim.so (the actual LD_PRELOAD module, no dependencies except glibc) and libMangoHud_opengl.so (dlopen'd by the shim module, depends on whatever it needs to depend on).
This is a good approach and we should encourage it, but in the current pressure-vessel codebase it isn't going to be completely reliable for modules that are installed other than in /usr: for libMangoHud_shim.so to be able to load libMangoHud_opengl.so, we need to ensure that they are both visible in the filesystem of the game's container. However, since we can't know what modules and/or libraries libMangoHud_shim.so is going to load at runtime, we will have to use a heuristic.
The heuristic I've chosen here is to say that if you have
LD_PRELOAD=/path/to/some/module.so
, then instead of sharing/path/to/some/module.so
with the container as we did previously, we should share the parent directory/path/to/some/
. In particular, this is enough for MangoHud's requirements.However, one notable exception is that if the module is situated directly inside
$HOME
, and we are using--unshare-home
to avoid sharing the home directory with the container, we probably do not want to overrule that. In this case, users will still need to usePRESSURE_VESSEL_FILESYSTEMS_RO
to share the dependency explicitly.steamrt/tasks#595
-
wrap-setup test: Assert we export parents of LD_PRELOADs if appropriate
steamrt/tasks#595
Merge request reports
Activity
added Awaiting Review Bug To Test pressure-vessel labels
requested review from @denittis
assigned to @smcv
mentioned in merge request !780
removed To Test label
added 16 commits
-
7d792ce5...22f91863 - 5 commits from branch
main
- a43f0cd7 - 1 earlier commit
- 135f38dd - pv-wrap: Make it more obvious that we're using the environment read-only
- 1810e830 - tests: Store the mock environment in PvWrapContext instead of duplicating
- 73733519 - pv-wrap: Use the PvWrapContext to inspect the original environment
- 6557a5e2 - PvWrapContext: Store the PvRuntime in here
- 0755e37d - pv-wrap: Take "remove game overlay" option from PvWrapContext
- 1bbff5fa - pv-wrap: Store FlatpakExports in the PvWrapContext
- 2ad3a246 - pv-wrap: Store the home directory in the PvWrapContext
- 9ac81afe - pv-wrap: Have a file descriptor for $HOME in the PvWrapContext
- c4ec8a12 - pv-wrap: Export parent dir of LD_PRELOAD modules, unless it's $HOME
- e590fbbf - tests: Check for intended export behaviour
Toggle commit list-
7d792ce5...22f91863 - 5 commits from branch
v3: rebased on final !784 (merged), no functional changes. If you're already reviewing v2, sorry, please continue with v2 (last 3 commits only) and any review feedback there should be equally applicable to v3.
- Resolved by Simon McVittie
removed Awaiting Review label
removed review request for @denittis
- Resolved by Simon McVittie
On second thought this needs better test coverage, so I'm going to respin this with more thorough/intentional test coverage if that's OK.
added 6 commits
-
e590fbbf...44855f60 - 3 commits from branch
main
- 9e257a57 - pv-wrap: Have a file descriptor for $HOME in the PvWrapContext
- 0fd0ec78 - pv-wrap: Export parent dir of LD_PRELOAD modules, unless it's $HOME
- 043feeff - tests: Check for intended export behaviour
Toggle commit list-
e590fbbf...44855f60 - 3 commits from branch
added 15 commits
- 043feeff...f4bae94a - 5 earlier commits
- 8ba3b8d6 - wrap-setup test: Move expectations into same data structure as inputs
- 76995e52 - wrap-setup test: Stop asserting that "/" is marked as exported
- 00073507 - wrap-setup test: Store a pointer to the configuration in the fixture
- 065cffd8 - wrap-setup test: Set a mock home directory
- be40344c - wrap-setup test: Make assertions about the FlatpakExports declarative
- 48f92a03 - wrap-setup test: Clarify why it's correct to not export $HOME
- b69911a7 - wrap-setup test: Systematically check ancestors of LD_PRELOADs
- 44a94426 - wrap-setup test: Test export behaviour in a subdirectory of $HOME
- ba0ffa1e - pv-wrap: Export parent dir of LD_PRELOAD modules, unless it's $HOME
- 80260f1c - wrap-setup test: Assert we export parents of LD_PRELOADs if appropriate
Toggle commit listadded 1 commit
- 9ca47131 - wrap-setup test: Assert we export parents of LD_PRELOADs if appropriate