diff --git a/lib/dlc_paths.py b/lib/dlc_paths.py index 0202a25..1c61041 100644 --- a/lib/dlc_paths.py +++ b/lib/dlc_paths.py @@ -14,6 +14,7 @@ import os from pathlib import Path import appstate +from safepath import resolved_root def _get_dlc_dir(cfg: dict | None = None) -> Path | None: @@ -86,7 +87,14 @@ def _resolve_dlc_path(dlc: Path, filename: str) -> Path | None: or PureWindowsPath(safe).drive): return None try: - root = dlc.resolve() + # The library root is fixed for the life of the process, but this + # function runs once per song / art fetch / scanned row — and + # `.resolve()` lstats every path component. Re-resolving here was + # ~23,500 stat calls/sec on a 50,944-song library, which pins a core + # when the library sits on a FUSE mount (NTFS-3G, SMB, sshfs) where each + # stat is a userspace round trip. Resolve the root once; see + # safepath.resolved_root for the caching contract. + root = resolved_root(dlc) # normpath collapses `.`/`..`/duplicate separators purely lexically — # it never touches the filesystem, so an in-library junction component # is preserved (allowed) while `..`/absolute segments still escape and diff --git a/lib/safepath.py b/lib/safepath.py index 4442e11..d1eefc0 100644 --- a/lib/safepath.py +++ b/lib/safepath.py @@ -4,9 +4,34 @@ under a server-owned root. from __future__ import annotations +from functools import lru_cache from pathlib import Path +@lru_cache(maxsize=16) +def resolved_root(root: Path) -> Path: + """Canonical (link-resolved) form of a server-owned root directory. + + ``Path.resolve()`` is a filesystem call: it lstats every component of the + path. The roots we join against — the DLC library, a plugin's asset dir — + are fixed for the life of the process, but the containment helpers below + (and ``dlc_paths._resolve_dlc_path``) were re-resolving them on EVERY call, + and those are called once per song, per art fetch, per scanned row. + + On a real 50,944-song library that cost ~23,500 stat/lstat calls per second, + pinning a core. It is brutal when the library lives on a FUSE mount + (NTFS-3G, SMB, sshfs), where every stat is a userspace round trip: the same + three parent directories were being walked over and over. + + Cached because a root is a constant here, not because resolution is cheap. + Consequence: if a root's symlink/junction is re-pointed at a NEW target + while the server is running, the old target stays in effect until restart. + That is fine for a library path fixed at startup, and the cache is keyed on + the Path, so switching to a different library dir is a different key. + """ + return root.resolve() + + def safe_join(root: Path, name: str) -> Path | None: """Resolve ``name`` under ``root`` and return the resolved Path, or ``None`` if it would escape ``root`` or is unrepresentable. @@ -35,9 +60,12 @@ def safe_join(root: Path, name: str) -> Path | None: return None safe = name.replace("\\", "/") try: - root_resolved = root.resolve() - candidate = (root_resolved / safe).resolve() - if not candidate.is_relative_to(root_resolved): + # The ROOT is a constant — resolve it once (see resolved_root). The + # CANDIDATE must still be resolved on every call: following its symlinks + # is exactly the zip-slip / traversal defence, so it is never cached. + root_res = resolved_root(root) + candidate = (root_res / safe).resolve() + if not candidate.is_relative_to(root_res): return None except (ValueError, OSError): return None diff --git a/tests/test_resolved_root_cache.py b/tests/test_resolved_root_cache.py new file mode 100644 index 0000000..41b1782 --- /dev/null +++ b/tests/test_resolved_root_cache.py @@ -0,0 +1,103 @@ +"""The library root must be resolved ONCE, not on every path check. + +`Path.resolve()` lstats every component of a path. `_resolve_dlc_path` and +`safe_join` run once per song / art fetch / scanned row, and both used to +re-resolve their root every single call. + +Measured on a real 50,944-song library sitting on an NTFS-3G (FUSE) mount: +~23,500 stat/lstat calls per second, re-walking the same three parent +directories, pinning a core of the server. Every stat crosses into userspace on +FUSE, so the constant re-resolution — not the work itself — was the cost. + +These tests pin the fix (root resolved once) AND that caching it did not weaken +containment, which is the thing that matters: `safe_join` is the zip-slip guard. +""" + +from pathlib import Path + +import pytest + +from dlc_paths import _resolve_dlc_path +from safepath import resolved_root, safe_join + + +@pytest.fixture(autouse=True) +def _clear_cache(): + resolved_root.cache_clear() + yield + resolved_root.cache_clear() + + +def test_dlc_root_is_resolved_once_across_many_lookups(tmp_path): + """The regression: 500 lookups must not mean 500 root resolutions.""" + (tmp_path / "a.feedpak").write_bytes(b"x") + + for i in range(500): + assert _resolve_dlc_path(tmp_path, f"song{i}.feedpak") is not None + + info = resolved_root.cache_info() + assert info.misses == 1, ( + f"the library root must be resolved ONCE, not per call " + f"(got {info.misses} resolutions for 500 lookups)" + ) + assert info.hits == 499 + + +def test_safe_join_resolves_its_root_once_too(tmp_path): + for i in range(200): + assert safe_join(tmp_path, f"asset{i}.png") is not None + assert resolved_root.cache_info().misses == 1 + + +def test_a_different_root_is_a_different_cache_entry(tmp_path): + other = tmp_path / "other" + other.mkdir() + _resolve_dlc_path(tmp_path, "a.feedpak") + _resolve_dlc_path(other, "a.feedpak") + assert resolved_root.cache_info().misses == 2, "switching library dir must re-resolve" + + +# ── containment must be unchanged (the part that matters) ─────────────────── + +@pytest.mark.parametrize("evil", [ + "../etc/passwd", + "..\\etc\\passwd", + "a/../../etc/passwd", + "/etc/passwd", + "C:/Windows/system.ini", + "", +]) +def test_resolve_dlc_path_still_rejects_escapes(tmp_path, evil): + assert _resolve_dlc_path(tmp_path, evil) is None + + +@pytest.mark.parametrize("evil", [ + "../outside.txt", + "..\\outside.txt", + "a/../../outside.txt", + "", +]) +def test_safe_join_still_rejects_escapes(tmp_path, evil): + assert safe_join(tmp_path, evil) is None + + +def test_safe_join_still_follows_symlinks_out(tmp_path): + """safe_join's candidate resolution is the zip-slip defence and is NOT cached: + a symlink pointing outside the root must still be refused.""" + outside = tmp_path.parent / "outside_secret" + outside.mkdir(exist_ok=True) + (outside / "secret.txt").write_text("x") + + root = tmp_path / "root" + root.mkdir() + (root / "escape").symlink_to(outside) + + assert safe_join(root, "escape/secret.txt") is None, ( + "a symlink escaping the root must still be rejected — caching the ROOT " + "must not disable resolution of the CANDIDATE" + ) + + +def test_in_library_paths_still_resolve(tmp_path): + assert _resolve_dlc_path(tmp_path, "sub/song.feedpak") == tmp_path / "sub" / "song.feedpak" + assert safe_join(tmp_path, "art/cover.png") == (tmp_path / "art" / "cover.png").resolve()