fix(windows): conditional DACL re-stamp + damaged-children repair, gated on the adoption-level owner-only predicate - #1684
Closed
DeusData wants to merge 7 commits into
Closed
fix(windows): conditional DACL re-stamp + damaged-children repair, gated on the adoption-level owner-only predicate#1684DeusData wants to merge 7 commits into
DeusData wants to merge 7 commits into
Conversation
win_runtime_directory_secure called set_security_info with PROTECTED_DACL_SECURITY_INFORMATION on every process start, whether or not anything needed repairing. Line 4126 computes `created`, but it only guards the ERROR_ALREADY_EXISTS check - the re-stamp itself ran unconditionally. Two costs, both observed rather than theorised: #1601 counted ELEVEN "Security change" USN records against a single _config.db in one day. Windows propagates a directory's security descriptor to its children, so a rewrite that changes nothing still churns every file underneath. #1620 loses its atomic publish to this. MoveFileEx needs DELETE on the destination, and a concurrent re-protect of the parent is a window in which it can be refused - for a state that was about to be correct anyway. That reporter proved the interaction by running a background icacls loop during indexing and watching the identical index succeed. The repair is what matters, not the ritual. When the owner is already the exact current user AND the DACL already passes the private-directory check, there is nothing to fix and the right action is to leave it alone. When it IS wrong, the repair is byte-for-byte what it was before. This does not fix the underlying ACL damage on already-broken installs - files created under the pre-v0.10.3 flagless regime still carry empty DACLs and need a child repair, which is a separate change. It stops us from making it worse and from creating a failure window on every start. Costs one extra GetSecurityInfo to avoid a SetSecurityInfo, which is the cheap direction. Builds clean; daemon_ipc 48 passed. The changed region is Windows-only, so the real verification is the Windows CI leg. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The previous commit called win_file_security_secure with five arguments; on this branch it takes four. That is a compile error on Windows. It passed the local build because the entire win_* region is Windows-only and guarded out on macOS, so a clean macOS build says nothing about it. Worth recording as the trap it is: for Windows-only code, a green host build is not evidence, and the Windows CI leg is the first thing that can actually reject it. (The five-argument form belongs to #1623, which threads an ancestor flag through this function. When that lands, this call becomes the five-argument form again on rebase.) Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
…ignature #1623 landed on main and threads an ancestor flag through win_file_security_secure. The conditional check added here predates it and still passed four arguments; both calls now pass ancestor=false, which is correct - the private runtime directory is never an ancestor and must keep full strictness. This is the second arity correction on this branch, in opposite directions, and the reason is worth stating: the win_* region is Windows-only, so a local macOS build compiles none of it and reports success regardless. Nothing on this machine can catch a mismatched call here. The Windows CI leg is the only venue that can. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
…old DACL regime Between v0.9.1-rc and v0.10.2 the runtime directory carried a PROTECTED DACL whose ACE was not inheritable, so Windows gave every file created inside it either an empty DACL or the token default (SYSTEM + TokenOwner + logon SID). Under an elevated token TokenOwner is BUILTIN\Administrators, so the interactive user is left with no durable grant and the file becomes unreadable after the next logon. #1601: takeown and icacls both fail non-elevated, and the daemon can no longer open _config.db. now, so new children are fine. Nothing repaired the children already damaged, which is why upgrading rescued nobody whose cache was written under the old regime. Their only route back was a rename-the-parent dance the reporter worked out themselves. The repair runs after the directory is confirmed good, and is deliberately narrow: immediate children only, no recursion, capped at 4096, regular files only - directories, reparse points and symlinks are skipped rather than followed. A child is touched ONLY when demonstrably damaged: an empty DACL, or an owner that is not the current user. Failures are counted and reported, never fatal, because this runs inside daemon startup. Empty-DACL detection needs its own test and cannot reuse the validator: win_file_acl_secure scans ACEs for untrusted mutation grants, and a DACL with zero ACEs trivially has none - so the damage reads as compliance. It only ever touches cbm's own runtime/cache directory, which we created and own; it does not reach into user directories. Windows-only code. A clean macOS build says nothing about it - the previous commit on this branch shipped a five-argument call to a four-argument function and built green here - so the Windows CI leg is the first venue that can actually reject this. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
…-only DACL The conditional re-stamp skipped set_security_info whenever the runtime directory passed the general secure() predicate — but that predicate admits SYSTEM/Administrators ACEs, while lock-directory ADOPTION (private_win_owner_only_dacl) demands the exact protected owner-only descriptor the stamp writes. A fresh directory with an inherited DACL therefore skipped the stamp and stranded every subsequent lock adoption: 59 daemon-suite failures on the real Windows VM, 77/77 green on main. The fast path now applies the consumer's strictest predicate (ported ACE-for- ACE from the foundation layer, incl. SE_DACL_PROTECTED and the non-inherited single-ACE shape), so it only skips work when skipping is actually safe. Debug instrumentation from the diagnosis is removed. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This was referenced Aug 17, 2026
Owner
Author
|
Superseded by the DCO-clean single-commit variant with the byte-identical tree (the diagnosis instrumentation commits here lacked sign-offs). Same VM verification applies: canaries 77/77, full test-par 7346/0. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #1634 and #1636 (same commits, rebased onto current main, plus the fix their Windows CI failures were pointing at).
The stack (unchanged in intent): re-stamp the runtime DACL only when it is actually wrong (#1634 — stops the daily USN "Security change" storms of #1601 and the MoveFileEx refusal window of #1620), and detect/repair cache children left unusable by the pre-v0.10.3 DACL regime (#1636 — the 0-byte-unreadable-log class behind #1416's diagnosis).
What the old PRs' red CI was: real. On the actual Windows VM the branch failed 59/77 daemon-suite tests (main: 77/77). Root cause, found by instrumenting the VM build: the conditional-re-stamp fast path declared a directory "already correct" using the general secure() predicate — which admits SYSTEM/Administrators ACEs — while lock-directory ADOPTION (
private_win_owner_only_dacl) demands the exact protected owner-only single-ACE descriptor the stamp writes. A fresh runtime dir with an inherited DACL skipped the stamp and stranded every subsequent lock adoption. The fast path now applies the consumer's strictest predicate, ported ACE-for-ACE (SE_DACL_PROTECTED, non-inherited single ACE, owner SID, FILE_ALL_ACCESS/GENERIC_ALL), so it skips work only when skipping is safe.Verification (real arm64 Windows 11 VM): the three canary suites went 18/59-failed → 77/77; full
test-par7346 passed, 0 failed (onegrammar_probe_c900s wall-clock hang, cleared on retry — the known grammar_probe flake family, pre-recorded). macOS + Linux compile the POSIX paths untouched.🤖 Generated with Claude Code
https://claude.ai/code/session_018RiRAw9RQhvCoshqe7eZHV