From 302cf1b99576cad8fb8f7eae5929b2212052d20d Mon Sep 17 00:00:00 2001 From: oobabooga Date: Fri, 7 Aug 2026 09:27:54 -0300 Subject: [PATCH] Desktop: prevent thin AppImage environment failures (#8055) * Desktop: prevent thin AppImage environment failures * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Accept the unversioned AppIndicator names in the tray preflight * Desktop: harden AppIndicator preflight * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stop the AppIndicator preflight at the first candidate the loader would use The loader commits to the first file it finds for a dlopen name, so a broken copy ahead of a working one fails the dlopen rather than falling through to a later directory. The search now walks one name at a time across LD_LIBRARY_PATH, the ldconfig cache and the default directories, and judges only the first readable match, so a host whose tray still crashes no longer passes the guard. * Tighten the AppIndicator preflight comments --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: danielhanchen --- studio/src-tauri/linux/build-thin-appimage.sh | 80 +++++- studio/src-tauri/src/commands.rs | 3 + studio/src-tauri/src/desktop_auth.rs | 3 + studio/src-tauri/src/install.rs | 3 + studio/src-tauri/src/preflight/managed.rs | 6 + studio/src-tauri/src/process.rs | 22 ++ studio/src-tauri/src/update.rs | 3 + .../security/test_release_desktop_appimage.py | 229 +++++++++++++++++- 8 files changed, 337 insertions(+), 12 deletions(-) diff --git a/studio/src-tauri/linux/build-thin-appimage.sh b/studio/src-tauri/linux/build-thin-appimage.sh index 8399d44b11..f222eb1629 100755 --- a/studio/src-tauri/linux/build-thin-appimage.sh +++ b/studio/src-tauri/linux/build-thin-appimage.sh @@ -129,6 +129,83 @@ missing=$( ldd "$binary" 2>/dev/null | sed -n 's/^[[:space:]]*\([^[:space:]]*\)[[:space:]]*=>[[:space:]]*not found.*$/\1/p' ) + +# The tray crate dlopens AppIndicator, so it never appears in the binary's ldd +# output. libappindicator-sys tries the versioned sonames then the unversioned +# ones, so accept all four in that order or we reject hosts whose tray works. +appindicator_names="libayatana-appindicator3.so.1 libappindicator3.so.1" +appindicator_names="$appindicator_names libayatana-appindicator3.so libappindicator3.so" + +appindicator_is_usable() { + [ -r "$1" ] || return 1 + # An absent ldd is inconclusive, as in the guard above: the loader, not ldd, + # decides whether a readable candidate can load. + command -v ldd >/dev/null 2>&1 || return 0 + appindicator_ldd=$(ldd "$1" 2>&1) || return 1 + ! printf '%s\n' "$appindicator_ldd" | grep -q 'not found' +} + +# Every path the loader tries for a dlopen name, in order: LD_LIBRARY_PATH, +# the ldconfig cache, then the default directories. +appindicator_candidates() { + library_name="$1" + + if [ "${LD_LIBRARY_PATH+x}" = x ]; then + # The loader accepts ':' and ';', and an empty component means the current + # directory. The appended separator keeps a trailing empty one visible. + library_path="${LD_LIBRARY_PATH}:" + while [ -n "$library_path" ]; do + library_dir=${library_path%%[:;]*} + library_path=${library_path#*[:;]} + [ -n "$library_dir" ] || library_dir=. + printf '%s\n' "$library_dir/$library_name" + done + fi + + if command -v ldconfig >/dev/null 2>&1; then + ldconfig -p 2>/dev/null | awk -v name="$library_name" '$1 == name { print $NF }' + fi + + printf '%s\n' \ + /lib/"$library_name" \ + /lib/*-linux-gnu/"$library_name" \ + /lib64/"$library_name" \ + /usr/lib/"$library_name" \ + /usr/lib/*-linux-gnu/"$library_name" \ + /usr/lib64/"$library_name" \ + /usr/local/lib/"$library_name" +} + +find_appindicator() { + for library_name in $appindicator_names; do + # The loader commits to the first file it finds for a name, so stop there: + # skipping ahead would pass a host whose dlopen still fails. + first_candidate=$( + appindicator_candidates "$library_name" | + while IFS= read -r candidate; do + if [ -r "$candidate" ]; then + printf '%s\n' "$candidate" + break + fi + done + ) + [ -n "$first_candidate" ] || continue + if appindicator_is_usable "$first_candidate"; then + printf '%s\n' "$first_candidate" + return 0 + fi + done + return 1 +} + +if ! find_appindicator >/dev/null; then + if [ -n "$missing" ]; then + missing="$missing +" + fi + missing="${missing}libayatana-appindicator3.so.1 or libappindicator3.so.1" +fi + if [ -n "$missing" ]; then message="Unsloth cannot start because required Linux libraries are missing: @@ -167,7 +244,8 @@ fi ) assert_thin_appdir "$verify_dir/squashfs-root" -if grep -q 'LD_LIBRARY_PATH' "$verify_dir/squashfs-root/AppRun"; then +if grep -Eq '(^|[[:space:]])(export[[:space:]]+)?LD_LIBRARY_PATH=' \ + "$verify_dir/squashfs-root/AppRun"; then echo "Thin AppImage must not override the host library search path." >&2 exit 1 fi diff --git a/studio/src-tauri/src/commands.rs b/studio/src-tauri/src/commands.rs index 86ca3b8189..0ff9010709 100644 --- a/studio/src-tauri/src/commands.rs +++ b/studio/src-tauri/src/commands.rs @@ -153,6 +153,9 @@ pub async fn check_install_status() -> bool { cmd.creation_flags(crate::process::CREATE_NO_WINDOW); } + #[cfg(target_os = "linux")] + crate::process::scrub_appimage_python_env_tokio(&mut cmd); + // Tauri uses the legacy root regardless of UNSLOTH_STUDIO_HOME / STUDIO_HOME; // probe subprocesses must follow the same isolation as process.rs. cmd.env_remove("UNSLOTH_STUDIO_HOME"); diff --git a/studio/src-tauri/src/desktop_auth.rs b/studio/src-tauri/src/desktop_auth.rs index c0560300cf..3af08945ff 100644 --- a/studio/src-tauri/src/desktop_auth.rs +++ b/studio/src-tauri/src/desktop_auth.rs @@ -230,6 +230,9 @@ async fn provision_desktop_auth() -> Result<(), String> { cmd.args(["studio", "provision-desktop-auth"]) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::piped()); + #[cfg(target_os = "linux")] + crate::process::scrub_appimage_python_env_tokio(&mut cmd); + // Tauri uses the legacy root regardless of UNSLOTH_STUDIO_HOME / STUDIO_HOME. // Scrub so provisioning writes match what the Rust auth code reads. cmd.env_remove("UNSLOTH_STUDIO_HOME"); diff --git a/studio/src-tauri/src/install.rs b/studio/src-tauri/src/install.rs index dad51dbeea..607b5816b6 100644 --- a/studio/src-tauri/src/install.rs +++ b/studio/src-tauri/src/install.rs @@ -426,6 +426,9 @@ fn spawn_script( .stdout(Stdio::piped()) .stderr(Stdio::piped()); + #[cfg(target_os = "linux")] + crate::process::scrub_appimage_python_env(&mut cmd); + // Tauri only does default-root installs; install.sh / install.ps1 reject // these under --tauri. Scrub so an inherited value can't trip the guard. cmd.env_remove("UNSLOTH_STUDIO_HOME"); diff --git a/studio/src-tauri/src/preflight/managed.rs b/studio/src-tauri/src/preflight/managed.rs index 532c5040d5..8f8313c762 100644 --- a/studio/src-tauri/src/preflight/managed.rs +++ b/studio/src-tauri/src/preflight/managed.rs @@ -307,6 +307,9 @@ async fn run_cli_probe(bin: &Path, args: &[&str]) -> bool { let mut cmd = Command::new(bin); cmd.args(args).stdout(Stdio::null()).stderr(Stdio::null()); + #[cfg(target_os = "linux")] + crate::process::scrub_appimage_python_env_tokio(&mut cmd); + // Tauri uses the legacy root regardless of UNSLOTH_STUDIO_HOME / STUDIO_HOME; // probe subprocesses must follow the same isolation as process.rs. cmd.env_remove("UNSLOTH_STUDIO_HOME"); @@ -353,6 +356,9 @@ async fn probe_cli_capability(bin: &Path) -> Option { .stdout(Stdio::piped()) .stderr(Stdio::null()); + #[cfg(target_os = "linux")] + crate::process::scrub_appimage_python_env_tokio(&mut cmd); + // Tauri uses the legacy root regardless of UNSLOTH_STUDIO_HOME / STUDIO_HOME; // probe subprocesses must follow the same isolation as process.rs. cmd.env_remove("UNSLOTH_STUDIO_HOME"); diff --git a/studio/src-tauri/src/process.rs b/studio/src-tauri/src/process.rs index 78e62d465c..9040ef9229 100644 --- a/studio/src-tauri/src/process.rs +++ b/studio/src-tauri/src/process.rs @@ -12,6 +12,25 @@ use tauri::{AppHandle, Emitter, Manager}; const MAX_LOG_LINES: usize = 1000; +// An AppImage can be launched from an activated Python environment. Keep the +// host library path the thin bundle needs, but do not let PYTHONHOME/PYTHONPATH +// shadow the managed Studio environment. +#[cfg(target_os = "linux")] +pub(crate) fn scrub_appimage_python_env(cmd: &mut Command) { + if std::env::var_os("APPIMAGE").is_some() { + cmd.env_remove("PYTHONHOME"); + cmd.env_remove("PYTHONPATH"); + } +} + +#[cfg(target_os = "linux")] +pub(crate) fn scrub_appimage_python_env_tokio(cmd: &mut tokio::process::Command) { + if std::env::var_os("APPIMAGE").is_some() { + cmd.env_remove("PYTHONHOME"); + cmd.env_remove("PYTHONPATH"); + } +} + #[cfg(windows)] const STUDIO_MANAGED_RUNTIME_MUTEX_PREFIX: &str = "Global\\UnslothStudioManagedEnvironment-"; @@ -1130,6 +1149,9 @@ pub fn start_backend( ); } + #[cfg(target_os = "linux")] + scrub_appimage_python_env(&mut cmd); + // Tauri uses the legacy root regardless of UNSLOTH_STUDIO_HOME / STUDIO_HOME; // scrub so the spawned Python backend can't diverge. UNSLOTH_LLAMA_CPP_PATH // is a pre-existing user-controlled llama.cpp dir override; keep it. diff --git a/studio/src-tauri/src/update.rs b/studio/src-tauri/src/update.rs index bb87b4f741..020ffaa15e 100644 --- a/studio/src-tauri/src/update.rs +++ b/studio/src-tauri/src/update.rs @@ -89,6 +89,9 @@ fn spawn_update( let mut cmd = build_update_command(bin)?; cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); + #[cfg(target_os = "linux")] + crate::process::scrub_appimage_python_env(&mut cmd); + // Tauri manages the legacy root; scrub so 'unsloth studio update' targets // the same install the desktop app uses, not an inherited custom root. cmd.env_remove("UNSLOTH_STUDIO_HOME"); diff --git a/tests/security/test_release_desktop_appimage.py b/tests/security/test_release_desktop_appimage.py index 4e6ca314fd..262f13f4d4 100644 --- a/tests/security/test_release_desktop_appimage.py +++ b/tests/security/test_release_desktop_appimage.py @@ -1,6 +1,7 @@ """Contracts for the host-integrated Linux AppImage release path.""" import os +import shlex import shutil import subprocess from pathlib import Path @@ -76,6 +77,8 @@ def test_thin_appimage_never_injects_a_partial_desktop_runtime(): ): assert library in script assert "sudo apt install libayatana-appindicator3-1 libwebkit2gtk-4.1-0 libgtk-3-0" in script + assert "libayatana-appindicator3.so.1 libappindicator3.so.1" in script + assert "libayatana-appindicator3.so libappindicator3.so" in script assert "zenity --error" in script assert "xmessage -center" in script @@ -200,8 +203,19 @@ def test_thin_appimage_preserves_host_xdg_data_dirs(tmp_path): apprun = app_dir / "AppRun" command = [apprun, "-c", 'printf %s "$XDG_DATA_DIRS"'] + fake_bin = tmp_path / "fake-bin" + fake_bin.mkdir() + ldconfig = fake_bin / "ldconfig" + ldconfig.write_text("#!/bin/sh\nexit 0\n", encoding = "utf-8") + ldconfig.chmod(0o755) + host_libraries = tmp_path / "host-libraries" + host_libraries.mkdir() + (host_libraries / "libayatana-appindicator3.so.1").symlink_to("/bin/sh") + default_env = os.environ.copy() default_env.pop("XDG_DATA_DIRS", None) + default_env["PATH"] = f"{fake_bin}:{default_env['PATH']}" + default_env["LD_LIBRARY_PATH"] = str(host_libraries) default_result = subprocess.run( command, env = default_env, @@ -222,6 +236,191 @@ def test_thin_appimage_preserves_host_xdg_data_dirs(tmp_path): assert custom_result.stdout == f"{app_dir}/usr/share:/opt/share:/srv/share" +def _fake_library_host(tmp_path, ldconfig_lines = ()): + """Stub the tools AppRun probes so the build machine's own libraries don't count. + + The AppImage binary always resolves; any other library resolves only when a + test planted it under tmp_path. Without this the default-directory search + would find the real AppIndicator on any developer or CI box. + """ + fake_bin = tmp_path / "fake-bin" + fake_bin.mkdir() + ldd = fake_bin / "ldd" + ldd.write_text( + "#!/bin/sh\n" + 'case "$1" in\n' + " */usr/bin/unsloth-studio) exit 0 ;;\n" + ' ./*) [ -e "$1" ] && exit 0 || exit 1 ;;\n' + " " + str(tmp_path) + '/*) [ -e "$1" ] && exit 0 || exit 1 ;;\n' + " *) exit 1 ;;\n" + "esac\n", + encoding = "utf-8", + ) + cache = "".join(f"printf '%s\\n' {shlex.quote(line)}\n" for line in ldconfig_lines) + (fake_bin / "ldconfig").write_text(f"#!/bin/sh\n{cache}exit 0\n", encoding = "utf-8") + for program in ("zenity", "xmessage"): + (fake_bin / program).write_text("#!/bin/sh\nexit 0\n", encoding = "utf-8") + for stub in fake_bin.iterdir(): + stub.chmod(0o755) + return fake_bin + + +def _run_apprun( + app_dir, + fake_bin, + library_path, + *, + path = None, +): + return subprocess.run( + [app_dir / "AppRun", "-c", "exit 0"], + env = { + **os.environ, + "APPDIR": str(app_dir), + "LD_LIBRARY_PATH": str(library_path), + "PATH": path or f"{fake_bin}:{os.environ['PATH']}", + }, + check = False, + capture_output = True, + text = True, + ) + + +def test_thin_appimage_reports_a_missing_dynamic_tray_library(tmp_path): + _, app_dir, _ = _build_fake_appimage(tmp_path) + result = _run_apprun(app_dir, _fake_library_host(tmp_path), tmp_path / "missing-libraries") + + assert result.returncode == 127 + assert "libayatana-appindicator3.so.1 or libappindicator3.so.1" in result.stderr + assert "sudo apt install libayatana-appindicator3-1" in result.stderr + + +# libappindicator-sys falls back from the versioned sonames to the unversioned +# ones, so rejecting those two would refuse a host whose tray works. +def test_thin_appimage_accepts_every_tray_library_name_the_loader_tries(tmp_path): + _, app_dir, _ = _build_fake_appimage(tmp_path) + fake_bin = _fake_library_host(tmp_path) + + for library_name in ( + "libayatana-appindicator3.so.1", + "libappindicator3.so.1", + "libayatana-appindicator3.so", + "libappindicator3.so", + ): + library_dir = tmp_path / f"host-{library_name}" + library_dir.mkdir() + (library_dir / library_name).symlink_to("/bin/sh") + + result = _run_apprun(app_dir, fake_bin, library_dir) + + assert result.returncode == 0, f"{library_name}: {result.stderr}" + + +def test_thin_appimage_matches_loader_library_path_separators_and_empty_components(tmp_path): + _, app_dir, _ = _build_fake_appimage(tmp_path) + fake_bin = _fake_library_host(tmp_path) + library_dir = tmp_path / "host-libraries" + library_dir.mkdir() + (library_dir / "libayatana-appindicator3.so.1").symlink_to("/bin/sh") + + for library_path in ( + f"{tmp_path / 'missing'};{library_dir}", + f"{tmp_path / 'missing'}::{library_dir}", + f"{tmp_path / 'missing'}:", + ): + if library_path.endswith(":"): + working_dir = tmp_path / "working-directory" + working_dir.mkdir() + (working_dir / "libayatana-appindicator3.so.1").symlink_to("/bin/sh") + else: + working_dir = tmp_path + + result = subprocess.run( + [app_dir / "AppRun", "-c", "exit 0"], + cwd = working_dir, + env = { + **os.environ, + "APPDIR": str(app_dir), + "LD_LIBRARY_PATH": str(library_path), + "PATH": f"{fake_bin}:{os.environ['PATH']}", + }, + check = False, + capture_output = True, + text = True, + ) + + assert result.returncode == 0, f"{library_path}: {result.stderr}" + + +# The loader commits to the first file it finds for a name, so a broken copy +# ahead of a working one must fail rather than skip ahead. +def test_thin_appimage_stops_at_a_broken_library_path_candidate(tmp_path): + _, app_dir, _ = _build_fake_appimage(tmp_path) + fake_bin = _fake_library_host(tmp_path) + + broken_dir = tmp_path / "broken-libraries" + broken_dir.mkdir() + (broken_dir / "libayatana-appindicator3.so.1").symlink_to("/bin/sh") + working_dir = tmp_path / "working-libraries" + working_dir.mkdir() + (working_dir / "libayatana-appindicator3.so.1").symlink_to("/bin/sh") + + ldd = fake_bin / "ldd" + ldd.write_text( + "#!/bin/sh\n" + 'case "$1" in\n' + " */usr/bin/unsloth-studio) exit 0 ;;\n" + " " + str(broken_dir) + "/*) printf '\\tlibmissing.so.0 => not found\\n'; exit 0 ;;\n" + " " + str(tmp_path) + '/*) [ -e "$1" ] && exit 0 || exit 1 ;;\n' + " *) exit 1 ;;\n" + "esac\n", + encoding = "utf-8", + ) + ldd.chmod(0o755) + + result = _run_apprun(app_dir, fake_bin, f"{broken_dir}:{working_dir}") + + assert result.returncode == 127 + assert "libayatana-appindicator3.so.1 or libappindicator3.so.1" in result.stderr + + +def test_thin_appimage_accepts_a_readable_tray_library_when_ldd_is_unavailable(tmp_path): + _, app_dir, _ = _build_fake_appimage(tmp_path) + fake_bin = _fake_library_host(tmp_path) + (fake_bin / "ldd").unlink() + sed = fake_bin / "sed" + sed.write_text("#!/bin/sh\nexit 0\n", encoding = "utf-8") + sed.chmod(0o755) + library_dir = tmp_path / "host-libraries" + library_dir.mkdir() + (library_dir / "libayatana-appindicator3.so.1").symlink_to("/bin/sh") + + result = _run_apprun(app_dir, fake_bin, library_dir, path = str(fake_bin)) + + assert result.returncode == 0, result.stderr + + +# The cache is how a library outside LD_LIBRARY_PATH and the default directories +# gets found, so keep the ldconfig -p parsing covered. +def test_thin_appimage_finds_a_tray_library_through_the_ldconfig_cache(tmp_path): + _, app_dir, _ = _build_fake_appimage(tmp_path) + cached_dir = tmp_path / "cached-libraries" + cached_dir.mkdir() + cached_library = cached_dir / "libappindicator3.so.1" + cached_library.symlink_to("/bin/sh") + fake_bin = _fake_library_host( + tmp_path, + ldconfig_lines = ( + "\tlibunrelated.so.1 (libc6,x86-64) => /usr/lib/libunrelated.so.1", + f"\tlibappindicator3.so.1 (libc6,x86-64) => {cached_library}", + ), + ) + + result = _run_apprun(app_dir, fake_bin, tmp_path / "missing-libraries") + + assert result.returncode == 0, result.stderr + + def test_release_notes_keep_existing_appimage_guidance(): notes = _workflow()["env"]["DESKTOP_RELEASE_NOTES"] assert "`.AppImage` is experimental." in notes @@ -230,17 +429,25 @@ def test_release_notes_keep_existing_appimage_guidance(): assert "--appimage-extract-and-run" not in notes -def test_thin_appimage_preserves_user_runtime_environment_for_child_processes(): +def test_thin_appimage_preserves_host_libraries_and_scrubs_python_overrides(): source_root = REPO_ROOT / "studio" / "src-tauri" / "src" - child_process_sources = ( - source_root / "commands.rs", - source_root / "desktop_auth.rs", - source_root / "install.rs", - source_root / "preflight" / "managed.rs", - source_root / "process.rs", - source_root / "update.rs", - ) + process_source = (source_root / "process.rs").read_text(encoding = "utf-8") + child_process_calls = { + source_root / "commands.rs": ("scrub_appimage_python_env_tokio(&mut cmd)", 1), + source_root / "desktop_auth.rs": ("scrub_appimage_python_env_tokio(&mut cmd)", 1), + source_root / "install.rs": ("scrub_appimage_python_env(&mut cmd)", 1), + source_root / "preflight" / "managed.rs": ( + "scrub_appimage_python_env_tokio(&mut cmd)", + 2, + ), + source_root / "process.rs": ("scrub_appimage_python_env(&mut cmd)", 1), + source_root / "update.rs": ("scrub_appimage_python_env(&mut cmd)", 1), + } - for source_path in child_process_sources: + assert process_source.count('cmd.env_remove("PYTHONHOME")') == 2 + assert process_source.count('cmd.env_remove("PYTHONPATH")') == 2 + + for source_path, (call, expected) in child_process_calls.items(): source = source_path.read_text(encoding = "utf-8") - assert 'var_os("APPIMAGE")' not in source, source_path + assert source.count(call) == expected, source_path + assert 'cmd.env_remove("LD_LIBRARY_PATH")' not in source, source_path