From 076bbe1bd2ae6c6915e45e14baf7519b0d481811 Mon Sep 17 00:00:00 2001 From: Dawei Wei Date: Tue, 20 Oct 2026 17:44:08 +0000 Subject: [PATCH] shim: don't tear down shared pod UVM on Hyper-V container restart On Hyper-V isolated Windows pods, a container restart can tear down the shared pod UVM, killing the sandbox and every other container in the pod. A Windows guest holds the WaitForProcess response of a process until the host closes the process's stdio relay. A descendant process (for example the child of "sh -c") can keep the relay open after the process itself has exited, and the host only closes the relay once the wait completes, so neither side makes progress. The SIGKILL that follows returns hrNotFound, which was logged and ignored, leaving the wait pending. The 30s SIGKILL watchdog in KillExec then closed the UVM, even when the task did not own it. Fix both sides: - Gate the KillExec watchdog on ownsHost, so only a task that owns its UVM can close it. A workload container shares the pod UVM. - When a Windows guest reports a process missing while its wait is still pending, wait a grace period for the real exit, then close the host side of the process's stdio relay. The guest then delivers the real exit code; no exit is synthesized. The grace period matches the default CopyAfterExitTimeout the host already allows stdio relays after a process exits. Linux guests are excluded: they drain and close the relay themselves before answering the wait, and reconnect if the host closes it first. Signed-off-by: Dawei Wei --- cmd/containerd-shim-runhcs-v1/task_hcs.go | 5 +- internal/gcs/process.go | 45 ++++- internal/gcs/process_test.go | 218 ++++++++++++++++++++++ 3 files changed, 266 insertions(+), 2 deletions(-) create mode 100644 internal/gcs/process_test.go diff --git a/cmd/containerd-shim-runhcs-v1/task_hcs.go b/cmd/containerd-shim-runhcs-v1/task_hcs.go index afadc50c5e..eb396a7dc9 100644 --- a/cmd/containerd-shim-runhcs-v1/task_hcs.go +++ b/cmd/containerd-shim-runhcs-v1/task_hcs.go @@ -469,11 +469,14 @@ func (ht *hcsTask) KillExec(ctx context.Context, eid string, signal uint32, all return true }) } - if signal == 0x9 && eid == "" && ht.host != nil { + if signal == 0x9 && eid == "" && ht.host != nil && ht.ownsHost { // If this is a SIGKILL against the init process we start a background // timer and wait on either the timer expiring or the process exiting // cleanly. If the timer expires first we forcibly close the UVM as we // assume the guest is misbehaving for some reason. + // + // Only a task that owns its UVM may close it. A workload container + // shares the pod UVM, and closing it would tear down the whole pod. go func() { t := time.NewTimer(30 * time.Second) execExited := make(chan struct{}) diff --git a/internal/gcs/process.go b/internal/gcs/process.go index adc249328e..ecfb68a2bf 100644 --- a/internal/gcs/process.go +++ b/internal/gcs/process.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "sync" + "time" "github.com/Microsoft/go-winio" "github.com/Microsoft/hcsshim/internal/cow" @@ -30,6 +31,12 @@ const ( exitProbeTimeoutMs = 50 ) +// missingProcessExitGrace is how long releaseMissingProcessExit waits for the +// real exit before closing the stdio relay. It matches the default +// CopyAfterExitTimeout the host allows stdio relays after a process exits. +// A variable so tests can shorten it. +var missingProcessExitGrace = time.Second + // Process represents a process in a container or container host. type Process struct { gc *GuestConnection @@ -43,6 +50,8 @@ type Process struct { // stdinPort, stdoutPort, stderrPort are the vsock ports gc.exec // allocated for this process's stdio relay. stdinPort, stdoutPort, stderrPort uint32 + // missingOnce starts releaseMissingProcessExit at most once. + missingOnce sync.Once } var _ cow.Process = &Process{} @@ -294,13 +303,47 @@ func (p *Process) Signal(ctx context.Context, options interface{}) (_ bool, err logrus.ErrorKey: err, logfields.ContainerID: p.cid, logfields.ProcessID: p.id, - }).Warn("ignoring missing process") + }).Warn("process reported missing by guest while its wait is pending") + // gc.os is the OS of the guest (the UVM), not of the host. Only a + // Windows guest (WCOW) holds the wait response until the host + // closes the stdio relay. A Linux guest (LCOW) drains and closes + // the relay itself, and reconnects if the host closes it first. + if p.gc.os == "windows" { + p.missingOnce.Do(func() { + go p.releaseMissingProcessExit(context.WithoutCancel(ctx)) + }) + } } return false, nil } return true, nil } +// releaseMissingProcessExit closes the host side of the stdio relay of a +// process the guest reported missing while its wait is still pending. +// +// A Windows guest holds the wait response of a process until the host closes +// its stdio relay, and a descendant process can keep the relay open after the +// process itself exits. The host only closes the relay once the wait +// completes, so neither side would make progress. The real exit normally +// follows the missing report closely, so give it a short grace period first. +func (p *Process) releaseMissingProcessExit(ctx context.Context) { + t := time.NewTimer(missingProcessExitGrace) + defer t.Stop() + select { + case <-p.waitCall.ch: + return + case <-t.C: + } + log.G(ctx).WithFields(logrus.Fields{ + logfields.ContainerID: p.cid, + logfields.ProcessID: p.id, + }).Warn("closing stdio relay of process reported missing by guest") + _ = p.stdin.Close() + _ = p.stdout.Close() + _ = p.stderr.Close() +} + // Stdio returns the standard IO streams associated with the container. They // will be closed when Close is called. func (p *Process) Stdio() (stdin io.Writer, stdout, stderr io.Reader) { diff --git a/internal/gcs/process_test.go b/internal/gcs/process_test.go new file mode 100644 index 0000000000..167d4d169a --- /dev/null +++ b/internal/gcs/process_test.go @@ -0,0 +1,218 @@ +//go:build windows + +package gcs + +import ( + "context" + "encoding/json" + "io" + "net" + "testing" + "time" + + "github.com/Microsoft/hcsshim/internal/gcs/prot" + "github.com/sirupsen/logrus" +) + +func setMissingProcessExitGrace(t *testing.T, grace time.Duration) { + t.Helper() + old := missingProcessExitGrace + missingProcessExitGrace = grace + t.Cleanup(func() { missingProcessExitGrace = old }) +} + +// newMissingProcessTest returns a started bridge with a process whose +// WaitForProcess is pending, and the guest side of the bridge connection. +func newMissingProcessTest(t *testing.T, guestOS string) (*Process, io.ReadWriteCloser) { + t.Helper() + s, c := pipeConn() + b := newBridge(s, nil, logrus.NewEntry(logrus.StandardLogger())) + p := &Process{ + gc: &GuestConnection{brdg: b, os: guestOS}, + cid: t.Name(), + id: 1068, + waitResp: &prot.ContainerWaitForProcessResponse{}, + } + p.waitCall = &rpc{ + ch: make(chan struct{}), + id: 42, + proc: prot.RPCWaitForProcess, + resp: p.waitResp, + } + b.rpcs[p.waitCall.id] = p.waitCall + b.nextID = p.waitCall.id + 1 + b.Start() + t.Cleanup(func() { b.Close() }) + return p, c +} + +// replySignalNotFound reads the SignalProcess request and answers hrNotFound. +func replySignalNotFound(t *testing.T, c io.ReadWriter) { + t.Helper() + signalID, signalType, _, err := readMessage(c) + if err != nil { + t.Error(err) + return + } + if got := signalType &^ prot.MsgTypeRequest; got != prot.MsgType(prot.RPCSignalProcess) { + t.Errorf("request type = %s, want SignalProcess", signalType) + return + } + result := uint32(hrNotFound) + resp, err := json.Marshal(&prot.ResponseBase{ + Result: int32(result), + ErrorMessage: "Element not found.", + }) + if err != nil { + t.Error(err) + return + } + sendMessage(t, c, signalType^prot.MsgTypeRequest^prot.MsgTypeResponse, signalID, resp) +} + +func sendWaitResponse(t *testing.T, c io.Writer, id int64, exitCode uint32) { + t.Helper() + resp, err := json.Marshal(&prot.ContainerWaitForProcessResponse{ExitCode: exitCode}) + if err != nil { + t.Error(err) + return + } + sendMessage(t, c, prot.MsgType(prot.RPCWaitForProcess)|prot.MsgTypeResponse, id, resp) +} + +func signalMissing(t *testing.T, p *Process) { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + signaled, err := p.Signal(ctx, nil) + if err != nil { + t.Fatalf("Signal() error = %v", err) + } + if signaled { + t.Fatal("Signal() reported signaling a process the guest said was missing") + } +} + +func waitForExit(t *testing.T, p *Process) int { + t.Helper() + select { + case <-p.waitCall.ch: + case <-time.After(10 * time.Second): + t.Fatal("pending WaitForProcess was not completed") + } + exitCode, err := p.ExitCode() + if err != nil { + t.Fatalf("ExitCode() error = %v", err) + } + return exitCode +} + +// newStdioRelay returns a connected host-side relay channel and the guest end. +func newStdioRelay(t *testing.T) (*ioChannel, net.Conn) { + t.Helper() + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + ch := newIoChannel(l) + guest, err := net.Dial("tcp", l.Addr().String()) + if err != nil { + t.Fatal(err) + } + <-ch.ch + t.Cleanup(func() { + guest.Close() + ch.Close() + }) + return ch, guest +} + +// assertRelayOpen checks that the stdio relay still carries guest output. +func assertRelayOpen(t *testing.T, host *ioChannel, guest net.Conn) { + t.Helper() + if _, err := guest.Write([]byte("x")); err != nil { + t.Fatalf("guest write to stdio relay: %v", err) + } + if err := host.c.SetReadDeadline(time.Now().Add(5 * time.Second)); err != nil { + t.Fatal(err) + } + b := make([]byte, 1) + if _, err := io.ReadFull(host, b); err != nil { + t.Fatalf("stdio relay was closed: %v", err) + } +} + +// The guest may answer the signal before sending the wait response of a +// process that just exited. The real exit code must be kept and the stdio +// relay left open. +func TestProcessSignalNotFoundKeepsRealExit(t *testing.T) { + const grace = 20 * time.Millisecond + setMissingProcessExitGrace(t, grace) + p, c := newMissingProcessTest(t, "windows") + var stdoutGuest net.Conn + p.stdout, stdoutGuest = newStdioRelay(t) + go func() { + replySignalNotFound(t, c) + sendWaitResponse(t, c, p.waitCall.id, 137) + reflector(t, c, 0) + }() + + signalMissing(t, p) + if exitCode := waitForExit(t, p); exitCode != 137 { + t.Fatalf("ExitCode() = %d, want real exit code 137", exitCode) + } + time.Sleep(5 * grace) + assertRelayOpen(t, p.stdout, stdoutGuest) +} + +// A Linux guest drains and closes the stdio relay itself before it answers +// the wait, so the host must not close the relay for it. +func TestProcessSignalNotFoundLinuxGuestKeepsStdio(t *testing.T) { + const grace = 20 * time.Millisecond + setMissingProcessExitGrace(t, grace) + p, c := newMissingProcessTest(t, "linux") + var stdoutGuest net.Conn + p.stdout, stdoutGuest = newStdioRelay(t) + signaled := make(chan struct{}) + go func() { + replySignalNotFound(t, c) + <-signaled + sendWaitResponse(t, c, p.waitCall.id, 137) + reflector(t, c, 0) + }() + + signalMissing(t, p) + time.Sleep(5 * grace) + assertRelayOpen(t, p.stdout, stdoutGuest) + close(signaled) + if exitCode := waitForExit(t, p); exitCode != 137 { + t.Fatalf("ExitCode() = %d, want real exit code 137", exitCode) + } +} + +// A Windows guest holds the wait response of a process until the host closes +// the process's stdio relay. Closing it must release the real exit code. +func TestProcessSignalNotFoundClosesStdioToReleaseExit(t *testing.T) { + setMissingProcessExitGrace(t, 10*time.Millisecond) + p, c := newMissingProcessTest(t, "windows") + var stdoutGuest, stderrGuest net.Conn + p.stdout, stdoutGuest = newStdioRelay(t) + p.stderr, stderrGuest = newStdioRelay(t) + const exitCode = 0xC000013A + go func() { + replySignalNotFound(t, c) + for _, guest := range []net.Conn{stdoutGuest, stderrGuest} { + if _, err := io.Copy(io.Discard, guest); err != nil { + t.Error(err) + return + } + } + sendWaitResponse(t, c, p.waitCall.id, exitCode) + reflector(t, c, 0) + }() + + signalMissing(t, p) + if got := waitForExit(t, p); got != exitCode { + t.Fatalf("ExitCode() = %#x, want real exit code %#x", got, exitCode) + } +}