Repository navigation
Fix Windows defects: AppContainer sandbox spawn, token DACL, release-gate scripts - #16
metaphorics wants to merge 8 commits into
Conversation
…ontract - dalgon sandbox: spawn via quoted argv0 (lpApplicationName NULL under AppContainer), plant writable-root ACE + Low label via O(1) object writes, restore on lift; refuse MSYS/cygwin targets legibly (PE import preflight + 0xC0000142 post-exit mapping) - dal-wire: serve token gets protected owner+SYSTEM+Administrators DACL on Windows (0600 parity); crate lint forbid->deny with named mod win exception - gates/release_support: resolve Git Bash before PATH bash (WSL launcher cannot open C:\ paths); normalize CRLF streams - scripts: accept python3 or python; .gitattributes pins *.sh to LF
…se-guard literal - Traverse grants and revokes on directories write the object DACL alone through NtSetSecurityObject: SetNamedSecurityInfoW re-propagates every inheritable ACE on the directory through its whole subtree, which cost ~40s per call on C:\Users and made the sandbox gate time out. - record orig_label on first write access and restore it on lift so the Low mandatory label does not outlive the run - skip PATH directories the container can already read through ALL APPLICATION PACKAGES; otherwise plant traverse on the directory, the executable, and a bounded set of runnable files - preflight refuses executables importing msys-*/cygwin* and maps the 0xC0000142/66 early-init death to a legible sandbox refusal - lpApplicationName stays NULL so argv0 resolves through the normal path under the container token - release_guards: expect the repaired "release dal first" literal from upstream ee66c0a
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
Original prompt from a
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1bde28466
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(dll) = posix_runtime_import(Path::new(executable)) { | ||
| return Err(format!( | ||
| "dalgon sandbox: {} loads {dll}; MSYS/cygwin runtimes cannot run inside the Windows sandbox (AppContainer namespaces are private).", | ||
| executable.to_string_lossy() | ||
| )); |
There was a problem hiding this comment.
On Windows with sandbox = true and no custom shell, dal-tools/src/exec/shell.rs::default_windows_ladder selects Git's bin\bash.exe, which imports msys-2.0.dll; this new preflight therefore rejects the executable before every default exec call reaches CreateProcessW. The existing Windows gate does not catch this because it only attempts a denied removal and accepts any error containing dalgon sandbox, so the default sandbox configuration now makes all commands fail with exit 126.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The refusal does not introduce the failure — it makes a pre-existing one legible. MSYS/cygwin cannot initialize inside a classic AppContainer: the container gets a private \BaseNamedObjects namespace, so cygwin1/msys-2.0 early init dies with STATUS_DLL_INIT_FAILED (0xC0000142) before main. Before this preflight, the same default-shell exec already failed every call — just with an opaque exit code that read like a denied exec instead of an architecture constraint. Dropping the check would return exit 126/0xC0000142 with no explanation rather than restore a working shell.
The real gap is upstream of this: default_windows_ladder has no usable POSIX shell to offer the Windows sandbox at all. That is a product-level decision (e.g. route default exec through cmd.exe/powershell, or ship a sandboxable shell) rather than something this defect fix can invent here — it is recorded as a decision row in the spec repo so the shell contract gets designed deliberately instead of silently pointing at an executable that cannot run.
| }) | ||
| .take(PATH_GRANT_LIMIT) | ||
| .collect() |
There was a problem hiding this comment.
For a PATH directory without a usable ALL APPLICATION PACKAGES grant and more than 128 matching files, read_dir's unspecified enumeration order determines which programs and DLLs receive an ACE. Any command or loader dependency outside that arbitrary prefix remains inaccessible to the AppContainer, so ordinary PATH resolution fails depending on directory layout. Grant the required executable set without silently dropping entries, or resolve and grant each requested program on demand.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping the bound — removing it reintroduces the regression this PR fixes. I tested the unbounded form on this box: PATH dirs that are not packages-readable (Git usr\bin/mingw64\bin carry hundreds of matching files) each need a per-file object-only DACL write at ~30ms, and the first __sandbox plant ran past two minutes — the same "spawn takes tens of seconds" failure mode as the SetNamedSecurityInfoW propagation this PR removes. The cap keeps plant latency bounded and predictable.
On the other half of your suggestion: the invoked executable always gets its own non-optional grant, so the tool the caller actually named can never fall outside the prefix — what the prefix can miss is a co-located DLL or secondary binary past entry 128, an acknowledged limitation. Grant-on-demand for load-time dependencies cannot be done lazily from the parent (the dependency is loaded by the already-running container child, which cannot self-grant), and a propagated directory ACE is exactly the minutes-long subtree write we removed. A directory-level ALL APPLICATION PACKAGES-style grant would be the clean answer but requires the directory to opt into that ACL by design — outside the sandbox's authority.
…ken DACL, bash ladder probes - capture the write root's label at the first write holder even when read/traverse holders exist, and replay the recorded ACE flags and policy mask on lift instead of hard-coded Low-label defaults (state L record gains flag/policy columns, still reads the old form) - undo the just-planted grant if the Low label write fails, so a failed launch leaves no live ACE its retired record cannot reap - run plant_files' per-file DACL edits inside the transact so the DACL lock serializes them against other helpers' read-modify-write - readable_by_packages now evaluates the ACE mask and skips inherit-only entries, and a denied ACE touching the needed access stops the search - report only STATUS_DLL_INIT_FAILED as a runtime-init failure; pass other exit codes through without the MSYS diagnosis - mint the token file's protected DACL in SECURITY_ATTRIBUTES at CreateFileW so the file never exists with a wider inherited DACL - probe LOCALAPPDATA\Programs\Git in the shell ladders and skip the WSL System32 bash.exe stub during the PATH fallback
read TOKEN_USER via read_unaligned (byte-aligned buffer), u32::try_from for SECURITY_ATTRIBUTES.nLength, rustfmt the touched files
…ippy fixes - write the writable root's Low label ACE with (OI)(CI) so it reaches the whole subtree like the DACL grant does - hoist the readable_by_packages ACE constants ahead of statements and drop the needless borrow in the label-restore rollback
- Git\bin\<tool>.exe launchers carry no msys import but exec the msys binary at <root>\usr\bin\<name>; probe that twin so the launcher is refused like the binary it forwards to (windows-11-arm ran the launcher and died inside the container with a bare exit 5) - runtime_init_failure now diagnoses any nonzero exit from a binary that links an MSYS/cygwin runtime, since the launcher propagates whatever code the runtime died with
Get-Acl lives in Microsoft.PowerShell.Security, which fails to autoload on CI runners without a complete PSModulePath; the raw .NET API needs no module
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
dalgona/dal/crates/dalgon/src/sandbox.rs
Lines 1045 to 1049 in ca7526c
Revoke grants when the post-edit state save fails
If the second transact applies edit_dacl and the Low label successfully but then fails in state.save, this error branch removes the persisted holder without revoking the already-live ACE or restoring the label. A transient state-file write/rename failure can therefore leave a writable root permanently Low and its AppContainer grant undiscoverable by the reaper; roll back the filesystem changes before retiring the holder, or preserve the intent for recovery.
AGENTS.md reference: AGENTS.md:L107-L109
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let _ = transact(edge, |state| { | ||
| for path in recorded.iter().skip(planted.len()) { | ||
| let key = path_key(path); | ||
| if state.holders.get(&key).is_some_and(|h| h.len() == 1) { | ||
| state.orig.remove(&key); |
There was a problem hiding this comment.
When edit_dacl fails after one or more earlier files in the batch succeeded, this cleanup removes intents only for the failed and unattempted suffix. The successfully edited paths remain in planted, but returning Err discards that list, so plant_grants cannot include them in lift_all; their ACEs and holder records remain after this helper exits until some later sandbox transaction happens to reap them. Revoke the successful prefix here or return it to the caller for cleanup.
AGENTS.md reference: AGENTS.md:L129-L131
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 98318d5. The error branch now revokes each planted path's ACE and retires its holder (only once the revoke succeeds — a miss keeps the holder for the reaper to retry), then retires the never-attempted suffix as before. Nothing stays live but untracked when the helper returns Err.
- plant: if edit_dacl + the Low label landed but state.save failed, the error path retired the holder while the ACE stayed live and the root stayed Low — undo revoke + label restore before dropping the record, and keep the holder when the revoke misses so the reaper can retry - plant_files: returning Err dropped the planted prefix, leaving those ACEs and holders alive until a later reap; revoke each planted path and retire its holder alongside the never-attempted suffix
|
Re the P1 "revoke grants when the post-edit state save fails": fixed in 98318d5 — the retire transact now revokes the ACE (and restores the original label for write grants) before dropping the holder record; a missed revoke keeps the holder so the reaper can retry the lift. |
1 participant
Summary
Fix the Windows defects surfaced by the
dalworkspace suite on Windows Server 2022 — the AppContainer sandbox timed out and failed spawns, and the release gates could not run.crates/dalgon/src/sandbox.rs(the bulk of the diff):write_dir_daclwrites a directory's DACL object-only throughNtSetSecurityObject.SetNamedSecurityInfoWre-propagates every inheritable ACE on the directory through its whole subtree regardless of the new ACE's flags — measured ~40–50s per call onC:\Users/C:\Users\Administrator, which is what madesandbox_rejects_rm_outside_allowed_rootsexceed the gate's timeouts. Traverse grants/revokes now take ~ms.DaclStaterecordsorig_label; the first write planter snapshots the object's integrity label andliftrestores it on the last holder leaving, so the Low mandatory label does not outlive the run.plant_grantsskips PATH directories the container can already read viaALL APPLICATION PACKAGES; otherwise it plants a traverse grant on the directory, the executable, and a bounded (PATH_GRANT_LIMIT = 128) set of runnable files (exe|dll|bat|cmd|ps1|com) frompath_grant_files.posix_runtime_importwalks the PE import table and refuses executables that importmsys-*/cygwin*before spawn;runtime_init_failuremaps a0xC0000142/66early-init death (e.g. launchers likeGit\bin\bash.exe) to the same legible refusal — MSYS/cygwin cannot init inside a classic AppContainer's private\BaseNamedObjects.spawnkeepslpApplicationNameNULL: under the container token the loader rejects explicit paths it cannot map, while quoted argv0 resolves through the normal argv path.crates/dal-wire/src/token.rs— on Windows, token creation stamps a protected DACLD:P(A;;FA;;;<owner>)(A;;FA;;;SY)(A;;FA;;;BA)(the POSIX 0600 counterpart); the crate lint movedforbid(unsafe_code)→deny+#[expect]onmod win.Repo scripts on Windows —
.gitattributespins*.shto LF;scripts/*.shacceptpython3orpython;release_support/mod.rsresolves bash through%ProgramFiles%\Git\bin\bash.exebefore PATH (the WSL stub sorts earlier but cannot openC:\paths) and normalizes CRLF.gates/tests/release_guards.rs— expects therelease dal firstliteral that upstreamee66c0ainstalled inscripts/release.py(the test was stale ondev, failed on every platform).Verified on this box:
cargo fmt --check,cargo clippy --all-targets --all-features --locked -D warnings, andcargo test --workspace --all-features --lockedall pass (the long-runningshuttle_interleaving_race_proofskipped — see note).dal-full-03now finishes in ~1.6s.Not addressed here:
open_locked_journal's 60s own-lock retry makes the shuttle test take ~20min on Windows (cross-platform quirk, left as-is).Link to Devin session: https://app.devin.ai/sessions/8f7410ed0f314371ad3bc98105fb34c9
Open in Devin Desktop: https://app.devin.ai/desktop/session/8f7410ed0f314371ad3bc98105fb34c9?variant=devin
Requested by: @metaphorics