Merge nucleic/lucid-river-toad-6efj into dev

NucleicBroker/BrokerService.cs

@@ -58,8 +58,11 @@ public sealed class BrokerService : IBrokerEvents
 
         try
         {
-            var result = await DispatchAsync(method, @params, ct).ConfigureAwait(false);
-            if (id is { } requestId)
+            var result = await DispatchAsync(method, @params, id, ct).ConfigureAwait(false);
+            // A null result means the handler already enqueued its own response because it had to
+            // send it before doing something else (proc.exec — see below). Everything else
+            // returns a value and is responded to here.
+            if (result is not null && id is { } requestId)
                 outbound.EnqueueJson(Rpc.Response(requestId, result));
         }
         catch (JsonException e)
@@ -80,7 +83,10 @@ public sealed class BrokerService : IBrokerEvents
         }
     }
 
-    private async Task<object> DispatchAsync(string method, JsonElement? p, CancellationToken ct)
+    /// <summary>Returns the result to respond with, or null when the handler has already
+    /// responded (it needed the response on the wire before continuing).</summary>
+    private async Task<object?> DispatchAsync(
+        string method, JsonElement? p, JsonElement? id, CancellationToken ct)
     {
         switch (method)
         {
@@ -176,7 +182,32 @@ public sealed class BrokerService : IBrokerEvents
                 var procId = Interlocked.Increment(ref nextProcId);
                 var proc = await wslc.ExecAsync(procId, spec, ct).ConfigureAwait(false);
                 lock (procsLock) procs[procId] = proc;
-                return new { procId };
+                // Respond BEFORE running it. Output and exit are enqueued on the same ordered
+                // outbound queue as this response, so a command that finishes fast (`echo`) would
+                // otherwise put `proc.exit` on the wire ahead of the `procId` naming it — and a
+                // client that registers interest when it learns the procId then waits forever.
+                // Hit on the first live run, hardware-confirmed (docs/WINDOWS_PORT.md §13.3).
+                if (id is { } requestId)
+                    outbound.EnqueueJson(Rpc.Response(requestId, new { procId }));
+
+                try
+                {
+                    await proc.StartAsync(ct).ConfigureAwait(false);
+                }
+                catch (Exception e)
+                {
+                    // The response is already on the wire, so this failure CANNOT be reported as a
+                    // JSON-RPC error — that would put two responses under one id, which is a
+                    // protocol violation and left the client waiting for an exit that never came.
+                    // Report it the way the process itself would have: the reason on stderr, then
+                    // an exit. 126 is the shell's "command found but not executable", which is
+                    // what "could not start" means to every caller above.
+                    lock (procsLock) procs.Remove(procId);
+                    var reason = $"nucleic-brokerd: could not start process: {e.Message}\n";
+                    ProcOutput(procId, stderr: true, System.Text.Encoding.UTF8.GetBytes(reason));
+                    ProcExited(procId, 126);
+                }
+                return null;
             }
             case "proc.stdin":
             {

NucleicBroker/IWslc.cs

@@ -64,14 +64,32 @@ public interface IWslc
     /// <summary>cgroup counters for the resource monitor; null when not running.</summary>
     Task<ContainerStatsInfo?> ContainerStatsAsync(string name, CancellationToken ct);
 
-    /// <summary>Start a process in a running container. `procId` is minted by the broker and
-    /// keys every event this process emits through <see cref="IBrokerEvents"/>.</summary>
+    /// <summary>
+    /// Create a process in a running container and attach its event handlers, but **do not run
+    /// it** — the caller runs it with <see cref="IWslcProcess.StartAsync"/> once it has sent the
+    /// `procId` downstream. `procId` is minted by the broker and keys every event this process
+    /// emits through <see cref="IBrokerEvents"/>.
+    ///
+    /// The two-step split is not ceremony. See <see cref="IWslcProcess.StartAsync"/>.
+    /// </summary>
     Task<IWslcProcess> ExecAsync(long procId, ProcSpec spec, CancellationToken ct);
 }
 
 /// <summary>Control half of a running in-container process (output arrives via events).</summary>
 public interface IWslcProcess
 {
+    /// <summary>
+    /// Actually run the process. Separate from <see cref="IWslc.ExecAsync"/> because a short
+    /// command can finish before the `proc.exec` RESPONSE has been written: output and exit ride
+    /// the same ordered outbound queue, so starting first puts `proc.exit` on the wire ahead of
+    /// the `procId` that identifies it, and a client that registers interest on receiving that
+    /// procId waits forever. Observed on hardware with `echo` (docs/WINDOWS_PORT.md §13.3).
+    ///
+    /// This is the same reasoning that makes wslc itself split `CreateProcess` from `Start` — so
+    /// handlers can attach before output flows — applied one level up, to the RPC boundary.
+    /// </summary>
+    Task StartAsync(CancellationToken ct);
+
     Task WriteStdinAsync(ReadOnlyMemory<byte> data, CancellationToken ct);
     Task CloseStdinAsync(CancellationToken ct);
     Task SignalAsync(int signal, CancellationToken ct);
@@ -107,6 +125,17 @@ public sealed class WslcError(string kind, string message) : Exception(message)
     /// permanent capability gap, not a transient failure — hostd must not retry.</summary>
     public const string Unsupported = "unsupported";
 
+    /// <summary>
+    /// A CONTAINER of that name already exists in the session. Distinct from
+    /// <see cref="SessionExists"/> because wslc answers `ERROR_ALREADY_EXISTS` for both and the
+    /// remedies differ completely — remove one container, versus restart the whole WSL stack.
+    ///
+    /// Reachable today because this broker's container roster is process-local (there is no
+    /// enumeration on the compat surface), so a container left behind by a crashed broker holds
+    /// its name against every later one. See docs/WINDOWS_PORT.md §13.3.
+    /// </summary>
+    public const string AlreadyExists = "already_exists";
+
     /// <summary>A session of that name is already running and this facade cannot re-adopt it
     /// (the compat SDK's `Start()` answers ERROR_ALREADY_EXISTS, and its constructor is lazy, so
     /// a second handle is not a second session). Distinct from <see cref="StartFailed"/> because

NucleicBroker/Wslc/WslcFacade.cs

@@ -72,7 +72,17 @@ public sealed class WslcFacade : IWslc
     private readonly Dictionary<string, Entry> containers = [];
     private readonly Lock containersLock = new();
 
-    private sealed record Entry(Sdk.Container Container, string Image);
+    /// <summary>
+    /// A container handle plus what we have learned about it. <c>SetprivWorks</c> is resolved
+    /// lazily on the first uid-dropping exec and then cached — see
+    /// <see cref="ResolvePrivilegeDropAsync"/>.
+    /// </summary>
+    private sealed class Entry(Sdk.Container container, string image)
+    {
+        internal Sdk.Container Container { get; } = container;
+        internal string Image { get; } = image;
+        internal bool? SetprivWorks { get; set; }
+    }
 
     public string? WslcVersion
     {
@@ -541,7 +551,7 @@ public sealed class WslcFacade : IWslc
 
     // MARK: - Processes
 
-    public Task<IWslcProcess> ExecAsync(long procId, ProcSpec spec, CancellationToken ct)
+    public async Task<IWslcProcess> ExecAsync(long procId, ProcSpec spec, CancellationToken ct)
     {
         if (spec.Tty)
             // Not a failure to retry: ProcessSettings has no Terminal and Process has no resize.
@@ -553,7 +563,7 @@ public sealed class WslcFacade : IWslc
         var container = RequireContainer(spec.Container);
         var settings = new Sdk.ProcessSettings
         {
-            CommandLine = WithPrivilegeDrop(spec).ToList(),   // CommandLine, not CmdLine
+            CommandLine = (await WithPrivilegeDropAsync(spec, ct).ConfigureAwait(false)).ToList(),
             OutputMode = Sdk.ProcessOutputMode.Event,
         };
         if (spec.Cwd is { } cwd) settings.WorkingDirectory = cwd;
@@ -578,27 +588,35 @@ public sealed class WslcFacade : IWslc
         process.ErrorReceived += data => events?.ProcOutput(procId, stderr: true, data);
         process.Exited += code => events?.ProcExited(procId, code);
 
-        try
-        {
-            process.Start();
-        }
-        catch (Exception e) when (e is not WslcError)
-        {
-            process.Dispose();
-            throw Translate(e, WslcError.StartFailed);
-        }
-        return Task.FromResult<IWslcProcess>(new WslcProcess(process));
+        // NOT started here — BrokerService starts it after the `procId` response is on the wire.
+        // See IWslcProcess.StartAsync for why that ordering is load-bearing.
+        return new WslcProcess(process);
     }
 
     /// <summary>
     /// <c>ProcessSettings</c> has no <c>UserId</c>/<c>GroupId</c>, so dropping to the agent uid is
     /// done in-guest by wrapping argv — the fallback §3.2 always named, which costs nothing
     /// because the interceptors and nash never look at the numeric uid.
+    ///
+    /// argv is passed to `setpriv` directly rather than through a shell, so nothing here can be
+    /// quoted wrong or injected into.
     /// </summary>
-    private static IReadOnlyList<string> WithPrivilegeDrop(ProcSpec spec)
+    private async Task<IReadOnlyList<string>> WithPrivilegeDropAsync(
+        ProcSpec spec, CancellationToken ct)
     {
         if (spec.Uid is not { } uid || uid == 0) return spec.Argv;
         var gid = spec.Gid ?? uid;
+
+        if (!await ResolvePrivilegeDropAsync(spec.Container, ct).ConfigureAwait(false))
+            // Refuse rather than run the agent as root. This path exists because the alternative
+            // is a silent privilege escalation: the caller asked for uid 501 and got 0, in the
+            // one place the sandbox's user separation is enforced.
+            throw new WslcError(
+                WslcError.Unsupported,
+                $"cannot drop to uid {uid} in this image: it has no util-linux `setpriv` "
+                + "(BusyBox ships a `setpriv` that does not support --reuid). Use an image with "
+                + "util-linux, as the narOS agent image does — refusing to run as root instead.");
+
         return
         [
             "setpriv", $"--reuid={uid}", $"--regid={gid}", "--init-groups", "--",
@@ -606,6 +624,48 @@ public sealed class WslcFacade : IWslc
         ];
     }
 
+    /// <summary>
+    /// Does this image have a `setpriv` that can actually change uid? Probed once per container
+    /// and cached, because the answer is a property of the image and an extra exec per agent
+    /// command would not be free.
+    ///
+    /// The probe is `setpriv --reuid=0 --regid=0 --init-groups -- true`: a no-op on util-linux,
+    /// and an "unrecognized option" failure on BusyBox's namesake, which accepts only capability
+    /// flags. Testing for the *binary* is not enough — BusyBox has one, it just cannot do this
+    /// (observed on hardware, docs/WINDOWS_PORT.md §13.3).
+    /// </summary>
+    private async Task<bool> ResolvePrivilegeDropAsync(string name, CancellationToken ct)
+    {
+        Entry entry;
+        lock (containersLock)
+        {
+            if (!containers.TryGetValue(name, out entry!))
+                throw new WslcError(WslcError.NotFound, $"no container named {name}");
+            if (entry.SetprivWorks is { } cached) return cached;
+        }
+
+        bool works;
+        try
+        {
+            var (exitCode, _) = await RunCapturingAsync(
+                entry.Container,
+                ["setpriv", "--reuid=0", "--regid=0", "--init-groups", "--", "true"],
+                TimeSpan.FromSeconds(15), ct).ConfigureAwait(false);
+            works = exitCode == 0;
+        }
+        catch (Exception)
+        {
+            works = false;
+        }
+
+        lock (containersLock) entry.SetprivWorks = works;
+        if (!works)
+            Console.Error.WriteLine(
+                $"wslc: container '{name}' has no usable setpriv — uid-dropping execs will be "
+                + "refused rather than run as root");
+        return works;
+    }
+
     /// <summary>Run to completion and capture stdout+stderr. The in-guest half of what the macOS
     /// engine's `runCapturing` does, and the only way stats and shim re-seeding work without a
     /// second mechanism.</summary>
@@ -654,6 +714,20 @@ public sealed class WslcFacade : IWslc
 
     private sealed class WslcProcess(Sdk.Process process) : IWslcProcess
     {
+        public Task StartAsync(CancellationToken ct)
+        {
+            try
+            {
+                process.Start();
+            }
+            catch (Exception e) when (e is not WslcError)
+            {
+                process.Dispose();
+                throw Translate(e, WslcError.StartFailed);
+            }
+            return Task.CompletedTask;
+        }
+
         // stdin is a WinRT stream, not a WriteStdin call, and DataWriter is how you put bytes
         // into an IOutputStream. Held for the process lifetime and guarded, because hostd may
         // pipeline proc.stdin writes and StoreAsync is not reentrant.
@@ -752,7 +826,13 @@ public sealed class WslcFacade : IWslc
             0x80040605 => WslcError.NotRunning,    // WSLC_E_CONTAINER_NOT_RUNNING
             0x8004060F => WslcError.NotFound,      // WSLC_E_SESSION_NOT_FOUND
             0x80040607 => WslcError.SessionExists, // WSLC_E_SESSION_RESERVED
-            0x800700B7 => WslcError.SessionExists, // ERROR_ALREADY_EXISTS
+            // ERROR_ALREADY_EXISTS is CONTEXT-FREE: wslc returns it for a session name conflict
+            // AND a container name conflict. It used to map to session_exists here, which made a
+            // stale container report itself as a stuck session — a wrong diagnosis with a wrong
+            // remedy (`wsl --shutdown` instead of removing one container). The session paths catch
+            // this code by number before reaching Translate, so anything arriving here is the
+            // other kind.
+            0x800700B7 => WslcError.AlreadyExists, // ERROR_ALREADY_EXISTS (container name in use)
             // Nothing installed vs. installed-but-too-old: opposite diagnoses, and both mean the
             // sandbox is unusable rather than this call being wrong.
             0x80040154 => WslcError.Unavailable,   // REGDB_E_CLASSNOTREG