Skip to content

Commit

Permalink
Lock to OS thread when Pdeathsig is set
Browse files Browse the repository at this point in the history 
See golang/go#27505 for context. Pdeathsig
isn't safe to set without locking to the current OS thread, because
otherwise thread termination will send the signal, which isn't the
desired behavior.

I discovered this while troubleshooting a problem that turned out to be
unrelated, but I think it's necessary for correctness.

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
  • Loading branch information
@aaronlehmann
aaronlehmann committed Sep 8, 2022
1 parent 2e979ca commit 1caf1a7
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 26 deletions.
49 changes: 45 additions & 4 deletions monitor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package runc

import (
"os/exec"
"runtime"
"syscall"
"time"
)
Expand All @@ -32,15 +33,18 @@ type Exit struct {
Status int
}

// ProcessMonitor is an interface for process monitoring
// ProcessMonitor is an interface for process monitoring.
//
// It allows daemons using go-runc to have a SIGCHLD handler
// to handle exits without introducing races between the handler
// and go's exec.Cmd
// These methods should match the methods exposed by exec.Cmd to provide
// a consistent experience for the caller
// and go's exec.Cmd.
//
// ProcessMonitor also provides a StartLocked method which is similar to
// Start, but locks the goroutine used to start the process to an OS thread
// (for example: when Pdeathsig is set).
type ProcessMonitor interface {
Start(*exec.Cmd) (chan Exit, error)
StartLocked(*exec.Cmd) (chan Exit, error)
Wait(*exec.Cmd, chan Exit) (int, error)
}

Expand Down Expand Up @@ -72,6 +76,43 @@ func (m *defaultMonitor) Start(c *exec.Cmd) (chan Exit, error) {
return ec, nil
}

// StartLocked is like Start, but locks the goroutine used to start the process to
// the OS thread for use-cases where the parent thread matters to the child process
// (for example: when Pdeathsig is set).
func (m *defaultMonitor) StartLocked(c *exec.Cmd) (chan Exit, error) {
started := make(chan error)
ec := make(chan Exit, 1)
go func() {
runtime.LockOSThread()
defer runtime.UnlockOSThread()

if err := c.Start(); err != nil {
started <- err
return
}
close(started)
var status int
if err := c.Wait(); err != nil {
status = 255
if exitErr, ok := err.(*exec.ExitError); ok {
if ws, ok := exitErr.Sys().(syscall.WaitStatus); ok {
status = ws.ExitStatus()
}
}
}
ec <- Exit{
Timestamp: time.Now(),
Pid: c.Process.Pid,
Status: status,
}
close(ec)
}()
if err := <-started; err != nil {
return nil, err
}
return ec, nil
}

func (m *defaultMonitor) Wait(c *exec.Cmd, ec chan Exit) (int, error) {
e := <-ec
return e.Status, nil
Expand Down
62 changes: 40 additions & 22 deletions runc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,22 @@ const (
type Runc struct {
// Command overrides the name of the runc binary. If empty, DefaultCommand
// is used.
Command string
Root string
Debug bool
Log string
LogFormat Format
Command string
Root string
Debug bool
Log string
LogFormat Format
// PdeathSignal sets a signal the child process will receive when the
// parent dies.
//
// When Pdeathsig is set, command invocations will call runtime.LockOSThread
// to prevent OS thread termination from spuriously triggering the
// signal. See https://github.com/golang/go/issues/27505 and
// https://github.com/golang/go/blob/126c22a09824a7b52c019ed9a1d198b4e7781676/src/syscall/exec_linux.go#L48-L51
//
// A program with GOMAXPROCS=1 might hang because of the use of
// runtime.LockOSThread. Callers should ensure they retain at least one
// unlocked thread.
PdeathSignal syscall.Signal // using syscall.Signal to allow compilation on non-unix (unix.Syscall is an alias for syscall.Signal)
Setpgid bool

Expand All @@ -83,7 +94,7 @@ type Runc struct {

// List returns all containers created inside the provided runc root directory
func (r *Runc) List(context context.Context) ([]*Container, error) {
data, err := cmdOutput(r.command(context, "list", "--format=json"), false, nil)
data, err := r.cmdOutput(r.command(context, "list", "--format=json"), false, nil)
defer putBuf(data)
if err != nil {
return nil, err
Expand All @@ -97,7 +108,7 @@ func (r *Runc) List(context context.Context) ([]*Container, error) {

// State returns the state for the container provided by id
func (r *Runc) State(context context.Context, id string) (*Container, error) {
data, err := cmdOutput(r.command(context, "state", id), true, nil)
data, err := r.cmdOutput(r.command(context, "state", id), true, nil)
defer putBuf(data)
if err != nil {
return nil, fmt.Errorf("%s: %s", err, data.String())
Expand Down Expand Up @@ -157,6 +168,13 @@ func (o *CreateOpts) args() (out []string, err error) {
return out, nil
}

func (r *Runc) startCommand(cmd *exec.Cmd) (chan Exit, error) {
if r.PdeathSignal != 0 {
return Monitor.StartLocked(cmd)
}
return Monitor.Start(cmd)
}

// Create creates a new container and returns its pid if it was created successfully
func (r *Runc) Create(context context.Context, id, bundle string, opts *CreateOpts) error {
args := []string{"create", "--bundle", bundle}
Expand All @@ -174,14 +192,14 @@ func (r *Runc) Create(context context.Context, id, bundle string, opts *CreateOp
cmd.ExtraFiles = opts.ExtraFiles

if cmd.Stdout == nil && cmd.Stderr == nil {
data, err := cmdOutput(cmd, true, nil)
data, err := r.cmdOutput(cmd, true, nil)
defer putBuf(data)
if err != nil {
return fmt.Errorf("%s: %s", err, data.String())
}
return nil
}
ec, err := Monitor.Start(cmd)
ec, err := r.startCommand(cmd)
if err != nil {
return err
}
Expand Down Expand Up @@ -263,14 +281,14 @@ func (r *Runc) Exec(context context.Context, id string, spec specs.Process, opts
opts.Set(cmd)
}
if cmd.Stdout == nil && cmd.Stderr == nil {
data, err := cmdOutput(cmd, true, opts.Started)
data, err := r.cmdOutput(cmd, true, opts.Started)
defer putBuf(data)
if err != nil {
return fmt.Errorf("%w: %s", err, data.String())
}
return nil
}
ec, err := Monitor.Start(cmd)
ec, err := r.startCommand(cmd)
if err != nil {
return err
}
Expand Down Expand Up @@ -309,7 +327,7 @@ func (r *Runc) Run(context context.Context, id, bundle string, opts *CreateOpts)
if opts != nil && opts.IO != nil {
opts.Set(cmd)
}
ec, err := Monitor.Start(cmd)
ec, err := r.startCommand(cmd)
if err != nil {
return -1, err
}
Expand Down Expand Up @@ -382,7 +400,7 @@ func (r *Runc) Stats(context context.Context, id string) (*Stats, error) {
if err != nil {
return nil, err
}
ec, err := Monitor.Start(cmd)
ec, err := r.startCommand(cmd)
if err != nil {
return nil, err
}
Expand All @@ -404,7 +422,7 @@ func (r *Runc) Events(context context.Context, id string, interval time.Duration
if err != nil {
return nil, err
}
ec, err := Monitor.Start(cmd)
ec, err := r.startCommand(cmd)
if err != nil {
rd.Close()
return nil, err
Expand Down Expand Up @@ -448,7 +466,7 @@ func (r *Runc) Resume(context context.Context, id string) error {

// Ps lists all the processes inside the container returning their pids
func (r *Runc) Ps(context context.Context, id string) ([]int, error) {
data, err := cmdOutput(r.command(context, "ps", "--format", "json", id), true, nil)
data, err := r.cmdOutput(r.command(context, "ps", "--format", "json", id), true, nil)
defer putBuf(data)
if err != nil {
return nil, fmt.Errorf("%s: %s", err, data.String())
Expand All @@ -462,7 +480,7 @@ func (r *Runc) Ps(context context.Context, id string) ([]int, error) {

// Top lists all the processes inside the container returning the full ps data
func (r *Runc) Top(context context.Context, id string, psOptions string) (*TopResults, error) {
data, err := cmdOutput(r.command(context, "ps", "--format", "table", id, psOptions), true, nil)
data, err := r.cmdOutput(r.command(context, "ps", "--format", "table", id, psOptions), true, nil)
defer putBuf(data)
if err != nil {
return nil, fmt.Errorf("%s: %s", err, data.String())
Expand Down Expand Up @@ -647,7 +665,7 @@ func (r *Runc) Restore(context context.Context, id, bundle string, opts *Restore
if opts != nil && opts.IO != nil {
opts.Set(cmd)
}
ec, err := Monitor.Start(cmd)
ec, err := r.startCommand(cmd)
if err != nil {
return -1, err
}
Expand Down Expand Up @@ -691,7 +709,7 @@ type Version struct {

// Version returns the runc and runtime-spec versions
func (r *Runc) Version(context context.Context) (Version, error) {
data, err := cmdOutput(r.command(context, "--version"), false, nil)
data, err := r.cmdOutput(r.command(context, "--version"), false, nil)
defer putBuf(data)
if err != nil {
return Version{}, err
Expand Down Expand Up @@ -753,7 +771,7 @@ func (r *Runc) args() (out []string) {
// <stderr>
func (r *Runc) runOrError(cmd *exec.Cmd) error {
if cmd.Stdout != nil || cmd.Stderr != nil {
ec, err := Monitor.Start(cmd)
ec, err := r.startCommand(cmd)
if err != nil {
return err
}
Expand All @@ -763,7 +781,7 @@ func (r *Runc) runOrError(cmd *exec.Cmd) error {
}
return err
}
data, err := cmdOutput(cmd, true, nil)
data, err := r.cmdOutput(cmd, true, nil)
defer putBuf(data)
if err != nil {
return fmt.Errorf("%s: %s", err, data.String())
Expand All @@ -773,14 +791,14 @@ func (r *Runc) runOrError(cmd *exec.Cmd) error {

// callers of cmdOutput are expected to call putBuf on the returned Buffer
// to ensure it is released back to the shared pool after use.
func cmdOutput(cmd *exec.Cmd, combined bool, started chan<- int) (*bytes.Buffer, error) {
func (r *Runc) cmdOutput(cmd *exec.Cmd, combined bool, started chan<- int) (*bytes.Buffer, error) {
b := getBuf()

cmd.Stdout = b
if combined {
cmd.Stderr = b
}
ec, err := Monitor.Start(cmd)
ec, err := r.startCommand(cmd)
if err != nil {
return nil, err
}
Expand Down

0 comments on commit 1caf1a7

Please sign in to comment.