mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 16:02:50 +00:00
* fix(windows): read command lines from the kernel, not each process's PEB MDE incident D scored Orca for suspicious memory activity: the vendored `@vscode/windows-process-tree` recovered every process's command line by opening it with `PROCESS_QUERY_INFORMATION | PROCESS_VM_READ` and chaining three `ReadProcessMemory` calls through the PEB and `RTL_USER_PROCESS_PARAMETERS`. On a 750ms/2s cadence over the whole table that is the credential-dumping primitive, whatever the intent. Windows 8.1 added `NtQueryInformationProcess`'s `ProcessCommandLineInformation` class (60), which returns the same string as a kernel-built `UNICODE_STRING` under `PROCESS_QUERY_LIMITED_INFORMATION` alone. Electron's floor is Windows 10, so every supported OS has it. The PEB reader stays behind a process-wide latch that only `STATUS_INVALID_INFO_CLASS`/`NOT_SUPPORTED`/`NOT_IMPLEMENTED` can set; a pid that merely denied a handle does not re-arm it, because `PROCESS_QUERY_INFORMATION` implicitly grants the limited right and so cannot be obtained where the weaker open already failed. The same hunk drops `PROCESS_VM_READ` from `GetProcessMemoryUsage` and `GetCpuUsage`, which acquired it and never read an address space. Measured on Windows 11 (514 processes), counted in-process by swapping the addon's import table entries for counting stubs, per CommandLine scan: `ReadProcessMemory` 1128 -> 0, desired access 0x0410 -> 0x1000, p50 12.7ms -> 9.3ms. Command lines were byte-identical on every process both readers recovered (376/376, 379/379 across runs), including a 24,068-character argv with quotes, non-ASCII and trailing whitespace, and a WOW64 target. Three processes that refused the old rights granted the new one; none went the other way. * chore(deps): refresh the windows-process-tree patch hash in the lockfile * fix(windows): drop the PEB fallback and detect the unpatched prebuilt Review of #17886 found three ways the reader could still perform, or silently resume, the primitive it exists to remove. The class-missing latch was a permanent, process-wide, one-way downgrade back to the PEB read, and any single target returning STATUS_INVALID_INFO_CLASS / NOT_SUPPORTED / NOT_IMPLEMENTED could trip it. On an EDR-hooked ntdll -- the entire premise of this change -- a hook that does not recognise class 60 would have restored PROCESS_VM_READ plus three ReadProcessMemory per pid per scan for the life of the process, unobservably, on precisely the machines this was written for. The fallback is deleted rather than guarded: GetProcessCommandLine now returns false and leaves the command line empty, which callers already handle, so the addon imports no ReadProcessMemory at all. That absence is what makes the property checkable on the artifact. The published 0.8.0 tarball ships a loadable prebuilt built from unpatched source; it is node-addon-api, so a bare require() accepts it, allowBuilds is false and CI installs with --ignore-scripts, and a rebuild that soft-exits on a Windows file lock leaves it in place. Source-text guards could never see it. windowsProcessTreeAddonReadsProcessMemory() checks the compiled binary instead, and is wired into the install check, the rebuild, and the relay build. The repair itself never worked: `git apply` run inside a work tree prefixes patch paths with the cwd-relative prefix, skips what does not match, and exits 0, so the branch always fell through to its own post-check throw. The package dir is always under the project root, while the fixture that covered it was in %TEMP%, outside any repo. Blinding git with GIT_DIR fixes it, and the test now runs inside a real work tree. Also from review: bounds-check the returned UNICODE_STRING against the allocation (not the size the second query clobbers) and cap the probe so a bogus length cannot bad_alloc a whole scan; test NT_SUCCESS explicitly; value- initialize ProcessInfo, which left `memory` as stack garbage -- measured, 82 processes reported the same bogus working set; and correct a comment in windows-process-table.ts that still described the command line as a PEB read. Re-measured on Windows 11 (543 processes): ReadProcessMemory 1128 -> 0, with the symbol absent from the import table so the IAT hook finds no slot to count; desired access 0x0410 -> 0x1000 on all 543 opens; p50 13.5 -> 12.3ms; 405/405 command lines byte-identical including a 24,087-character quoted non-ASCII argv and a WOW64 target; 3 processes recovered only by the new path, 0 only by the old. * chore(deps): refresh the windows-process-tree patch hash in the lockfile * test(scripts): stage a script's local imports into the native-runtime fixture ensure-native-runtime.mjs gained an import of windows-process-tree-gyp-rebuild.mjs, but the fixture copied only the script itself, so every case in the suite died with ERR_MODULE_NOT_FOUND before reaching its own assertions. copyScriptWithLocalModules already walks a script's co-located imports for exactly this reason -- its own doc comment names this failure -- so use it rather than listing files by hand. The two Windows cases still fail here, on a missing node-pty ConPTY runtime that also fails on main; this only stops a resolution error from standing in front of whatever they were meant to catch. * fix(windows): route a locked stale addon to the Windows file-lock message `pnpm install` with Orca running aborted with a raw EPERM stack. The stale-binary guard -- which deletes an addon that still imports ReadProcessMemory so a skipped rebuild cannot use it -- ran outside the try whose catch classifies Windows file locks, and whose message is literally "Close running Orca/Electron/dev processes for this worktree": exactly this situation. Measured rather than assumed: rmSync against a loaded (memory-mapped) addon throws EPERM, and `force: true` does not help, since it only swallows ENOENT. Cold copies of the same file delete fine. So the delete threw a page before the handler that knows what it means. Moving the guard inside the try is the whole fix; the classifier already matches the EPERM text. The new case runs the real script against a temp project whose stale addon is held open by a live child process, and fails against the old placement with the raw `syscall: 'rm'` stack the report described. * feat(windows): warn once when command-line recovery is refused host-wide Removing the PEB fallback removed a total-defeat vector, but it left a cliff: if NtQueryInformationProcess(ProcessCommandLineInformation) is refused -- a hooked ntdll that does not know class 60 -- every command line comes back empty and agent identity matching silently degrades to image names. The addon still loads and still enumerates, so every health check the app has stays green. A cliff nobody can see is the failure mode this area keeps producing. The querying process is the unambiguous probe. A process can always open itself with PROCESS_QUERY_LIMITED_INFORMATION, so its own command line coming back empty means the query is refused for every process -- not that some target denied a handle, which is normal for roughly a quarter of the table. Keying on our own row rather than a fraction means no threshold to tune and no false positive on a hardened box where most processes deny. One warning per session, gated on the CommandLine flag actually being requested so a future identity-only reader cannot trip it. The suite's own SELF fixture gains a command line for the same reason: a self row without one is the alarm, not a detail. * fix(windows): check the relay's staged addon at load, and answer tri-state Two gaps in the ReadProcessMemory check, both about what it does not see. It only ever looked at node_modules/@vscode/windows-process-tree. A relay host has no node_modules of ours: it loads ./windows-process-tree.node staged beside the bundle. The relay build asserts the symbol on the artifact it produces, but a bundle and the addon beside it redeploy independently, so a host that has not taken a new bundle keeps whatever binary is already there -- and the published prebuilt is node-addon-api, so it binds cleanly and then walks every process's address space. loadWindowsProcessTree now checks that file too and refuses it, falling back to the CIM scan: slower, but not the thing an EDR quarantines a host for. The predicate is duplicated rather than imported, because the config-script copy is install-time tooling that drags in node-gyp and child_process, and this module is bundled into the app and the relay. And it returned false for a binary that is not there. All three callers happened to be safe, but the name read as a safety predicate, so a future caller would take a missing binary as verified. inspectWindowsProcessTreeAddon() now answers clean/unpatched/missing over an explicit binary path -- which is also what lets the relay's staged addon be checked at all -- and each caller states which state it acts on. Both are covered by cases that fail against the old code: without the load-time check the unpatched staged addon is bound and the CIM fallback never runs, and with 'missing' folded back into 'clean' the absence case fails outright. * test(windows): load the addon in beforeAll, not at collection time loadAddon() ran while the file was being collected, so on a Windows checkout with no built addon the require threw before any case existed and took the seven patch-text cases down with it -- cases that read only the patch file and need no binary at all. Verified both ways against a deliberately unresolvable addon path: at collection time vitest reports "no tests" for the file; from beforeAll the seven text cases pass and only the three addon cases go. * fix(deps): normalize the windows-process-tree patch to LF and let pnpm own its hash `pnpm install --frozen-lockfile` failed on this branch on every platform with ERR_PNPM_LOCKFILE_CONFIG_MISMATCH, which breaks CI and the release build. Two coupled defects. The patch file was committed with CRLF -- 174 CR bytes, against zero on main -- and `.gitattributes` pins `/config/patches/*.patch -text` precisely so checkout cannot convert it, so those bytes reached every runner. And pnpm hashes a patch **LF-normalized**, so the raw sha256 of a CRLF file is a value pnpm never computes: raw sha256 322965470c05f63d8527f7d8e892ee26ee444136b66b57fd64c362a9f2ff05d1 LF-normalized f8ea245391c94da5770045aeea01fa6de466c2199c6ef46b5b769b398aa9823e The lockfile carried the raw one, at all three sites. It is the only one of the seven patches where the two digests differ, which is why the other six passed. Normalized the patch to LF and took pnpm's own value from `pnpm install --no-frozen-lockfile`; nothing here is hand-computed. With the file LF-only the two interpretations coincide, so the lockfile, the contract test's no-CR assertion and its hash assertion all agree at one number -- and `config/scripts/windows-process-tree-patch-contract.test.mjs`, which was red on this branch for the same reason, is green again. The lockfile diff is exactly the three hash lines. The regression check is the installer, not a digest. Two separate reviews "verified" the shipped hash by recomputing sha256(patchBytes) and matching the lockfile; both were wrong, because both repeated the same wrong assumption about which bytes pnpm hashes. A check that reproduces the original mistake is not independent. So the new case runs `pnpm install --frozen-lockfile --lockfile-only --ignore-scripts` against a copy of the manifest, lockfile and patches, and asserts exit 0 -- verified by deletion: restoring the shipped hash fails it with the exact ERR_PNPM_LOCKFILE_CONFIG_MISMATCH from the branch's package (windows) job. Also corrected the `.gitattributes` comment claiming pnpm hashes patches byte-for-byte. The `-text` setting is right -- `git apply` needs the exact bytes -- but that sentence is the claim that produced the wrong hash twice. * ci(windows): run the process-tree patch suites in CI Both suites only self-skip off Windows, so the binary-level check that the addon carries no ReadProcessMemory passed vacuously in every lane. * fix(windows): force core.autocrlf=input for the patch repair My LF normalization of the windows-process-tree patch broke the `git apply` repair path introduced in this PR. The two are coupled and I checked only one. Those 174 CR bytes were not editor noise. They sat on exactly the pre-image lines and nowhere else -- 107/107 in src/process.cc, 67/67 in src/process_commandline.cc, 0 on every added or context line -- because @vscode/windows-process-tree@0.8.0 ships those two sources as CRLF. Normalizing the patch made its pre-image stop matching the file it is applied against. Measured, reconstructing the true CRLF pre-image from the pre-normalization blob and applying the current LF patch: core.autocrlf plain -c core.autocrlf=input true exit 0 exit 0 input exit 0 exit 0 false exit 1 exit 0 `false` is Git's own built-in default and what "checkout as-is" selects in the Git for Windows installer -- on this box the `true` that hides it comes from the installer's system gitconfig, not from anything in the repo. There the repair throws, ensureWindowsProcessTreeCommandLinePatch reports "still reads the PEB, and repairing it ... failed", isWindowsNativeLockError does not match that text, and `pnpm install` dies with no path forward. Forcing the mode rather than `--ignore-whitespace`: both fix every cell and both leave the applied file fully LF, but `input` relaxes line endings only, so a hunk whose real content drifted is still rejected. The repair rewrites a security-relevant source file; it should stay strict about everything except the thing that is legitimately ambiguous. Not reverting the patch to CRLF: windows-process-tree-patch-contract.test.mjs (pre-existing on main) forbids CR bytes in it, and pnpm computes the same hash either way. LF plus the forced mode is the end state. The suite could not have caught this. The fixture built its pre-image from the patch itself and joined with '\n', so fixture and patch agreed by construction on any encoding -- once again a test that passes without its fix. It now emits the CRLF the real package ships, and the case runs under both autocrlf modes pinned through a temp HOME gitconfig, because the repair blinds git to the repo and so reads global config. Verified by deletion in both directions: with the flag removed the autocrlf=false case fails with the exact "still reads the PEB" dead end while autocrlf=true still passes, and with the fixture back on LF all eight cases pass with no fix present at all. Also corrected the .gitattributes comment I added last commit. It said `git apply` needs the bytes the patch was written against, which is now false -- the pinned bytes are LF and the bytes it was written against are CRLF. That is the same class of confident-and-wrong claim that produced the bad hash twice. * fix(windows): assert the rebuilt addon, and install the patch for real in tests Three follow-ups from review. **The packaged binary had no check.** The relay build asserts its own artifact and ensure-native-runtime asserts what it loads, but nothing looked at the addon copied into the packaged app -- so a rebuild that silently produced the upstream reader shipped. `rebuild-native-deps.mjs` now asserts `clean` on it after `rebuild()`. This is also the caller D4's tri-state was missing: every existing site branches on `=== 'unpatched'`, so `missing` still behaved exactly like `clean` everywhere, which was the thing making it a state rather than a boolean. Here both non-clean states fail, and they fail differently: after a rebuild that reported success, an absent binary is a broken build, not an absence to shrug at. The fake `rebuild()` had to start producing a binary for that to mean anything, so it now emits stand-in bytes and takes `addon: 'clean' | 'unpatched' | 'none'`. Verified by deletion: with the assertion removed both new cases pass. **The frozen-install case could not see a patch at all.** `--lockfile-only` resolves and never applies one, so its coverage stops at hash consistency. Added a case that installs `@vscode/windows-process-tree@0.8.0` for real with the patch and asserts the materialized `src/process_commandline.cc` carries the marker and no longer carries `ReadProcessMemory` -- about 1.5s for the pair. Correcting the brief on that one: it does **not** catch the `git apply` breakage from the previous commit. Measured -- with `-c core.autocrlf=input` removed it passes cleanly, because `pnpm install` uses pnpm's own patch applier and never runs our repair script. What it does catch is a patch pnpm can no longer apply: corrupting one pre-image line fails both cases. The repair path stays covered by the CRLF fixture in rebuild-native-deps-node-pty.test.mjs. Worth recording, since it decides whether the LF normalization was safe at all: pnpm applies the LF patch to the CRLF tarball sources without complaint, and materializes them as LF with the marker present and `ReadProcessMemory` absent. The primary install path was never affected -- only the `git apply` fallback was. **Dead timeout.** The frozen-install case passed `timeoutMs: 300_000` to the spawn while vitest capped the case itself at 30s, so on a cold runner vitest would have killed it first. Both cases now declare the budget they use. * test(windows): route the frozen-install check through the pnpm invocation owner The new patched-dependencies check hand-rolled a PATH walk naming 'pnpm.cmd', which the windows batch shim spawn boundary ratchet rejects: pnpm-cli-invocation already owns that decision for every other script, and its allowlist only shrinks. Reuse resolvePnpmCliInvocation for the command and prefixArgs, and the shared resolveCliCommand for the presence check, so no shim name is spelled here. Its `shell` flag is dropped because runProcessSync refuses it and already drives a shim through the interpreter itself. --------- Co-authored-by: Orca Worker <orca-worker@localhost> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com>
462 lines
25 KiB
Markdown
462 lines
25 KiB
Markdown
# Reading the Windows process table
|
|
|
|
Orca needs three things from the Windows process table: who a PID's parent is
|
|
(descendant walks and teardown identity), what a process is running (agent
|
|
recognition), and how much memory/CPU it uses (Resource Manager).
|
|
|
|
Node cannot answer the first one without native code. That is why seven
|
|
independent readers existed, each forking `powershell.exe` to run
|
|
`Get-CimInstance Win32_Process`, with a `wmic` fallback that Windows 11 24H2 has
|
|
since removed.
|
|
|
|
## Use the native snapshot
|
|
|
|
`src/main/windows/windows-process-table.ts` is the only module that may read the
|
|
table. It wraps a Toolhelp32 snapshot from `@vscode/windows-process-tree`.
|
|
|
|
```ts
|
|
import {
|
|
readWindowsProcessTable,
|
|
readWindowsProcessTableFresh
|
|
} from '../windows/windows-process-table'
|
|
```
|
|
|
|
- `readWindowsProcessTable()` — shared TTL cache. Use for anything periodic.
|
|
- `readWindowsProcessTableFresh()` — a snapshot that starts after the call. Use
|
|
for teardown identity, where a cached row can predate the exit it is being
|
|
asked about.
|
|
|
|
Both **reject** when the table cannot be read. Do not convert that into an empty
|
|
array. An empty table is a claim that nothing is running, and callers act on
|
|
that claim by declaring a tree dead or a shell childless. "Unavailable" has to
|
|
stay distinguishable from "empty" — collapsing the two is how a PTY tree
|
|
survived its own teardown (#9045).
|
|
|
|
Measured on Windows 11 with 1050 processes (p50 / p95):
|
|
|
|
| | p50 | p95 |
|
|
| -------------------------------- | ------- | ------- |
|
|
| pid + ppid + name | 15.9 ms | 17.5 ms |
|
|
| + memory + command line | 30.6 ms | 33.7 ms |
|
|
| `Get-CimInstance` via PowerShell | 706 ms | 723 ms |
|
|
|
|
Those are the module's published figures. The flag set this module actually
|
|
requests is `CommandLine | CreationTime` — **not** `Memory`, which cost a second
|
|
`OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ)` plus
|
|
`GetProcessMemoryInfo` per process (`src/process.cc:47-63`) for a value nothing
|
|
read. Dropping it halves the handles a snapshot opens. The remaining set sits
|
|
between the two rows above and has not been measured separately; on a real
|
|
Windows host, `Get-Counter '\Process(Orca)\Handle Count'` sampled across a
|
|
snapshot cadence is the check.
|
|
|
|
Those CIM numbers are from a 1050-process host. The scan scales with process
|
|
count: on a 1486-process Windows SSH host it measured **1.36 s** and produced
|
|
**4.8 MiB** of JSON, against the fallback's 3 s and 8 MiB limits. Both limits
|
|
match the pre-#15749 reader, so relay hosts are at parity rather than newly at
|
|
risk — but the headroom is roughly 2x on time and 1.7x on bytes, not the ~4x the
|
|
706 ms figure implies. On overflow the output is truncated, the JSON fails to
|
|
parse, and the read rejects, so a busy host loses the table rather than
|
|
receiving a wrong one.
|
|
|
|
## When a read wedges
|
|
|
|
The vendored reader pushes every callback onto a module-global queue and drains
|
|
that queue only when the request holding its `requestInProgress` latch
|
|
completes. If a Toolhelp32 snapshot never comes back — an EDR hook, a restricted
|
|
token, a worker that dies — the latch is stuck for the life of the process and
|
|
every later call parks another closure in that queue.
|
|
|
|
Two guards, and they work together:
|
|
|
|
- a **3 s deadline** on each read, so a caller gets a rejection instead of a
|
|
promise that never settles;
|
|
- a **sticky wedge**: once a read misses its deadline and has not called back,
|
|
the module refuses every further read until that read's callback fires.
|
|
|
|
The wedge used to be a 30 s cooldown that let one probe through per window. That
|
|
bounded the _rate_ of new callbacks but not the total: a permanently wedged
|
|
reader retained one more closure every 30 s for as long as the app ran, and each
|
|
probe also blocked its caller for the full 3 s deadline first. Gating on the
|
|
outstanding read instead bounds retention at exactly one callback, and gives up
|
|
nothing on recovery — a probe queued behind the latch could never have observed
|
|
recovery anyway, whereas the stuck callback firing is the drain itself. On the
|
|
relay's bare addon, which has no queue of its own, it is also what keeps Orca
|
|
from re-entering `CreateToolhelp32Snapshot` while a call is still running.
|
|
|
|
That last part is not just tidiness. The addon runs each read as a
|
|
`Napi::AsyncWorker`, so a wedged read holds a libuv threadpool slot for good. On
|
|
the relay the JS queue is not there to absorb the retries, so one probe per
|
|
window would have pinned all four default threads inside ~2 minutes — hanging
|
|
every async `fs` and DNS call in that process, not only the process table.
|
|
|
|
A wedge does **not** engage the PowerShell fallback; see the next section for
|
|
why only absence does.
|
|
|
|
## The relay has no binding, and falls back
|
|
|
|
Relay deployment installs only `node-pty` and `@parcel/watcher` on the remote
|
|
host (`RELAY_NATIVE_DEPS` in `src/main/ssh/ssh-relay-deploy.ts`), so a Windows
|
|
machine used as an SSH host has no `@vscode/windows-process-tree` at all. It is
|
|
not added there on purpose. Both ways of installing it fail, and both were
|
|
checked on a real Windows SSH host with 1486 processes:
|
|
|
|
**Installing it normally rebuilds from source, and that build fails.** The
|
|
tarball carries a `binding.gyp`, so npm runs `node-gyp rebuild` regardless of
|
|
what is already compiled inside it. On a host that _already had_ MSVC Build
|
|
Tools 2022 installed, that build still failed:
|
|
|
|
```
|
|
error MSB8040: Spectre-mitigated libraries are required for this project.
|
|
```
|
|
|
|
That is the requirement the `binding.gyp` hunk of our patch deletes, and the
|
|
patch cannot reach a remote host — pnpm patches do not cross SSH. Relay deploy
|
|
would then break outright rather than degrade: `installNativeDeps` throws on
|
|
failure, and the toolchain-skip retry is gated to Linux.
|
|
|
|
**Skipping the build and using the shipped binary returns a truncated table.**
|
|
Contrary to what this file used to claim, the published 0.8.0 tarball _does_
|
|
contain `build/Release/windows_process_tree.node` — an MSVC build directory that
|
|
looks accidentally published (`.obj` and `.tlog` files ship with it). It is
|
|
N-API, so it loads on any modern Node. But it predates our patch and still has
|
|
the `process_count < 1024` cap, so on that 1486-process host:
|
|
|
|
```
|
|
LOADED OK
|
|
rows=1024
|
|
selfPid=21964 present=false
|
|
```
|
|
|
|
Exactly 1024 rows, with the querying process itself among the missing. The
|
|
self-presence guard rejects that, so the fallback engages anyway — but only on
|
|
hosts busy enough to cross the cap. That is worse than no binding at all: it
|
|
works on a quiet machine and fails silently under load, which is precisely the
|
|
shape of bug that survives testing.
|
|
|
|
So the constraint is not that no binary exists to ship. It is that the only
|
|
binary available to ship is the broken one, and building the good one needs a
|
|
toolchain the remote does not have.
|
|
|
|
Instead, `windows-process-table.ts` falls back to
|
|
`readWindowsProcessRowsWithCim` (`windows-process-table-cim-scan.ts`), the
|
|
`Get-CimInstance` scan this module replaced. The gate is deliberately narrow:
|
|
|
|
- it engages **only** when the module cannot be required, never when a loaded
|
|
module fails, wedges, or returns an unreadable table — a present-but-failing
|
|
reader must not silently start forking a shell at the caller's poll rate;
|
|
- a fallback that also fails still rejects, so "unavailable" never degrades into
|
|
"nothing is running";
|
|
- the scan applies the same self-presence guard as the native path.
|
|
|
|
`src/main/ssh/relay-native-dependency-coverage.test.ts` asserts that every
|
|
native addon reachable from the relay entry is either installed on relay hosts
|
|
or listed there with the reason its absence is safe. That test exists because
|
|
#15749 shipped this gap: the relay tests injected a fake module through
|
|
`__setWindowsProcessTreeLoaderForTests`, so nothing exercised the real require.
|
|
|
|
## Shipping the native reader to a relay anyway
|
|
|
|
The scan is the floor, not the destination: it costs ~1.4 s and a `powershell.exe`
|
|
where the addon costs ~57 ms. Release builds therefore compile the addon and ship
|
|
it as an optional relay artifact.
|
|
|
|
`config/scripts/build-windows-process-tree-relay-addon.mjs` builds it from the
|
|
source pnpm has already patched, on a Windows runner, and refuses to run if
|
|
any patch hunk is missing — the Spectre hunk fails loudly, the 1024-process
|
|
hunk fails _silently_, and the relative gyp path dies at configure on Windows.
|
|
The source is checked rather than the install trusted. It also reads the PE
|
|
machine field of the output, because a cross-build that quietly emitted host
|
|
arch would ship a binary the target cannot load.
|
|
|
|
Windows arm64 cross-compiles from the x64 runner — verified on real hardware,
|
|
producing `IMAGE_FILE_MACHINE_ARM64` (0xaa64) against x64's 0x8664. It needs the
|
|
optional _MSVC v143 ARM64 build tools_ component; without it node-gyp fails with
|
|
`MSB8020`, which is why the addon build runs before the long packaging step.
|
|
`ORCA_REQUIRE_RELAY_NATIVE_ADDONS` is a per-arch list so a future arch can be
|
|
added best-effort before it is promoted to required.
|
|
|
|
`windows-process-table.ts` binds the bare addon directly rather than the package
|
|
wrapper. That wrapper adds only a queue over `getProcessList`, and that queue is
|
|
the wedge described above — it latches a module-global `requestInProgress` with
|
|
no try/catch. This module already holds a single-flight and a deadline, so going
|
|
straight to the addon drops the duplicate.
|
|
|
|
The artifact is optional in `RELAY_ARTIFACTS`: hashed when present, so a relay
|
|
carrying it never shares an immutable directory with one that does not, and
|
|
never probed, because requiring a file only a Windows build machine can produce
|
|
would make a correct relay read as MISSING and redeploy forever. A relay built
|
|
on any other OS keeps using the scan.
|
|
|
|
## Why the package is patched
|
|
|
|
`config/patches/@vscode__windows-process-tree@0.8.0.patch` carries four hunks.
|
|
|
|
1. **Spectre mitigation.** The upstream `binding.gyp` requires Spectre-mitigated
|
|
libraries, which Orca's Windows build agents do not install. `node-pty` is
|
|
patched the same way for the same reason.
|
|
2. **The 1024-process cap.** `GetRawProcessList` stopped after 1024 entries.
|
|
Measured on a real host with 1051 processes, the module returned exactly
|
|
1024 and the querying process was itself among the 27 missing. A truncated
|
|
snapshot silently hides the descendants a teardown is trying to reap — the
|
|
exact failure the native path exists to remove.
|
|
3. **Absolute `node-addon-api` gyp path.** `require('node-addon-api').targets`
|
|
is cwd-relative. node-gyp on Windows evaluates it from the pnpm store
|
|
realpath, then loads the relative path from the `node_modules` symlink, so
|
|
`node_addon_api.gyp` resolves outside the repo and hourly Windows builds
|
|
die at configure. `node-pty` is patched the same way for the same reason.
|
|
4. **No PEB reads, no `PROCESS_VM_READ`.** See below.
|
|
|
|
The typings claim `commandLine` is truncated at 512 characters. Measured, it is
|
|
not: the longest observed on a real host was 26,059.
|
|
|
|
### The command line comes from the kernel, not the target's memory
|
|
|
|
Upstream, `GetProcessCommandLine` opens every process with
|
|
`PROCESS_QUERY_INFORMATION | PROCESS_VM_READ` and issues three chained
|
|
`ReadProcessMemory` calls — PEB, `RTL_USER_PROCESS_PARAMETERS`, then the string
|
|
— to recover the command line. Walking another process's address space for
|
|
credentials-adjacent data on a repeating timer is what a credential dumper does,
|
|
so Defender for Endpoint scores it as such regardless of intent. Nothing about
|
|
the flag sets above changes that; only removing the read does.
|
|
|
|
Windows 8.1 added `NtQueryInformationProcess`'s `ProcessCommandLineInformation`
|
|
class (60), which returns the same string as a `UNICODE_STRING` the kernel
|
|
builds, needing only `PROCESS_QUERY_LIMITED_INFORMATION`. Electron's floor is
|
|
Windows 10, so every OS Orca supports has it. The entry point is resolved with
|
|
`GetProcAddress` on `ntdll.dll` — it has no import library — and the size is
|
|
probed with a null-buffer call that answers `STATUS_INFO_LENGTH_MISMATCH`.
|
|
|
|
The same hunk drops `PROCESS_VM_READ` from `GetProcessMemoryUsage` and
|
|
`GetCpuUsage`, which acquired it and never read an address space:
|
|
`GetProcessMemoryInfo` and `GetProcessTimes` are satisfied by
|
|
`PROCESS_QUERY_LIMITED_INFORMATION`. Measured, both return identical values
|
|
under the weaker right on every process that opens at all.
|
|
|
|
Measured on Windows 11, ~540 processes, counted in-process by replacing the
|
|
addon's import table entries with counting stubs:
|
|
|
|
| per `CommandLine` scan | before | after |
|
|
| ---------------------- | ----------------------------------------- | -------------------------------------- |
|
|
| `OpenProcess` calls | 543 | 543 |
|
|
| desired access | `0x0410` (`VM_READ \| QUERY_INFORMATION`) | `0x1000` (`QUERY_LIMITED_INFORMATION`) |
|
|
| `ReadProcessMemory` | 1128 | **0** |
|
|
| p50 / p95 | 13.5 / 14.5 ms | 12.3 / 13.5 ms |
|
|
|
|
Command lines were byte-identical on every process both readers recovered
|
|
(405/405, and 399/399 and 376/376 on other runs), including a 24,087-character
|
|
argv with embedded quotes, non-ASCII characters and trailing whitespace, and a
|
|
WOW64 target. The weaker right is also a strict superset in reach: three
|
|
processes that refused `PROCESS_QUERY_INFORMATION | PROCESS_VM_READ` granted
|
|
`PROCESS_QUERY_LIMITED_INFORMATION`, and none went the other way.
|
|
|
|
### There is no PEB fallback, deliberately
|
|
|
|
An earlier revision kept the PEB reader for a kernel without class 60, behind a
|
|
latch. That was wrong, and the reason is worth recording: `ClassifyQueryFailure`
|
|
mapped `STATUS_INVALID_INFO_CLASS` / `NOT_SUPPORTED` / `NOT_IMPLEMENTED` from
|
|
**any single target** onto a process-wide, one-way switch back to
|
|
`PROCESS_VM_READ` plus three `ReadProcessMemory` per pid per scan, for the life
|
|
of the process, with nothing observable from JS.
|
|
|
|
The environment this reader exists for is one where an EDR hooks `ntdll`. A hook
|
|
that returns `STATUS_INVALID_INFO_CLASS` for a class it does not recognise would
|
|
have silently reinstated the exact primitive the patch removes, on precisely the
|
|
machines it was written for — and one stray status from one process was enough.
|
|
The same applies under Wine or any instrumented `ntdll`.
|
|
|
|
So the fallback is gone rather than guarded. `GetProcessCommandLine` returns
|
|
false and leaves the command line empty, which is already a normal outcome
|
|
(`WindowsProcessRow.command` is documented as empty when a process denies a
|
|
query handle, and callers fall back to the image name). Degrading to no command
|
|
line is recoverable; silently resuming address-space reads is not.
|
|
|
|
This also makes the property checkable on the artifact rather than the source:
|
|
the patched reader never calls `ReadProcessMemory`, so the symbol is absent from
|
|
the compiled addon's import table. `inspectWindowsProcessTreeAddon()` in
|
|
`config/scripts/windows-process-tree-gyp-rebuild.mjs` is that check, and it is
|
|
the only way to tell the two binaries apart — see below. It answers
|
|
`clean` / `unpatched` / `missing` rather than a boolean, because a binary that is
|
|
not there has not been cleared, and a caller reading `false` as “verified” would
|
|
pass exactly the thing the check exists to catch.
|
|
|
|
Because the returned `UNICODE_STRING` comes from that same hookable boundary,
|
|
its `Buffer` and `Length` are bounds-checked against the allocation before the
|
|
characters are encoded, and the probed size is capped at the header plus 64 KiB
|
|
(`Length` is a `USHORT`) so a bogus size cannot turn into a `bad_alloc` that
|
|
fails an entire scan instead of one process.
|
|
|
|
### The published tarball ships a loadable unpatched prebuilt
|
|
|
|
`@vscode/windows-process-tree@0.8.0` publishes
|
|
`build/Release/windows_process_tree.node` in the tarball. It is node-addon-api,
|
|
so it is ABI-stable and loads cleanly under both Node and Electron — and it was
|
|
built from unpatched source, so it performs 1179 `ReadProcessMemory` calls and
|
|
opens every process at `0x0410` per scan.
|
|
|
|
That matters because `allowBuilds` is `false` for this package and CI installs
|
|
with `--ignore-scripts`, so nothing compiles it at install time. A `require()`
|
|
health check cannot tell the two binaries apart, and a rebuild that is skipped —
|
|
`rebuild-native-deps.mjs` soft-exits 0 on a Windows file lock during postinstall
|
|
— leaves the upstream prebuilt in place and cached.
|
|
|
|
Four checks close that, all keyed on the absent `ReadProcessMemory` import:
|
|
|
|
- `ensureWindowsProcessTreeCommandLinePatch()` deletes a binary that still has
|
|
it, so a skipped rebuild fails loudly instead of using the prebuilt;
|
|
- `ensure-native-runtime.mjs` treats such a binary as a load failure, which is
|
|
what triggers the rebuild;
|
|
- the relay build asserts it on the artifact it just produced;
|
|
- `loadWindowsProcessTree()` asserts it again on the addon staged beside a relay
|
|
bundle and refuses to bind one that still imports the symbol, falling back to
|
|
the CIM scan. The build-time assertion is not enough on its own: a bundle and
|
|
the addon beside it redeploy independently, so a host that has not taken a new
|
|
bundle keeps whatever `.node` is already there.
|
|
|
|
What none of this does is narrow _which_ processes are asked. A detailed scan
|
|
still queries every pid, including `lsass.exe`; it now asks with the same right
|
|
Task Manager uses instead of `PROCESS_VM_READ`. Restricting the command-line
|
|
pass to Orca's own subtree is the complementary change, and it belongs with the
|
|
identity/detailed reader split rather than here — a ppid-derived allowlist would
|
|
miss exactly the detached, reparented descendants the trackers exist to find
|
|
(#9045, #10475), so it needs the job-object membership as its source of truth.
|
|
|
|
## Packaging
|
|
|
|
The addon is Windows-only, so it follows the same contract as
|
|
`windows-native-registry` (asserted by
|
|
`config/scripts/package-electron-runtime-contract.test.mjs`):
|
|
|
|
- an `optionalDependency`, so a macOS/Linux install tolerates its absence;
|
|
- **not** enabled in `allowBuilds` in `pnpm-workspace.yaml` — pnpm installs optional dependencies
|
|
on every host, and macOS/Linux must never run `node-gyp` for it;
|
|
- listed in the win32 branch of `rebuild-native-deps.mjs` and
|
|
`ensure-native-runtime.mjs`;
|
|
- copied into the packaged `node_modules` for win32 only.
|
|
|
|
The relay's copy is a separate artifact staged beside the bundle, so a relay host
|
|
only picks up a rebuilt addon on redeploy. Until then it keeps whatever binary it
|
|
already has, which is why the addon is checked again at load.
|
|
|
|
## What the snapshot does not provide
|
|
|
|
`CreationDate` (process start time) has no equivalent. Anything using a start
|
|
time to prove a PID has not been recycled — daemon identity, managed-hook
|
|
ownership, and CPU accounting in the memory collector — still reads it through
|
|
its own query. Those callers are not migrated.
|
|
|
|
Committed private bytes have no equivalent either, and the one memory value the
|
|
snapshot _can_ carry is unusable for the sizes Orca now sees: `process.cc` stores
|
|
`pmc.WorkingSetSize` into a `DWORD`, so anything above 4 GB wraps. That is the
|
|
second reason `windows-process-resource-collector.ts` still runs its own
|
|
`Get-CimInstance` sweep — it needs `PageFileUsage` (commit) and the CPU-time
|
|
counters in the same pass. Migrating it to the native table would cost both, and
|
|
it is why this module no longer sets the `Memory` flag at all: the field had no
|
|
reader, and asking for it opened a handle per process on every snapshot.
|
|
|
|
Start time is a proxy for identity, not identity. The durable answer for the
|
|
process trees Orca itself spawns is an inherited handle: a job object names the
|
|
tree Orca created, so no start-time comparison is needed. Those readers should
|
|
be resolved that way rather than by adding a start time to this module.
|
|
|
|
Do not adopt `getProcessCpuUsage()` from the package. It takes both CPU samples
|
|
inside one call with a blocking `Sleep(1000)` in the middle, which would hold a
|
|
libuv threadpool slot for a full second out of the Resource Manager's two-second
|
|
poll.
|
|
|
|
## Owning a PTY's process tree
|
|
|
|
`src/main/windows/windows-pty-job.ts` is the counterpart to reading the table:
|
|
it answers "is this tree mine, and how do I kill it?" with a handle instead of
|
|
an inference.
|
|
|
|
node-pty is patched (`config/patches/node-pty@1.1.0.patch`) to create a job
|
|
object per ConPTY and assign the shell to it under `CREATE_SUSPENDED`, before
|
|
the shell can spawn anything. Assigning after the fact leaves a window in which
|
|
a fast child escapes the job.
|
|
|
|
- `terminatePtyJob(proc)` — one `TerminateJobObject` call for the whole tree.
|
|
- `listPtyJobProcessIds(proc)` — the live pids under a tree that is still
|
|
tracked, including children that detached from the console.
|
|
|
|
Measured on Windows 11 against a shell whose grandchild was spawned `detached`:
|
|
job membership was `[shell, grandchild]` and one call killed both. Neither a
|
|
parent-pid walk nor `GetConsoleProcessList` sees that grandchild — it leaves
|
|
the console and reparents, which is what left `claude.exe`/`node.exe`/`cmd.exe`
|
|
holding worktree directories open (#9045, #10475, #10897).
|
|
|
|
The per-PTY job deliberately does **not** set
|
|
`JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`. Measured on Windows 11: with that flag,
|
|
releasing the handle when the shell exits also kills whatever the user left
|
|
running, so typing `exit` in a pane reaped a `start /b` server that used to
|
|
survive. The job exists to make an _explicit_ teardown exact, not to redefine
|
|
what a clean exit means.
|
|
|
|
Reaping a dead daemon's shells (#9195, #10415) is therefore a **second, nested
|
|
job**, not this one. The terminal daemon assigns itself to a kill-on-close job
|
|
at startup (`assignHostProcessToKillOnCloseJob`); children inherit membership,
|
|
so every pty is covered and the per-PTY jobs nest inside it. Its handle is
|
|
released only when the daemon process dies, so a crashed daemon reaps its tree
|
|
without changing what a clean shell exit means.
|
|
|
|
The split is the point. One job answers "kill exactly this pane's tree, now";
|
|
the other answers "do not strand anything if the host dies". Trying to get both
|
|
from one job is what reaped users' backgrounded work on a clean `exit`.
|
|
|
|
It belongs to the daemon and never to the app: an app-main crash must still
|
|
leave sessions alive, which `.github/workflows/win-crash-survival-e2e.yml`
|
|
asserts. The app spawns the daemon `detached` and is itself in no job, so
|
|
nothing is inherited across that boundary.
|
|
|
|
The consequence is that a PTY hosted by the app rather than the daemon gets a
|
|
per-PTY job but no crash reaping. That is deliberate — the alternative is a
|
|
kill-on-close job on the app, which is exactly what the crash-survival
|
|
guarantee forbids.
|
|
|
|
Once the shell exits, node-pty drops its handle record and closes the job, so a
|
|
terminated tree reports `null` rather than `[]`. Null means _unverifiable_ in
|
|
the sense of [`ssh-execution-boundary.md`](./ssh-execution-boundary.md) — no job
|
|
support, not a ConPTY, or no longer tracked. It is never evidence that
|
|
processes died.
|
|
|
|
Both functions report `unavailable` / `null` rather than a false success when a
|
|
pty has no job — an outer job without `JOB_OBJECT_LIMIT_BREAKAWAY_OK` (some EDR
|
|
and container hosts) can refuse the assignment, and a pty started before this
|
|
build has none. Callers must fall back, not conclude the tree is gone. That
|
|
conflation is the original bug.
|
|
|
|
### Known limitation: the baton table is not synchronised
|
|
|
|
node-pty keeps its per-terminal handles in a plain `std::vector` and erases from
|
|
it on a detached exit thread, while `get_pty_baton` is called from the main JS
|
|
thread. That race predates this change — `PtyResize`, `PtyClear` and `PtyKill`
|
|
all read the table the same way — but `terminatePtyJob` adds an instance of it:
|
|
the exit thread can close `hJob` between the lookup and `TerminateJobObject`.
|
|
|
|
Losing that race normally just returns `FALSE`, which surfaces as `unavailable`
|
|
and falls back. The case that would not be benign is a recycled `HANDLE` value,
|
|
where the call could reach a different job in the same process. Fixing it
|
|
properly means synchronising node-pty's handle table rather than adding a lock
|
|
around one accessor, so it is deliberately left alone here.
|
|
|
|
### The patch must actually be compiled
|
|
|
|
node-pty prefers its upstream prebuild and only builds from source when
|
|
`npm_config_build_from_source` is set or no prebuild exists for the platform.
|
|
The Windows prebuild does **not** contain this patch, so a plain `pnpm install`
|
|
on Windows yields a node-pty without the job-object exports — and
|
|
`terminatePtyJob` then reports `unavailable` on every call, which is
|
|
indistinguishable from a correctly degraded build.
|
|
|
|
Packaging is unaffected: `rebuild-native-deps.mjs` rebuilds node-pty from source
|
|
for Electron and restores the ConPTY runtime files that a bare `node-gyp
|
|
rebuild` skips. The gap is the **node-runtime test environment**, which is why
|
|
the Windows CI job rebuilds from source before running the win32 suites.
|
|
|
|
`isPtyJobOwnershipAvailable()` exists for exactly this: the win32 suite asserts
|
|
it is true before asserting anything else, so an unpatched binary fails loudly
|
|
instead of passing every case vacuously. That guard is what caught this.
|
|
|
|
`requiresPatchedNodePtySourceBuild()` in `ensure-native-runtime.mjs` now covers
|
|
win32 as well, and `pnpm rebuild node-pty` sets `npm_config_build_from_source`
|
|
so the patched source build actually replaces the upstream prebuild.
|