From 714fbed0fec2b241064eface28dd1455e87bf8e5 Mon Sep 17 00:00:00 2001 From: lennix1337 <28357644+lennix1337@users.noreply.github.com> Date: Sat, 24 Oct 2026 15:45:28 -0300 Subject: [PATCH 1/3] fix(cache): skip chmod when the cache dir is already 0700 On Win32 chmod re-propagates inheritable ACLs through the whole cache tree even when the mode is unchanged, which made warm runs on a 38k-file repo take ~2 s instead of ~18 ms. Regression: test/cache_ladder_windows.py and the cacheisolationcheck ACL arm. --- src/quality.h | 5 +- test/cache_ladder_windows.py | 160 +++++++++++++++++++++++++++++++++++ test/cacheisolationcheck.sh | 33 ++++++-- 3 files changed, 192 insertions(+), 6 deletions(-) create mode 100644 test/cache_ladder_windows.py diff --git a/src/quality.h b/src/quality.h index 16c12f8d6..a3bb7241d 100644 --- a/src/quality.h +++ b/src/quality.h @@ -1869,7 +1869,10 @@ inline std::string cacheDirLadder() os::stat_t st {}; if( mkdirRc == 0 || ( os::lstat( d.c_str(), &st ) == 0 && S_ISDIR( st.st_mode ) && st.st_uid == os::getuid() ) ) { - if( os::chmod( d.c_str(), 0700 ) == 0 + // Win32 chmod propagates inheritable ACLs through the whole cache tree, even when unchanged. + // lstat already verifies a protected owner-only DACL; repair only a missing/private-mode mismatch. + const bool alreadyOwnerOnly = ( st.st_mode & 0777 ) == 0700; + if( ( alreadyOwnerOnly || os::chmod( d.c_str(), 0700 ) == 0 ) && os::lstat( d.c_str(), &st ) == 0 && S_ISDIR( st.st_mode ) && st.st_uid == os::getuid() && ( st.st_mode & 0777 ) == 0700 ) { diff --git a/test/cache_ladder_windows.py b/test/cache_ladder_windows.py new file mode 100644 index 000000000..44a4f27d3 --- /dev/null +++ b/test/cache_ladder_windows.py @@ -0,0 +1,160 @@ +"""Native Windows cache ACL regression; invoked by cacheisolationcheck.sh. + +Observe NTFS ChangeTime, not timing thresholds: reapplying an unchanged DACL +changes it and propagates inheritable ACEs through the entire cache tree. +""" +import ctypes +from ctypes import wintypes +import os +from pathlib import Path +import re +import subprocess +import sys +import tempfile +import unittest + + +class FileBasicInfo(ctypes.Structure): + _fields_ = [(name, ctypes.c_longlong) for name in + ("creation", "access", "write", "change")] + [("attributes", wintypes.DWORD)] + + +def change_time(path): + kernel = ctypes.WinDLL("kernel32", use_last_error=True) + kernel.CreateFileW.argtypes = [wintypes.LPCWSTR, wintypes.DWORD, wintypes.DWORD, + ctypes.c_void_p, wintypes.DWORD, wintypes.DWORD, wintypes.HANDLE] + kernel.CreateFileW.restype = wintypes.HANDLE + kernel.GetFileInformationByHandleEx.argtypes = [wintypes.HANDLE, ctypes.c_int, + ctypes.c_void_p, wintypes.DWORD] + kernel.GetFileInformationByHandleEx.restype = wintypes.BOOL + kernel.CloseHandle.argtypes = [wintypes.HANDLE] + kernel.CloseHandle.restype = wintypes.BOOL + handle = kernel.CreateFileW(str(path), 0x80, 7, None, 3, 0x02000000, None) + if handle == wintypes.HANDLE(-1).value: + raise ctypes.WinError(ctypes.get_last_error()) + try: + info = FileBasicInfo() + if not kernel.GetFileInformationByHandleEx(handle, 0, ctypes.byref(info), ctypes.sizeof(info)): + raise ctypes.WinError(ctypes.get_last_error()) + return info.change + finally: + if not kernel.CloseHandle(handle): + raise ctypes.WinError(ctypes.get_last_error()) + + +def dacl_sddl(path): + security = ctypes.WinDLL("advapi32", use_last_error=True) + kernel = ctypes.WinDLL("kernel32", use_last_error=True) + security.GetNamedSecurityInfoW.argtypes = [wintypes.LPWSTR, ctypes.c_int, wintypes.DWORD, + ctypes.c_void_p, ctypes.c_void_p, ctypes.c_void_p, + ctypes.c_void_p, ctypes.POINTER(ctypes.c_void_p)] + security.GetNamedSecurityInfoW.restype = wintypes.DWORD + security.ConvertSecurityDescriptorToStringSecurityDescriptorW.argtypes = [ + ctypes.c_void_p, wintypes.DWORD, wintypes.DWORD, ctypes.POINTER(wintypes.LPWSTR), ctypes.c_void_p] + security.ConvertSecurityDescriptorToStringSecurityDescriptorW.restype = wintypes.BOOL + kernel.LocalFree.argtypes = [ctypes.c_void_p] + kernel.LocalFree.restype = ctypes.c_void_p + descriptor = ctypes.c_void_p() + error = security.GetNamedSecurityInfoW(str(path), 1, 4, None, None, None, None, + ctypes.byref(descriptor)) + if error: + raise ctypes.WinError(error) + text = wintypes.LPWSTR() + try: + if not security.ConvertSecurityDescriptorToStringSecurityDescriptorW( + descriptor, 1, 4, ctypes.byref(text), None): + raise ctypes.WinError(ctypes.get_last_error()) + return text.value + finally: + if text: + kernel.LocalFree(ctypes.cast(text, ctypes.c_void_p)) + kernel.LocalFree(descriptor) + + +OWNER_CLASS_TRUSTEES = {"OW", "BA", "SY"} + + +def owner_only(path): + """Windows counterpart of mode 0700: protected DACL whose allowed ACEs name only + owner-class principals, the same rule os_win32.cpp readOwnerAndMode applies.""" + sddl = dacl_sddl(path) + dacl = sddl[sddl.index("D:"):] if "D:" in sddl else "" + if not dacl.startswith("D:P"): + return False, sddl + for ace in re.findall(r"\(([^)]*)\)", dacl): + fields = ace.split(";") + if fields[0] != "A" or fields[-1] not in OWNER_CLASS_TRUSTEES: + return False, sddl + return True, sddl + + +class CacheLadderWindows(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory(prefix="ripwire-acl-") + self.addCleanup(self.temp.cleanup) + self.root = Path(self.temp.name) + self.shared = self.root / "shared" + self.shared.mkdir() + self.fixture = self.root / "fixture" + self.fixture.mkdir() + (self.fixture / "code.ts").write_text("export function alphaOne() { return 42; }\n") + self.env = {**os.environ, "TMPDIR": str(self.shared), "RIPWIRE_CACHE_STATS": "1"} + self.cache = self.shared / "ripwire" + + def query(self): + result = subprocess.run([BINARY, str(self.fixture), "--grep=alphaOne", "--legend=compact"], + env=self.env, capture_output=True, text=True, timeout=30, check=True) + self.assertIn("alphaOne", result.stdout) + return result + + def prime(self): + self.query() + warm = self.query() + self.assertIn("reparsed=0 reused=1", warm.stderr) + return warm.stdout + + def test_warm_private_cache_does_not_reapply_acl(self): + expected = self.prime() + before = change_time(self.cache) + result = self.query() + self.assertEqual(result.stdout, expected) + self.assertIn("reparsed=0 reused=1", result.stderr) + self.assertEqual(change_time(self.cache), before, + "warm cache rewrote directory metadata (inheritable ACL propagation)") + + def test_permissive_cache_is_repaired_once(self): + expected = self.prime() + subprocess.run(["icacls.exe", str(self.cache), "/grant", "*S-1-1-0:(OI)(CI)RX"], + capture_output=True, text=True, check=True, timeout=30) + before = change_time(self.cache) + self.assertEqual(self.query().stdout, expected) + self.assertNotEqual(change_time(self.cache), before, "permissive ACL was not repaired") + repaired = dacl_sddl(self.cache) + self.assertTrue(repaired.startswith("D:P"), repaired) + self.assertNotIn(";;;WD)", repaired, "Everyone still has cache access") + stable = change_time(self.cache) + self.query() + self.assertEqual(change_time(self.cache), stable) + + def test_junction_cache_is_refused_without_touching_target(self): + target = self.root / "target" + target.mkdir() + subprocess.run(["cmd.exe", "/d", "/c", "mklink", "/J", str(self.cache), str(target)], + capture_output=True, text=True, check=True, timeout=30) + self.addCleanup(self.cache.rmdir) + before = (change_time(target), dacl_sddl(target)) + result = self.query() + self.assertIn("reparsed=1 reused=0", result.stderr) + self.assertEqual((change_time(target), dacl_sddl(target)), before) + self.assertEqual(list(target.iterdir()), []) + + +if __name__ == "__main__": + if sys.platform != "win32": + raise SystemExit("This regression requires native Windows Python") + if len(sys.argv) == 3 and sys.argv[1] == "--owner-only": + private, sddl = owner_only(sys.argv[2]) + print(sddl) + raise SystemExit(0 if private else 1) + BINARY = str(Path(sys.argv.pop(1)).resolve()) + unittest.main() diff --git a/test/cacheisolationcheck.sh b/test/cacheisolationcheck.sh index 147bf96ec..2416ebb3f 100755 --- a/test/cacheisolationcheck.sh +++ b/test/cacheisolationcheck.sh @@ -12,6 +12,23 @@ no(){ echo " FAIL $1"; fail=1; } [ -x "$BIN" ] || { echo "no ripwire binary at $BIN — build first"; exit 2; } +# Under MSYS the shell converts POSIX paths in argv and TMPDIR for a native binary, but not +# inside JSON on stdin, and GNU stat reports an emulated mode instead of the real DACL. +case "$(uname -s)" in MINGW*|MSYS*|CYGWIN*) win32=1 ;; *) win32=0 ;; esac +native_path(){ if [ "$win32" = 1 ]; then cygpath -m "$1"; else printf '%s' "$1"; fi; } +check_private_mode(){ + if [ "$win32" = 1 ]; then + if sddl="$( python "$ROOT/test/cache_ladder_windows.py" --owner-only "$( native_path "$1" )" )"; then + ok "private directory DACL is owner-only ($sddl)" + else + no "private directory DACL is not owner-only: $sddl" + fi + else + mode="$( mode_of "$1" )" + if [ "$mode" = "700" ]; then ok "private directory mode is 0700"; else no "private directory mode is $mode, expected 700"; fi + fi +} + TMP="$( mktemp -d )"; trap 'rm -rf "$TMP"' EXIT SHARED="$TMP/shared"; CORPUS="$TMP/corpus"; mkdir -p "$SHARED" "$CORPUS" mkdir -m 0755 "$SHARED/ripwire" # the binary must repair an existing permissive directory @@ -24,16 +41,13 @@ echo "cacheisolationcheck: BIN=$BIN" TMPDIR="$SHARED" "$BIN" "$CORPUS" >/dev/null 2>"$TMP/cli.err" printf '%s\n' \ '{"jsonrpc":"2.0","id":1,"method":"initialize"}' \ - '{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"replace_symbol_body","arguments":{"path":"'"$CORPUS"'","symbol":"target","new_body":"int target( void ) { return 2; }"}}}' \ + '{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"replace_symbol_body","arguments":{"path":"'"$( native_path "$CORPUS" )"'","symbol":"target","new_body":"int target( void ) { return 2; }"}}}' \ | TMPDIR="$SHARED" "$BIN" --mcp >"$TMP/mcp.out" 2>"$TMP/mcp.err" PRIVATE="$SHARED/ripwire" if [ -d "$PRIVATE" ]; then ok "creates a dedicated TMPDIR/ripwire directory"; else no "missing private directory: $PRIVATE"; fi -if [ -d "$PRIVATE" ]; then - mode="$( mode_of "$PRIVATE" )" - if [ "$mode" = "700" ]; then ok "private directory mode is 0700"; else no "private directory mode is $mode, expected 700"; fi -fi +[ -d "$PRIVATE" ] && check_private_mode "$PRIVATE" topArtifacts="$( find "$SHARED" -mindepth 1 -maxdepth 1 -name 'ripwire-*' -print 2>/dev/null )" [ -z "$topArtifacts" ] && ok "shared TMPDIR root has no ripwire-* artifacts" \ @@ -46,4 +60,13 @@ if [ "$lockCount" -ge 1 ]; then ok "edit lock is sharded under the private locks if [ -f "$SHARED/not-ripwire" ]; then ok "unrelated TMPDIR content remains untouched"; else no "unrelated TMPDIR content was removed"; fi +# The Win32 DACL repair is observable without a noisy wall-clock threshold. +if [ "$win32" = 1 ]; then + if python "$ROOT/test/cache_ladder_windows.py" "$BIN"; then + ok "native Windows ACL repair, warm no-rewrite, and junction refusal" + else + no "native Windows cache ACL regression" + fi +fi + [ "$fail" -eq 0 ] && echo "ALL PASS" || { echo "FAILURES ABOVE"; exit 1; } From 3f1a11f9a26e35424e8d6063a45f069a294db1d9 Mon Sep 17 00:00:00 2001 From: lennix1337 <28357644+lennix1337@users.noreply.github.com> Date: Sat, 24 Oct 2026 19:57:32 -0300 Subject: [PATCH 2/3] changelog: Windows cache directory ACL is no longer rewritten on warm runs --- changelog.d/win32-cache-dir-acl-skip.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog.d/win32-cache-dir-acl-skip.md diff --git a/changelog.d/win32-cache-dir-acl-skip.md b/changelog.d/win32-cache-dir-acl-skip.md new file mode 100644 index 000000000..e08b42c0c --- /dev/null +++ b/changelog.d/win32-cache-dir-acl-skip.md @@ -0,0 +1,3 @@ +### Fixed — Windows: warm runs no longer rewrite the cache directory's ACL + +- The per-user cache directory ladder skips `chmod(0700)` when `lstat` already reports an owner-only directory. On Win32 that `chmod` re-applies a protected DACL with inheritable ACEs, which Windows propagates through every file under the cache: with 38,000 files in the cache a warm run took ~2,024 ms instead of ~10.6 ms (p50 of 3, same revision, only this guard changed). A permissive directory is still repaired. Regression: `test/cache_ladder_windows.py` and the Windows arm of `test/cacheisolationcheck.sh` (thanks @lennix1337). From c8f9804a36602bf6091c5d22ab2c955421c299cf Mon Sep 17 00:00:00 2001 From: lennix1337 <28357644+lennix1337@users.noreply.github.com> Date: Sat, 24 Oct 2026 21:11:28 -0300 Subject: [PATCH 3/3] test(cache): native Windows Python under Cygwin, deny ACEs and user SID in the owner-only check, junction without cmd.exe; changelog names #436 --- ...kip.md => 436-win32-cache-dir-acl-skip.md} | 2 +- test/cache_ladder_windows.py | 24 ++++++++++++++----- test/cacheisolationcheck.sh | 18 ++++++++++++-- 3 files changed, 35 insertions(+), 9 deletions(-) rename changelog.d/{win32-cache-dir-acl-skip.md => 436-win32-cache-dir-acl-skip.md} (95%) diff --git a/changelog.d/win32-cache-dir-acl-skip.md b/changelog.d/436-win32-cache-dir-acl-skip.md similarity index 95% rename from changelog.d/win32-cache-dir-acl-skip.md rename to changelog.d/436-win32-cache-dir-acl-skip.md index e08b42c0c..158d777ea 100644 --- a/changelog.d/win32-cache-dir-acl-skip.md +++ b/changelog.d/436-win32-cache-dir-acl-skip.md @@ -1,3 +1,3 @@ ### Fixed — Windows: warm runs no longer rewrite the cache directory's ACL -- The per-user cache directory ladder skips `chmod(0700)` when `lstat` already reports an owner-only directory. On Win32 that `chmod` re-applies a protected DACL with inheritable ACEs, which Windows propagates through every file under the cache: with 38,000 files in the cache a warm run took ~2,024 ms instead of ~10.6 ms (p50 of 3, same revision, only this guard changed). A permissive directory is still repaired. Regression: `test/cache_ladder_windows.py` and the Windows arm of `test/cacheisolationcheck.sh` (thanks @lennix1337). +- The per-user cache directory ladder skips `chmod(0700)` when `lstat` already reports an owner-only directory. On Win32 that `chmod` re-applies a protected DACL with inheritable ACEs, which Windows propagates through every file under the cache: with 38,000 files in the cache a warm run took ~2,024 ms instead of ~10.6 ms (p50 of 3, same revision, only this guard changed). A permissive directory is still repaired. Regression: `test/cache_ladder_windows.py` and the Windows arm of `test/cacheisolationcheck.sh` (#436, thanks @lennix1337). diff --git a/test/cache_ladder_windows.py b/test/cache_ladder_windows.py index 44a4f27d3..47174f559 100644 --- a/test/cache_ladder_windows.py +++ b/test/cache_ladder_windows.py @@ -71,19 +71,30 @@ def dacl_sddl(path): kernel.LocalFree(descriptor) -OWNER_CLASS_TRUSTEES = {"OW", "BA", "SY"} +# isOwnerClassSid in os_win32.cpp: OWNER RIGHTS, CREATOR OWNER, Administrators, SYSTEM and this user. +OWNER_CLASS_TRUSTEES = {"OW", "CO", "BA", "SY"} + + +def current_user_sid(): + result = subprocess.run(["whoami.exe", "/user", "/fo", "csv", "/nh"], + capture_output=True, text=True, check=True, timeout=30) + return result.stdout.strip().split(",")[-1].strip('"') def owner_only(path): - """Windows counterpart of mode 0700: protected DACL whose allowed ACEs name only - owner-class principals, the same rule os_win32.cpp readOwnerAndMode applies.""" + """Windows counterpart of mode 0700: protected DACL whose allow ACEs name only + owner-class principals, the same rule os_win32.cpp readOwnerAndMode applies + (deny ACEs only take access away, so they never make a DACL less private).""" sddl = dacl_sddl(path) dacl = sddl[sddl.index("D:"):] if "D:" in sddl else "" if not dacl.startswith("D:P"): return False, sddl + trustees = OWNER_CLASS_TRUSTEES | {current_user_sid()} for ace in re.findall(r"\(([^)]*)\)", dacl): fields = ace.split(";") - if fields[0] != "A" or fields[-1] not in OWNER_CLASS_TRUSTEES: + if fields[0] == "D": + continue + if fields[0] != "A" or fields[-1] not in trustees: return False, sddl return True, sddl @@ -139,8 +150,9 @@ def test_permissive_cache_is_repaired_once(self): def test_junction_cache_is_refused_without_touching_target(self): target = self.root / "target" target.mkdir() - subprocess.run(["cmd.exe", "/d", "/c", "mklink", "/J", str(self.cache), str(target)], - capture_output=True, text=True, check=True, timeout=30) + # No command shell: a temp path holding cmd.exe metacharacters (&, ^) stays one argument. + import _winapi # Windows-only CPython module; the platform guard runs before the tests + _winapi.CreateJunction(str(target), str(self.cache)) self.addCleanup(self.cache.rmdir) before = (change_time(target), dacl_sddl(target)) result = self.query() diff --git a/test/cacheisolationcheck.sh b/test/cacheisolationcheck.sh index 2416ebb3f..e4d416966 100755 --- a/test/cacheisolationcheck.sh +++ b/test/cacheisolationcheck.sh @@ -16,9 +16,23 @@ no(){ echo " FAIL $1"; fail=1; } # inside JSON on stdin, and GNU stat reports an emulated mode instead of the real DACL. case "$(uname -s)" in MINGW*|MSYS*|CYGWIN*) win32=1 ;; *) win32=0 ;; esac native_path(){ if [ "$win32" = 1 ]; then cygpath -m "$1"; else printf '%s' "$1"; fi; } +# The DACL helper needs a native Windows Python: Cygwin's own python reports sys.platform "cygwin". +WINPY="" +if [ "$win32" = 1 ]; then + for candidate in python py python3; do + if command -v "$candidate" >/dev/null 2>&1 \ + && "$candidate" -c 'import sys; sys.exit(sys.platform != "win32")' 2>/dev/null; then + WINPY="$candidate"; break + fi + done +fi +winpy(){ + if [ -z "$WINPY" ]; then echo "no native Windows Python (python, py or python3 with sys.platform win32) on PATH"; return 1; fi + "$WINPY" "$( native_path "$ROOT/test/cache_ladder_windows.py" )" "$@" +} check_private_mode(){ if [ "$win32" = 1 ]; then - if sddl="$( python "$ROOT/test/cache_ladder_windows.py" --owner-only "$( native_path "$1" )" )"; then + if sddl="$( winpy --owner-only "$( native_path "$1" )" )"; then ok "private directory DACL is owner-only ($sddl)" else no "private directory DACL is not owner-only: $sddl" @@ -62,7 +76,7 @@ if [ -f "$SHARED/not-ripwire" ]; then ok "unrelated TMPDIR content remains untou # The Win32 DACL repair is observable without a noisy wall-clock threshold. if [ "$win32" = 1 ]; then - if python "$ROOT/test/cache_ladder_windows.py" "$BIN"; then + if winpy "$( native_path "$BIN" )"; then ok "native Windows ACL repair, warm no-rewrite, and junction refusal" else no "native Windows cache ACL regression"