mirror of
https://github.com/stablyai/orca.git
synced 2026-10-18 16:03:17 +00:00
* fix(windows): guard process creation times
* fix(windows): ask the relay's bare addon for creation times too
The relay addon build now emits creationTimeMs, but the runtime binding
for the bare addon still declared only CommandLine, so a Windows relay
host requested flag 2 and every row came back without a creation time.
That leaves captureWindowsDescendantSnapshot returning null and
verifyWindowsProcessIdentity false forever on those hosts -- the relay
half of the patch was unreachable.
Naming CreationTime in the adapter is safe because the bare addon is a
content-hashed relay artifact: it ships in the same immutable relay
directory as the bundle reading it, so it can never be older than the
code asking for the bit.
Also bound the win32 guard test on our own row, which the addon can
never fail to answer, so an unconverted FILETIME or a 1601-epoch stamp
fails instead of satisfying a bare count.
* fix(windows): make the compiled addon prove its own CreationTime support
CI caught the real defect: the win32 guard test read
isWindowsProcessStartTimeAvailable() as true and then found 0 rows
carrying creationTimeMs. Unlike node-pty, this package publishes a
prebuilt .node at the same build/Release path node-gyp writes to, so
pnpm patches the source tree and leaves that binary alone. A host then
holds a patched lib/index.js -- ProcessDataFlag.CreationTime and all --
over a binary that ignores flag 4, and neither a load check nor a path
check can see the difference.
So the binary now says so itself: addon.cc exports
supportedProcessDataFlags, lib/index.js re-exports it, and
- windows-process-tree-creation-time.cjs asserts it during install,
which is what forces a from-source rebuild. It is shared by the Node
probe in ensure-native-runtime.mjs and the Electron probe in
rebuild-native-deps.mjs, exactly as node-pty-job-ownership.cjs is --
the Electron half matters because that probe decides onlyModules, so
without it the packaged app would ship the stale prebuilt.
- isWindowsProcessStartTimeAvailable() gates on the reported bit, not
the enum. Believing the enum is worse than reporting false: the
descendant snapshot returns null forever and the exit proof latches
unverifiable while structured chat believes it has a reaper.
rebuildNodeRuntimeModules could not actually have rebuilt this package:
the patched binding.gyp includes deps/node-addon-api, which the tarball
does not ship, and node-gyp must run from the physical dir.
Also closes the relay repair path's divergence: repairCreationTimeSources
wrote the C++ but not the buildNode splat or the tree-node typing, and
assertPatchApplied checked neither, so a repaired tree passed as patched
with buildProcessTree silently dropping the field.
The guard test is unchanged.
* fix(windows): keep the process-tree patch LF-only
windows-process-tree-patch-contract.test.mjs requires the patch file to
carry no CR bytes. Regenerating through pnpm patch-commit emitted 199 of
them, because the creation-time change is the first to touch files the
package ships as CRLF (src/process.h, src/process_worker.cc,
src/addon.cc, lib/index.js, lib/index.ts, the typings) -- and #17886's
own hunks over binding.gyp and src/process_commandline.cc carry the rest.
Stripping them is safe and changes nothing the lockfile records: pnpm
hashes patches CRLF-normalized, so the digest stays
e66202cc623996d02040c93449eb9ae353fddadf426cb53202a59ee710ee6fe7 and now
equals the file's plain sha256 too. It also still applies -- verified
against a deleted store entry, not a warm one -- and the precedent was
already there: the previous patch was LF-only and had been patching
those same CRLF files all along.
ensure-native-runtime.test.mjs stages the siblings the script loads at
module scope into its temp project. The import walk added by #17886 sees
`from './x.mjs'` only, so the createRequire'd .cjs siblings still have to
be named, and this PR adds a second one.
---------
Co-authored-by: Merge Sim <sim@local>
191 lines
8.1 KiB
TypeScript
191 lines
8.1 KiB
TypeScript
import { spawn } from 'node:child_process'
|
|
import { readFileSync } from 'node:fs'
|
|
import { createRequire } from 'node:module'
|
|
import { join, resolve } from 'node:path'
|
|
import { afterAll, beforeAll, describe, expect, it } from 'vitest'
|
|
|
|
/**
|
|
* The command-line reader is a patch, not repo source, so its contract is
|
|
* asserted against the patch's post-image. MDE scored the addon for
|
|
* `OpenProcess(PROCESS_VM_READ)` + `ReadProcessMemory` over the whole process
|
|
* table on a timer; these cases exist so a patch refresh cannot quietly restore
|
|
* that primitive.
|
|
*/
|
|
const PATCH_PATH = resolve(
|
|
import.meta.dirname,
|
|
'../../../config/patches/@vscode__windows-process-tree@0.8.0.patch'
|
|
)
|
|
|
|
/** Reconstruct a file as the patch leaves it: context plus added lines. */
|
|
function patchedFile(patch: string, path: string): string {
|
|
const lines = patch.split('\n')
|
|
const start = lines.findIndex((line) => line.startsWith(`diff --git a/${path} `))
|
|
if (start === -1) {
|
|
throw new Error(`${path} is not in the patch`)
|
|
}
|
|
const rest = lines.slice(start + 1)
|
|
const end = rest.findIndex((line) => line.startsWith('diff --git '))
|
|
return (
|
|
(end === -1 ? rest : rest.slice(0, end))
|
|
// A context line for an empty source line is a bare space, and unified
|
|
// diffs may drop even that, so an empty string is context too.
|
|
.filter((line) => line === '' || line.startsWith(' ') || line.startsWith('+'))
|
|
.filter((line) => !line.startsWith('+++') && !line.startsWith('@@'))
|
|
.map((line) => line.slice(1))
|
|
.join('\n')
|
|
)
|
|
}
|
|
|
|
const patch = readFileSync(PATCH_PATH, 'utf8')
|
|
const commandLineSource = patchedFile(patch, 'src/process_commandline.cc')
|
|
const processSource = patchedFile(patch, 'src/process.cc')
|
|
|
|
describe('windows-process-tree command line patch', () => {
|
|
it('reads the command line through ProcessCommandLineInformation', () => {
|
|
// Class 60 is Windows 8.1+; Electron's floor is Windows 10, so every OS
|
|
// Orca supports has it.
|
|
expect(commandLineSource).toContain('kProcessCommandLineInformation = 60')
|
|
expect(commandLineSource).toContain('OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION')
|
|
})
|
|
|
|
it('resolves NtQueryInformationProcess dynamically rather than linking it', () => {
|
|
expect(commandLineSource).toContain('GetModuleHandleW(L"ntdll.dll")')
|
|
expect(commandLineSource).toContain('GetProcAddress(ntdll, "NtQueryInformationProcess")')
|
|
})
|
|
|
|
it('probes the buffer size before allocating, and caps it', () => {
|
|
// STATUS_INFO_LENGTH_MISMATCH / STATUS_BUFFER_TOO_SMALL carry the size.
|
|
expect(commandLineSource).toContain(
|
|
'kStatusInfoLengthMismatch = static_cast<NTSTATUS>(0xC0000004L)'
|
|
)
|
|
expect(commandLineSource).toContain(
|
|
'kStatusBufferTooSmall = static_cast<NTSTATUS>(0xC0000023L)'
|
|
)
|
|
expect(commandLineSource).toMatch(
|
|
/query\(process, kProcessCommandLineInformation, nullptr, 0, &size\)/
|
|
)
|
|
// UNICODE_STRING::Length is a USHORT, so a bogus size must not become a
|
|
// bad_alloc that fails the whole scan.
|
|
expect(commandLineSource).toContain('size > kMaxCommandLineBytes')
|
|
})
|
|
|
|
it('treats the returned UNICODE_STRING as untrusted', () => {
|
|
// A hooked ntdll is the environment this reader targets, so an unchecked
|
|
// Buffer/Length would be an over-read encoded straight into JS. The bound
|
|
// must be buffer.size(), not `size`, which the second query overwrites.
|
|
expect(commandLineSource).toContain('const unsigned char* end = begin + buffer.size()')
|
|
expect(commandLineSource).toMatch(/chars == nullptr \|\|/)
|
|
expect(commandLineSource).toMatch(/command_line->Length > static_cast<ULONG>\(end - chars\)/)
|
|
})
|
|
|
|
it('has no PEB fallback and no latch that could reinstate one', () => {
|
|
// The fallback used to be reachable from any single anomalous NTSTATUS,
|
|
// which on an EDR-hooked ntdll is the realistic case -- one stray status
|
|
// would have silently restored the primitive for the process lifetime.
|
|
expect(commandLineSource).not.toContain('ReadCommandLineFromPeb')
|
|
expect(commandLineSource).not.toContain('PROCESS_BASIC_INFORMATION')
|
|
expect(commandLineSource).not.toContain('InterlockedExchange')
|
|
expect(commandLineSource).not.toMatch(/ReadProcessMemory\(/)
|
|
})
|
|
|
|
it('acquires PROCESS_VM_READ nowhere in the addon', () => {
|
|
for (const source of [commandLineSource, processSource]) {
|
|
expect(source).not.toMatch(/OpenProcess\([^)]*PROCESS_VM_READ/)
|
|
expect(source).not.toMatch(/ReadProcessMemory\(/)
|
|
}
|
|
// Memory and CPU counters kept VM_READ and never read an address space.
|
|
// Three sites now: those two plus GetProcessCreationTime, which needs the
|
|
// same limited handle for GetProcessTimes.
|
|
expect(processSource.match(/OpenProcess\(PROCESS_QUERY_LIMITED_INFORMATION/g)).toHaveLength(3)
|
|
})
|
|
|
|
it('value-initializes ProcessInfo so memory is not stack garbage', () => {
|
|
// Measured before: 82 processes reported the same bogus working set.
|
|
expect(processSource).toContain('ProcessInfo pinfo{};')
|
|
})
|
|
})
|
|
|
|
type Addon = {
|
|
getProcessList: (
|
|
callback: (rows: { pid: number; commandLine?: string }[] | undefined) => void,
|
|
flags: number
|
|
) => void
|
|
}
|
|
|
|
const addonRequire = createRequire(import.meta.url)
|
|
|
|
function loadAddon(): Addon {
|
|
const packageEntry = addonRequire.resolve('@vscode/windows-process-tree')
|
|
return addonRequire(
|
|
join(packageEntry, '..', '..', 'build', 'Release', 'windows_process_tree.node')
|
|
) as Addon
|
|
}
|
|
|
|
// Why fail rather than skip on win32: the published tarball ships a loadable
|
|
// prebuilt built from unpatched source, and both readers emit byte-identical
|
|
// strings, so a skipping suite would pass against the very binary this patch
|
|
// exists to keep out. On win32 the addon must be present, and it must be ours.
|
|
describe.runIf(process.platform === 'win32')('windows-process-tree command line addon', () => {
|
|
// Why not at collection time: a require that throws there fails the whole
|
|
// file, and the patch-text cases above need no binary at all -- a Windows
|
|
// checkout without a built addon would lose them to an unrelated failure.
|
|
let addon: Addon
|
|
beforeAll(() => {
|
|
addon = loadAddon()
|
|
})
|
|
const children: { kill: () => void }[] = []
|
|
afterAll(() => {
|
|
for (const child of children) {
|
|
try {
|
|
child.kill()
|
|
} catch {
|
|
// already gone
|
|
}
|
|
}
|
|
})
|
|
|
|
const scan = async (): Promise<Map<number, string>> =>
|
|
new Promise((resolveScan) => {
|
|
addon.getProcessList((rows) => {
|
|
resolveScan(new Map((rows ?? []).map((row) => [row.pid, row.commandLine ?? ''])))
|
|
}, 2 /* ProcessDataFlag.CommandLine */)
|
|
})
|
|
|
|
it('was built from the patched source, not the published prebuild', () => {
|
|
// The patched reader never calls ReadProcessMemory, so the symbol is
|
|
// absent from its import table. This is the only check that tells the two
|
|
// binaries apart -- a bare require() cannot.
|
|
const packageEntry = addonRequire.resolve('@vscode/windows-process-tree')
|
|
const binary = readFileSync(
|
|
join(packageEntry, '..', '..', 'build', 'Release', 'windows_process_tree.node')
|
|
)
|
|
expect(binary.includes('ReadProcessMemory')).toBe(false)
|
|
})
|
|
|
|
it('recovers command lines byte-for-byte, quoting and trailing spaces included', async () => {
|
|
const marker = `orca-cmdline-${Date.now()}`
|
|
// Quotes and trailing whitespace are exactly what a re-quoting bug eats.
|
|
const child = spawn(process.execPath, ['-e', 'setTimeout(() => {}, 20000)', `"${marker}" `], {
|
|
windowsHide: true,
|
|
stdio: 'ignore'
|
|
})
|
|
children.push(child)
|
|
await new Promise((r) => setTimeout(r, 400))
|
|
|
|
const rows = await scan()
|
|
const command = rows.get(child.pid!)
|
|
expect(command).toBeDefined()
|
|
expect(command).toContain(marker)
|
|
expect(command!.endsWith(' "') || command!.endsWith(' ')).toBe(true)
|
|
})
|
|
|
|
it('reports the querying process and most of the table', async () => {
|
|
const rows = await scan()
|
|
expect(rows.has(process.pid)).toBe(true)
|
|
const recovered = [...rows.values()].filter((command) => command.length > 0)
|
|
// Protected and cross-session processes legitimately deny a handle; a
|
|
// wholesale regression would show up as almost nothing recovered.
|
|
expect(recovered.length).toBeGreaterThan(rows.size * 0.25)
|
|
})
|
|
})
|