From b3f49c06ffaeef24d09c6c08ec8ec8425a0303e2 Mon Sep 17 00:00:00 2001 From: Kevin Parsons Date: Thu, 14 Nov 2019 10:46:01 -0800 Subject: [PATCH] Fix race condition in legacy process stdio code HcsCreateProcess returns a set of stdio handles for the newly created process. A while ago, we used to cache these handles and return them the first time stdio handles were requested for the process, and then get new handles via HcsGetProcessInfo for each subsequent request. At some point, this code was cleaned up to instead always return the original set of handles as non-closable (for new callers) and always get new handles via HcsGetProcessInfo (for legacy callers, who required closable handles). However, this change introduced a race condition for legacy callers, where in the case of a short lived container process, the container could have terminated between when it was started and when the orchestrator requested stdio handles. This led to ERROR_NOT_FOUND being returned from HcsGetProcessInfo. This change addresses this by returning the original handles the first time stdio handles are requested, and then calling HcsGetProcessInfo for every subsequent request (just as it used to work a while ago). Signed-off-by: Kevin Parsons --- container.go | 2 +- internal/hcs/process.go | 26 ++++++++++++++++++++++++-- internal/hcs/system.go | 33 +-------------------------------- 3 files changed, 26 insertions(+), 35 deletions(-) diff --git a/container.go b/container.go index 53c0a3854a..7205a62c5e 100644 --- a/container.go +++ b/container.go @@ -196,7 +196,7 @@ func (container *container) MappedVirtualDisks() (map[int]MappedVirtualDiskContr // CreateProcess launches a new process within the container. func (container *container) CreateProcess(c *ProcessConfig) (Process, error) { - p, err := container.system.CreateProcessNoStdio(c) + p, err := container.system.CreateProcess(context.Background(), c) if err != nil { return nil, convertSystemError(err, container) } diff --git a/internal/hcs/process.go b/internal/hcs/process.go index d366f629f6..2ad978f290 100644 --- a/internal/hcs/process.go +++ b/internal/hcs/process.go @@ -20,6 +20,8 @@ type Process struct { handle vmcompute.HcsProcess processID int system *System + hasCachedStdio bool + stdioLock sync.Mutex stdin io.WriteCloser stdout io.ReadCloser stderr io.ReadCloser @@ -272,8 +274,8 @@ func (process *Process) ExitCode() (int, error) { } // StdioLegacy returns the stdin, stdout, and stderr pipes, respectively. Closing -// these pipes does not close the underlying pipes; but this function can only -// be called once on each Process. +// these pipes does not close the underlying pipes. Once returned, these pipes +// are the responsibility of the caller to close. func (process *Process) StdioLegacy() (_ io.WriteCloser, _ io.ReadCloser, _ io.ReadCloser, err error) { operation := "hcsshim::Process::StdioLegacy" ctx, span := trace.StartSpan(context.Background(), operation) @@ -290,6 +292,15 @@ func (process *Process) StdioLegacy() (_ io.WriteCloser, _ io.ReadCloser, _ io.R return nil, nil, nil, makeProcessError(process, operation, ErrAlreadyClosed, nil) } + process.stdioLock.Lock() + defer process.stdioLock.Unlock() + if process.hasCachedStdio { + stdin, stdout, stderr := process.stdin, process.stdout, process.stderr + process.stdin, process.stdout, process.stderr = nil, nil, nil + process.hasCachedStdio = false + return stdin, stdout, stderr, nil + } + processInfo, resultJSON, err := vmcompute.HcsGetProcessInfo(ctx, process.handle) events := processHcsResult(ctx, resultJSON) if err != nil { @@ -307,6 +318,8 @@ func (process *Process) StdioLegacy() (_ io.WriteCloser, _ io.ReadCloser, _ io.R // Stdio returns the stdin, stdout, and stderr pipes, respectively. // To close them, close the process handle. func (process *Process) Stdio() (stdin io.Writer, stdout, stderr io.Reader) { + process.stdioLock.Lock() + defer process.stdioLock.Unlock() return process.stdin, process.stdout, process.stderr } @@ -340,9 +353,13 @@ func (process *Process) CloseStdin(ctx context.Context) error { return makeProcessError(process, operation, err, events) } + process.stdioLock.Lock() if process.stdin != nil { process.stdin.Close() + process.stdin = nil } + process.stdioLock.Unlock() + return nil } @@ -365,15 +382,20 @@ func (process *Process) Close() (err error) { return nil } + process.stdioLock.Lock() if process.stdin != nil { process.stdin.Close() + process.stdin = nil } if process.stdout != nil { process.stdout.Close() + process.stdout = nil } if process.stderr != nil { process.stderr.Close() + process.stderr = nil } + process.stdioLock.Unlock() if err = process.unregisterCallback(ctx); err != nil { return makeProcessError(process, operation, err, nil) diff --git a/internal/hcs/system.go b/internal/hcs/system.go index 98df25bd51..6300a79742 100644 --- a/internal/hcs/system.go +++ b/internal/hcs/system.go @@ -482,38 +482,6 @@ func (computeSystem *System) createProcess(ctx context.Context, operation string return newProcess(processHandle, int(processInfo.ProcessId), computeSystem), &processInfo, nil } -// CreateProcessNoStdio launches a new process within the computeSystem. The -// Stdio handles are not cached on the process struct. -func (computeSystem *System) CreateProcessNoStdio(c interface{}) (_ cow.Process, err error) { - operation := "hcsshim::System::CreateProcessNoStdio" - ctx, span := trace.StartSpan(context.Background(), operation) - defer span.End() - defer func() { oc.SetSpanStatus(span, err) }() - span.AddAttributes(trace.StringAttribute("cid", computeSystem.id)) - - process, processInfo, err := computeSystem.createProcess(ctx, operation, c) - if err != nil { - return nil, err - } - defer func() { - if err != nil { - process.Close() - } - }() - - // We don't do anything with these handles. Close them so they don't leak. - syscall.Close(processInfo.StdInput) - syscall.Close(processInfo.StdOutput) - syscall.Close(processInfo.StdError) - - if err = process.registerCallback(ctx); err != nil { - return nil, makeSystemError(computeSystem, operation, "", err, nil) - } - go process.waitBackground() - - return process, nil -} - // CreateProcess launches a new process within the computeSystem. func (computeSystem *System) CreateProcess(ctx context.Context, c interface{}) (cow.Process, error) { operation := "hcsshim::System::CreateProcess" @@ -534,6 +502,7 @@ func (computeSystem *System) CreateProcess(ctx context.Context, c interface{}) ( process.stdin = pipes[0] process.stdout = pipes[1] process.stderr = pipes[2] + process.hasCachedStdio = true if err = process.registerCallback(ctx); err != nil { return nil, makeSystemError(computeSystem, operation, "", err, nil)