Skip to content

wsl: terminate over COM, and stop demoting on service errors - #408

Merged
zcsizmadia merged 1 commit into
mainfrom
feat/356-com-exec
Sep 18, 2026
Merged

zcsizmadia merged 1 commit into
mainfrom
feat/356-com-exec

Conversation

@zcsizmadia

Copy link
Copy Markdown
Collaborator

Part of #356. Adds GetDistributionId and TerminateDistribution to the COM fast path — and records why Exec is not joining them, so the issue can be scoped honestly rather than left open on a hope.

Exec is not tractable

The issue and the existing code both suspected CreateLxProcess was the hard part. Reading the real signature in wslservice.idl settles it:

24 parameters, four returned sockets (stdin, stdout, stderr and a CommunicationChannel), a separate InteropSocket, a process handle and a server handle.

Driving that means reimplementing the relay and channel protocol wsl.exe already implements, against an interface whose stability Microsoft disclaims, for calls that are not on a hot loop. The spawn is the better trade. It is now stated in the Fast doc comment so nobody re-opens it hopefully.

This corrects a claim I made when proposing the sprint. I argued #356 would attack the remaining #398 start latency, because every Exec is 165 ms warm / 2,963 ms cold. That is not deliverable. The enabler argument still holds, and is stronger than I thought — see below.

The numbers, measured on this host

GetDistributionId          0.54 ms      (List, for scale: 0.66-0.81 ms)
Terminate over COM        10-16 ms
Terminate via wsl.exe       ~99 ms

~89 ms saved, all of it process spawn. The remaining ~15 ms is the service genuinely stopping the distro, which the CLI pays too. That is why this is 9× where List is 85×: List is a pure query, so spawn is the entire cost.

As latency this is marginal — Terminate runs on skrog stop and restarts, not in a loop, and 89 ms is under 2% of the 4.5–5.5 s start path. The real returns are error quality and GetDistributionId itself: every GUID-taking method needs that lookup, and it now exists at half a millisecond. #381 (ExportDistributionPipe, RegisterDistributionPipe), #382 (SetSparse, ResizeDistribution) and #383 (AttachDisk, MountDisk) all sit directly on it.

A bug the live test caught, that offline tests structurally could not

Terminating a distro that does not exist is the service answering, not the interface having moved. My first version treated it as the latter: it demoted COM for the rest of the process and re-ran the doomed operation through wsl.exe to produce a second, localised error.

before:  wsl --terminate skrog-no-such-distro: exit status 0xffffffff:
         There is no distribution with the supplied name.
after:   GetDistributionId: HRESULT 0x80040302

ServiceError now separates "the service replied with an error" from "the plumbing failed", and only the second demotes. The IID gate already covers slot correctness, so a call that came back at all is evidence the surface is intact, whatever it came back with.

Offline tests could not have found this: they exercise the fallback against a fake, and the fake has no opinion about what kind of error it returns.

Slot safety

Anchored on the one already proven. EnumerateDistributions is declaration 13 with three IUnknown methods ahead of it, so slot 15 — the existing constant, verified working against the live service. Counting from that anchor gives GetDistributionId = 6 and TerminateDistribution = 7. The IID gate covers these exactly as it covers slot 15.

The live tests assert the distro actually stopped, not that the call returned S_OK. A wrong slot would invoke a different method on a live object, and only a state assertion catches that.

Verified

  • go test -tags wslcom ./internal/wsl/ against the real wslservice: terminate works and the state changes; an unknown distro produces an HRESULT with no fallback and no demotion; latency and the lookup/work split are logged.
  • go test ./... and lint.ps1 green. Engine restored and running.

Leaving #356 open

Exec and Start were the bulk of the issue's ambition and are now explicitly out of scope with a reason. What remains worth doing under it is the rest of the GUID-taking surface, which #381/#382/#383 will pull in as they need it. Worth deciding whether to narrow the title or close it in favour of those three.

Adds GetDistributionId and TerminateDistribution to the COM fast path, and
records why Exec is not joining them.

## Exec is not tractable, and that is now written down

The issue and the existing code both suspected CreateLxProcess was the hard
part. Reading the real signature in wslservice.idl settles it: 24 parameters,
four RETURNED sockets (stdin, stdout, stderr, CommunicationChannel), a separate
InteropSocket, a process handle and a server handle. Driving it means
reimplementing the relay and channel protocol wsl.exe already implements,
against an interface whose stability Microsoft disclaims, for calls that are
not on a hot loop. The spawn is the better trade. Said so in the Fast doc
comment so nobody re-opens it hopefully.

## What the numbers actually are

  GetDistributionId          0.54 ms   (List, for scale: 0.66-0.81 ms)
  Terminate over COM        10-16 ms
  Terminate via wsl.exe     ~99 ms

So ~89 ms saved, all of it process spawn -- the remaining ~15 ms is the service
genuinely stopping the distro, which the CLI pays too. That is why this is 9x
and List is 85x: List is a pure query where spawn IS the cost.

Marginal as latency: terminate runs on `skrog stop` and restarts, not in a
loop. The real returns are error quality, and GetDistributionId itself -- every
GUID-taking method (#381's ExportDistributionPipe and RegisterDistributionPipe,
#382's SetSparse and ResizeDistribution, #383's AttachDisk) needs exactly this
lookup, and now has it at half a millisecond.

## A bug the live test caught, that offline tests structurally could not

Terminating a distro that does not exist is the service ANSWERING, not the
interface having moved. The first version treated it as the latter: it demoted
COM for the rest of the process and re-ran the doomed operation through wsl.exe
to produce a second, localised error. Visible only against a real service.

ServiceError now separates "the service replied with an error" from "the
plumbing failed", and only the second demotes. The IID gate already covers slot
correctness, so a call that came back at all is evidence the surface is intact,
whatever it came back with. Error text went from matching English prose to
`GetDistributionId: HRESULT 0x80040302`.

Slots are anchored on the one already proven -- EnumerateDistributions is
declaration 13, three IUnknown methods ahead, so slot 15, which is the existing
verified constant. Counting from that anchor gives 6 and 7.

Live tests assert the distro actually STOPPED rather than that the call
returned S_OK: a wrong slot would call a different method on a live object, and
only a state assertion catches that.
@zcsizmadia
zcsizmadia merged commit 8a52514 into main Sep 18, 2026
3 checks passed
@zcsizmadia
zcsizmadia deleted the feat/356-com-exec branch September 18, 2026 20:43
zcsizmadia added a commit that referenced this pull request Sep 18, 2026
* fix: three defects the independent release review found, all shipped today

An independent 4-agent review of main (correctness / security / concurrency /
release readiness) ahead of dropping the pre-release flag. These three were
confirmed against the code and are mine from the last two days.

1. deny-unattributable-builds (#376) was a COMPLETE no-op in the product.

   Watcher.DenyBuild returned a hardcoded ("", false). Watcher -- not Rules --
   is what the bridge installs as its gate (proxy.go, serve.go, supervise.go
   and wslcbackend.go all pass it), so the rule shipped, gained 176 lines of
   documentation, was reported active by `policy show`, and did nothing.

   The comment explaining the hardcode outlived its reason: it predated #376
   giving Rules.DenyBuild something to say.

   Every test in build_test.go called Rules.DenyBuild(). Nothing called the
   method the product reaches. The `var _ pipeproxy.ImageGate` guard -- which
   exists precisely to stop this gate going quiet -- only proves the method
   exists, not that it consults anything.

   combinedGate.DenyBuild had the same gap on wslc: DenyPull and DenyPush
   consult both layers, DenyBuild consulted only WSL's.

2. The machine-wide policy layer (#386) failed OPEN on an unreadable file.

   The user layer sets `unknown` and refuses when nothing has ever parsed
   (#254). refreshMachineLocked had no equivalent, so machineRules stayed the
   zero value and requests were judged as though no fleet policy existed.

   Parse uses KnownFields(true), so a misspelled rule is a hard error rather
   than an ignored key: one typo in an Intune deployment meant every machine
   that received it ran unenforced, with one log line -- while `skrog policy
   show` reported the file as broken. The two disagreeing in that direction is
   the worst available pair of answers.

   machineLastErr is now separate from lastErr; sharing one field let two
   layers erroring alternately defeat the once-only OnError dedupe.

3. internal/wsl stopped building for !windows in #408.

   `terminate` was added to com_windows.go and not to the shim. CI builds only
   windows-latest and windows-11-arm, so nothing caught it, and I had run
   GOOS=linux against a different package and moved on. com_other.go now
   carries a note that a method added on one side must be added on the other,
   with the command that checks it.

Each fix has a test that fails against the old code -- verified by reverting
the fix and watching the test catch it, rather than assuming it would.

* docs: the command reference described the ACL vulnerability #79 fixed (S1)

`skrog proxy --sddl` help said the default "restricts to SYSTEM, admins and
interactive users". It has not since v0.3: defaultSDDL returns

    D:P(A;;GA;;;SY)(A;;GA;;;BA)(A;;GA;;;<owning user SID>)

with no IU ACE, and dacl_windows_test.go asserts ";IU)" never appears.

That string is published verbatim into docs/reference.md and onto the docs
site, so the command reference told every reader -- including any security
reviewer evaluating this for an enterprise -- that the pipe is open to every
interactive account on the machine. docs/security.md describes exactly that as
a v0.2.0 behaviour that was tightened in v0.3, so the two documents
contradicted each other and the generated one was wrong.

Worth noting what did and did not work here. The CI drift-check did its job
perfectly: it faithfully republished the help text on every change. A
generated doc is only as true as its source string, and nothing checks a
source string against the behaviour it describes.

One line, plus a comment at the flag saying why the wording matters, so the
next person to reword it knows it is load-bearing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant