Improve wineserver discovery (fixes #208, alternative to #211) by showier-drastic · Pull Request #212 · hartwork/sandwine

showier-drastic · 2026-06-08T11:56:17Z

Close #208. Rationales are detailed in the commit message. Co-authored by AI (with wine source code as reference), but verified by human, and also verified on Ubuntu 26.04.

sandwine wraps the sandboxed program with `wineserver -p0` / `wineserver -k` for clean shutdown, invoking wineserver by bare name. That requires the wineserver directory to be present on ${PATH} inside the sandbox. To make this work, the sandbox ${PATH} was augmented with a single hardcoded directory, "/usr/lib/wine". On Debian/Ubuntu, however, wineserver is installed into a multiarch directory, e.g. "/usr/lib/x86_64-linux-gnu/wine/" on Ubuntu 26.04. That directory is neither on the host ${PATH} nor equal to the hardcoded path, so the bare `wineserver` call fails and clean shutdown breaks. Replace the hardcoded path with find_wineserver(), which mirrors the resolution order of Wine's own loader (dlls/ntdll/unix/loader.c:exec_wineserver): ${WINESERVER} -> ${PATH} -> the library directory next to the `wine` loader. The library-directory case is made multiarch-aware via sysconfig's MULTIARCH, so the correct location is discovered generically instead of being hardcoded per distribution. The discovered directory still flows through the existing mount-stack filter, so it is only added to ${PATH} if it exists in the sandbox.

This builds on the previous commit and replaces the ${PATH}-based mechanism with a more robust one. Rather than making the wineserver directory reachable by bare name on the sandbox ${PATH}, resolve the wineserver binary on the host with find_wineserver() and: - export it to the sandbox as ${WINESERVER}, - bind-mount it read-only into the sandbox, and - invoke it by "$WINESERVER" in the clean-shutdown wrapper. Why this is preferable: - Wine itself honors ${WINESERVER}, so both sandwine's own `wineserver -p0` / `wineserver -k` calls and Wine's internal server spawn use the exact same binary -- no divergence. - The binary is bind-mounted explicitly, so it is guaranteed to exist inside the sandbox even when it lives outside the default mount stack (e.g. a custom ${WINESERVER} pointing outside /usr). This does not rely on /usr happening to cover the install location, and the previous commit's ${PATH} append is therefore removed. - When wineserver cannot be found, sandwine now fails up front with a clear CommandNotFound("wineserver") instead of a cryptic "sh: wineserver: not found" from inside the sandbox. The mount and ${WINESERVER} export are placed next to the existing /opt/wine-* mounting logic so the bind mount is part of the mount stack that is later filtered and emitted.

showier-drastic · 2026-06-08T18:42:10Z

Interesting. Where is the wine binary in the CI image?

OK, maybe it's inside /usr/lib/wine. Guess we shouldn still add /usr/lib/wine (or whatever wineserver directory) to PATH?

hartwork · 2026-06-08T19:31:08Z

Interesting. Where is the wine binary in the CI image?

OK, maybe it's inside /usr/lib/wine. Guess we shouldn still add /usr/lib/wine (or whatever wineserver directory) to PATH?

@showier-drastic I would need to debug it myself also. I'll finish review first.

(Note to self: Context was the failed CI run.)

hartwork

I'm not sure yet this will work. Here's a few things I found and would like to clarify:

hartwork · 2026-06-08T18:46:16Z

+    if env_wineserver and os.access(env_wineserver, os.X_OK) and os.path.isfile(env_wineserver):
+        return os.path.realpath(env_wineserver)


I'll have to sleep over this, but I see a potential TOCTTOU vulnerablity here. Running os.path.realpath first and then checking access and file nature there change this picture (..). One more case like that below.

hartwork · 2026-06-08T18:47:14Z


+def find_wineserver() -> str | None:
+    # Locate the wineserver binary the way Wine's own loader does
+    # (dlls/ntdll/unix/loader.c:exec_wineserver), so that distribution-specific


The related code in Wine seems to have changed over time. Which version did you look at?

At https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/ntdll/unix/loader.c?ref_type=heads#L515-518 I see order…

[..]/wine

${WINESERVER}

${PATH}

…whereas in here I see:

${WINESERVER}

${PATH}

[..]/wine

[..]/bin

Would be important to me to better understand.

hartwork · 2026-06-08T18:54:22Z

+
+    multiarch = sysconfig.get_config_var("MULTIARCH")  # e.g. "x86_64-linux-gnu" on Debian/Ubuntu
+    subdirs = [f"lib/{multiarch}/wine"] if multiarch else []
+    subdirs += ["lib/wine", "lib64/wine", "bin"]  # plus BINDIR-style fallback


Mixing 32bit and 64bit paths here seems like a candidate for trouble, in particular in this order which would be 32bit before 64bit on my 64bit box:

# file -L /usr/lib/libz.so /usr/lib64/libz.so | awk '{print $3, $1}' 32-bit /usr/lib/libz.so: 64-bit /usr/lib64/libz.so:

hartwork · 2026-06-08T18:59:59Z

+            if candidate in seen:
+                continue
+            seen.add(candidate)


This mechanism seems to be needed only because subdirs is not checked for duplicates. What is the idea behind not dropping duplicates? E.g. something like prefixes = list(dict.fromkeys(prefixes)) # i.e. drop duplicates while keeping order upfront may work.

hartwork · 2026-06-08T19:05:30Z

+        wineserver_abs_path = find_wineserver()
+        if wineserver_abs_path is None:
+            raise CommandNotFound("wineserver")
+        env_tasks["WINESERVER"] = wineserver_abs_path


If we go forward with this, this new variable would need to be appended in the docs also here:

sandwine/README.md

Lines 146 to 153 in 9325f6e

### Environment Variables

- `${DISPLAY}`

- `${HOME}`

- `${HOSTNAME}` (with random 12-hex-digits value)

- `${PATH}` (with known-unavailable entries removed)

- `${TERM}`

- `${USER}`

If wine itself recognizes this variable, using a different name to not have tings collide in unexpected ways may be more robust. I'd need to sleep over that.

It is intentionally set to WINESERVER, so that wine can use exactly the same wine server to our code. I can add it to docs for sure.

hartwork · 2026-06-08T19:28:42Z

+    if (wine_bin_abs_path := shutil.which("wine")) is not None:
+        # e.g. /usr/bin/wine -> /usr
+        prefixes.append(os.path.dirname(os.path.dirname(os.path.realpath(wine_bin_abs_path))))
+    prefixes += ["/usr", "/usr/local"]


I'll need to sleep over bringing /usr/local in like this.

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>

find_wineserver() validated the unresolved path with os.access()/ os.path.isfile() but returned os.path.realpath() of it. Those calls each resolve symlinks independently, so a symlink swap in between could validate one target while returning a different one to be bind-mounted into the sandbox. Add _resolve_executable_file(), which canonicalizes with realpath first and then validates that resolved path, and use it for both the ${WINESERVER} and library-directory cases.

find_wineserver() previously searched ${WINESERVER} -> ${PATH} -> bin_dir and reverse-engineered bin_dir by guessing lib vs lib64, which on a 64-bit multilib host (where /usr/lib is 32-bit and /usr/lib64 is 64-bit) could pick a 32-bit wineserver. It also did not match the resolution *order* of wine master's dlls/ntdll/unix/loader.c:exec_wineserver(), which for an installed Wine is bin_dir -> ${WINESERVER} -> ${PATH} -> BINDIR. Three changes: - Follow wine master's order: bin_dir, then ${WINESERVER}, then ${PATH}, then a BINDIR (<prefix>/bin) fallback. The build-tree branches wine checks first do not apply to an installed Wine and are omitted. The order is version-sensitive, so the comment is pinned to master. - Stop guessing the bin_dir layout. Anchor on the 'wine' loader itself: os.path.realpath(shutil.which('wine')) follows symlinks (including /etc/alternatives/wine and winehq's /opt/wine-*/bin/wine) to the real loader, next to which wineserver is installed in essentially every packaging. The architecture-exact multiarch directory and a word-size-ordered lib/lib64 fallback remain only as backups (the latter now prefers the native word size, fixing the 32-bit-before-64-bit bug). - Keep the 'wine' loader reachable on the sandbox ${PATH}. wineserver is now invoked by its absolute ${WINESERVER} path, but 'wine' is still launched by bare name and lives next to wineserver (e.g. /usr/lib/wine/ on Ubuntu's wine32:i386, which ships no /usr/bin/wine). Re-add the discovered wineserver's directory to the candidate ${PATH} so the colocated 'wine' resolves again; the existing filter keeps it only if it exists in the sandbox mount stack.

showier-drastic · 2026-06-10T16:00:53Z

Smoke test passed on my side for the latest commit: https://github.com/showier-drastic/sandwine/actions/runs/27288472359

However, it is still complex, and I am not totally satisfied. The main issue is that: wine prefers bin_dir over WINESERVER variable, and in most cases it will pick "bin_dir". This makes WINESERVER almost useless. Should we just remove it to reduce complexity?

showier-drastic · 2026-06-10T16:01:50Z

If you have more issues, I would say please edit directly. I think I don't have sufficient knowledge to go further. Thanks!

showier-drastic · 2026-06-10T16:04:09Z

Oh no, not my fault this time ;)

hartwork · 2026-06-10T16:07:53Z

+    if wine_loader is not None:
+        # e.g. /usr/bin/wine -> /usr
+        prefixes.append(os.path.dirname(os.path.dirname(wine_loader)))
+    prefixes += ["/usr", "/usr/local"]


@showier-drastic update: let's drop prefixes += ["/usr", "/usr/local"] and only use the parent's parent of wine_loader. That's bringing the right of the two in already if needed (e.g. on Debian) and does not add trouble where not. What do you think?

PS: Looping over prefixes can also be fully resolved then, reducing complexity.

I'm not sure about this, but I'll just trust you :)

hartwork · 2026-06-10T16:12:56Z

Oh no, not my fault this time ;)

@showier-drastic restarted just now

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>

Debian / Ubuntu systems will just find it through the wrapper.

hartwork · 2026-06-10T16:21:16Z

+    prefixes = []
+    if wine_loader is not None:
+        # e.g. /usr/bin/wine -> /usr
+        prefixes.append(os.path.dirname(os.path.dirname(wine_loader)))


@showier-drastic can we resolve prefixes and the looping please?

Suggested change

prefixes = []

if wine_loader is not None:

# e.g. /usr/bin/wine -> /usr

prefixes.append(os.path.dirname(os.path.dirname(wine_loader)))

# e.g. /usr/bin/wine -> /usr

wine_prefix = os.path.dirname(os.path.dirname(wine_loader))

showier-drastic · 2026-06-10T16:21:34Z

Oh no, removing /usr and /usr/local broke it. Could you please debug it more?

showier-drastic · 2026-06-10T16:23:23Z

I've allowed "Allow edits and access to secrets by maintainers". You may directly push to my branch.

hartwork · 2026-06-11T01:24:13Z

If you have more issues, I would say please edit directly. I think I don't have sufficient knowledge to go further. Thanks!

@showier-drastic thanks but I believe that Git branches should only be written to by the author of that branch. I have created an alternative pull request #213 that also fixes the problem at arguably lower complexity. I'm good with you being in the review seat there or you you being the driver here — which do you prefer? Thanks!

showier-drastic added 2 commits June 8, 2026 19:51

showier-drastic mentioned this pull request Jun 8, 2026

Support Ubuntu 26.04 (fixes #208, alternative to #212) #211

Closed

hartwork requested changes Jun 8, 2026

View reviewed changes

hartwork changed the title ~~Fix wineserver discovery~~ Fix wineserver discovery (fixes #208, alternative to #211) Jun 9, 2026

hartwork changed the title ~~Fix wineserver discovery (fixes #208, alternative to #211)~~ Improve wineserver discovery (fixes #208, alternative to #211) Jun 9, 2026

showier-drastic and others added 3 commits June 10, 2026 22:47

Add curly braces to WINESERVER invocation

39d7730

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>

showier-drastic force-pushed the fix-wineserver-discovery branch from 171dcf7 to a7b7fae Compare June 10, 2026 15:51

hartwork reviewed Jun 10, 2026

View reviewed changes

Comment thread sandwine/_main.py Outdated

showier-drastic and others added 2 commits June 11, 2026 00:13

Stop if wine is not found

a41210f

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>

Remove hard-coded /usr and /usr/local from prefixes

494cfbd

Debian / Ubuntu systems will just find it through the wrapper.

hartwork reviewed Jun 10, 2026

View reviewed changes

hartwork mentioned this pull request Jun 10, 2026

Improve wineserver command detection (e.g. for Debian sid and Ubuntu 26.04) (fixes #208, alternative to #211 and #212) #213

Open

		if env_wineserver and os.access(env_wineserver, os.X_OK) and os.path.isfile(env_wineserver):
		return os.path.realpath(env_wineserver)

	### Environment Variables

	- `${DISPLAY}`
	- `${HOME}`
	- `${HOSTNAME}` (with random 12-hex-digits value)
	- `${PATH}` (with known-unavailable entries removed)
	- `${TERM}`
	- `${USER}`

Conversation

showier-drastic commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

showier-drastic commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hartwork commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hartwork left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showier-drastic commented Jun 10, 2026

Uh oh!

showier-drastic commented Jun 10, 2026

Uh oh!

showier-drastic commented Jun 10, 2026

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!

Uh oh!

hartwork commented Jun 10, 2026

Uh oh!

hartwork Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

showier-drastic commented Jun 10, 2026

Uh oh!

showier-drastic commented Jun 10, 2026

Uh oh!

hartwork commented Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

showier-drastic commented Jun 8, 2026 •

edited

Loading

showier-drastic commented Jun 8, 2026 •

edited

Loading

hartwork commented Jun 8, 2026 •

edited

Loading

hartwork Jun 10, 2026 •

edited

Loading