Skip to content

Commit 51f9c48

Browse files
aaronlehmannaaronlehmann
committed
Lock to OS thread when Pdeathsig is set
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 <[email protected]>
1 parent 2e979ca commit 51f9c48

File tree

2 files changed

+118
-26
lines changed

2 files changed

+118
-26
lines changed

‎monitor.go

Lines changed: 45 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package runc
1818

1919
import (
2020
"os/exec"
21+
"runtime"
2122
"syscall"
2223
"time"
2324
)
@@ -32,15 +33,18 @@ type Exit struct {
3233
Status int
3334
}
3435

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

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

79+
// StartLocked is like Start, but locks the goroutine used to start the process to
80+
// the OS thread for use-cases where the parent thread matters to the child process
81+
// (for example: when Pdeathsig is set).
82+
func (m *defaultMonitor) StartLocked(c *exec.Cmd) (chan Exit, error) {
83+
started := make(chan error)
84+
ec := make(chan Exit, 1)
85+
go func() {
86+
runtime.LockOSThread()
87+
defer runtime.UnlockOSThread()
88+
89+
if err := c.Start(); err != nil {
90+
started <- err
91+
return
92+
}
93+
close(started)
94+
var status int
95+
if err := c.Wait(); err != nil {
96+
status = 255
97+
if exitErr, ok := err.(*exec.ExitError); ok {
98+
if ws, ok := exitErr.Sys().(syscall.WaitStatus); ok {
99+
status = ws.ExitStatus()
100+
}
101+
}
102+
}
103+
ec <- Exit{
104+
Timestamp: time.Now(),
105+
Pid: c.Process.Pid,
106+
Status: status,
107+
}
108+
close(ec)
109+
}()
110+
if err := <-started; err != nil {
111+
return nil, err
112+
}
113+
return ec, nil
114+
}
115+
75116
func (m *defaultMonitor) Wait(c *exec.Cmd, ec chan Exit) (int, error) {
76117
e := <-ec
77118
return e.Status, nil

‎runc.go

Lines changed: 73 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"os"
2828
"os/exec"
2929
"path/filepath"
30+
"runtime"
3031
"strconv"
3132
"strings"
3233
"syscall"
@@ -61,11 +62,22 @@ const (
6162
type Runc struct {
6263
// Command overrides the name of the runc binary. If empty, DefaultCommand
6364
// is used.
64-
Command string
65-
Root string
66-
Debug bool
67-
Log string
68-
LogFormat Format
65+
Command string
66+
Root string
67+
Debug bool
68+
Log string
69+
LogFormat Format
70+
// PdeathSignal sets a signal the child process will receive when the
71+
// parent dies.
72+
//
73+
// When Pdeathsig is set, command invocations will call runtime.LockOSThread
74+
// to prevent OS thread termination from spuriously triggering the
75+
// signal. See https://github.com/golang/go/issues/27505 and
76+
// https://github.com/golang/go/blob/126c22a09824a7b52c019ed9a1d198b4e7781676/src/syscall/exec_linux.go#L48-L51
77+
//
78+
// A program with GOMAXPROCS=1 might hang because of the use of
79+
// runtime.LockOSThread. Callers should ensure they retain at least one
80+
// unlocked thread.
6981
PdeathSignal syscall.Signal // using syscall.Signal to allow compilation on non-unix (unix.Syscall is an alias for syscall.Signal)
7082
Setpgid bool
7183

@@ -83,7 +95,7 @@ type Runc struct {
8395

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

98110
// State returns the state for the container provided by id
99111
func (r *Runc) State(context context.Context, id string) (*Container, error) {
100-
data, err := cmdOutput(r.command(context, "state", id), true, nil)
112+
data, err := r.cmdOutput(r.command(context, "state", id), true, nil)
101113
defer putBuf(data)
102114
if err != nil {
103115
return nil, fmt.Errorf("%s: %s", err, data.String())
@@ -157,6 +169,13 @@ func (o *CreateOpts) args() (out []string, err error) {
157169
return out, nil
158170
}
159171

172+
func (r *Runc) startCommand(cmd *exec.Cmd) (chan Exit, error) {
173+
if r.PdeathSignal != 0 {
174+
return Monitor.StartLocked(cmd)
175+
}
176+
return Monitor.Start(cmd)
177+
}
178+
160179
// Create creates a new container and returns its pid if it was created successfully
161180
func (r *Runc) Create(context context.Context, id, bundle string, opts *CreateOpts) error {
162181
args := []string{"create", "--bundle", bundle}
@@ -174,14 +193,18 @@ func (r *Runc) Create(context context.Context, id, bundle string, opts *CreateOp
174193
cmd.ExtraFiles = opts.ExtraFiles
175194

176195
if cmd.Stdout == nil && cmd.Stderr == nil {
177-
data, err := cmdOutput(cmd, true, nil)
196+
data, err := r.cmdOutput(cmd, true, nil)
178197
defer putBuf(data)
179198
if err != nil {
180199
return fmt.Errorf("%s: %s", err, data.String())
181200
}
182201
return nil
183202
}
184-
ec, err := Monitor.Start(cmd)
203+
if r.PdeathSignal != 0 {
204+
runtime.LockOSThread()
205+
defer runtime.UnlockOSThread()
206+
}
207+
ec, err := r.startCommand(cmd)
185208
if err != nil {
186209
return err
187210
}
@@ -263,14 +286,18 @@ func (r *Runc) Exec(context context.Context, id string, spec specs.Process, opts
263286
opts.Set(cmd)
264287
}
265288
if cmd.Stdout == nil && cmd.Stderr == nil {
266-
data, err := cmdOutput(cmd, true, opts.Started)
289+
data, err := r.cmdOutput(cmd, true, opts.Started)
267290
defer putBuf(data)
268291
if err != nil {
269292
return fmt.Errorf("%w: %s", err, data.String())
270293
}
271294
return nil
272295
}
273-
ec, err := Monitor.Start(cmd)
296+
if r.PdeathSignal != 0 {
297+
runtime.LockOSThread()
298+
defer runtime.UnlockOSThread()
299+
}
300+
ec, err := r.startCommand(cmd)
274301
if err != nil {
275302
return err
276303
}
@@ -309,7 +336,11 @@ func (r *Runc) Run(context context.Context, id, bundle string, opts *CreateOpts)
309336
if opts != nil && opts.IO != nil {
310337
opts.Set(cmd)
311338
}
312-
ec, err := Monitor.Start(cmd)
339+
if r.PdeathSignal != 0 {
340+
runtime.LockOSThread()
341+
defer runtime.UnlockOSThread()
342+
}
343+
ec, err := r.startCommand(cmd)
313344
if err != nil {
314345
return -1, err
315346
}
@@ -382,7 +413,11 @@ func (r *Runc) Stats(context context.Context, id string) (*Stats, error) {
382413
if err != nil {
383414
return nil, err
384415
}
385-
ec, err := Monitor.Start(cmd)
416+
if r.PdeathSignal != 0 {
417+
runtime.LockOSThread()
418+
defer runtime.UnlockOSThread()
419+
}
420+
ec, err := r.startCommand(cmd)
386421
if err != nil {
387422
return nil, err
388423
}
@@ -404,7 +439,11 @@ func (r *Runc) Events(context context.Context, id string, interval time.Duration
404439
if err != nil {
405440
return nil, err
406441
}
407-
ec, err := Monitor.Start(cmd)
442+
if r.PdeathSignal != 0 {
443+
runtime.LockOSThread()
444+
defer runtime.UnlockOSThread()
445+
}
446+
ec, err := r.startCommand(cmd)
408447
if err != nil {
409448
rd.Close()
410449
return nil, err
@@ -448,7 +487,7 @@ func (r *Runc) Resume(context context.Context, id string) error {
448487

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

463502
// Top lists all the processes inside the container returning the full ps data
464503
func (r *Runc) Top(context context.Context, id string, psOptions string) (*TopResults, error) {
465-
data, err := cmdOutput(r.command(context, "ps", "--format", "table", id, psOptions), true, nil)
504+
data, err := r.cmdOutput(r.command(context, "ps", "--format", "table", id, psOptions), true, nil)
466505
defer putBuf(data)
467506
if err != nil {
468507
return nil, fmt.Errorf("%s: %s", err, data.String())
@@ -647,7 +686,11 @@ func (r *Runc) Restore(context context.Context, id, bundle string, opts *Restore
647686
if opts != nil && opts.IO != nil {
648687
opts.Set(cmd)
649688
}
650-
ec, err := Monitor.Start(cmd)
689+
if r.PdeathSignal != 0 {
690+
runtime.LockOSThread()
691+
defer runtime.UnlockOSThread()
692+
}
693+
ec, err := r.startCommand(cmd)
651694
if err != nil {
652695
return -1, err
653696
}
@@ -691,7 +734,7 @@ type Version struct {
691734

692735
// Version returns the runc and runtime-spec versions
693736
func (r *Runc) Version(context context.Context) (Version, error) {
694-
data, err := cmdOutput(r.command(context, "--version"), false, nil)
737+
data, err := r.cmdOutput(r.command(context, "--version"), false, nil)
695738
defer putBuf(data)
696739
if err != nil {
697740
return Version{}, err
@@ -753,7 +796,11 @@ func (r *Runc) args() (out []string) {
753796
// <stderr>
754797
func (r *Runc) runOrError(cmd *exec.Cmd) error {
755798
if cmd.Stdout != nil || cmd.Stderr != nil {
756-
ec, err := Monitor.Start(cmd)
799+
if r.PdeathSignal != 0 {
800+
runtime.LockOSThread()
801+
defer runtime.UnlockOSThread()
802+
}
803+
ec, err := r.startCommand(cmd)
757804
if err != nil {
758805
return err
759806
}
@@ -763,7 +810,7 @@ func (r *Runc) runOrError(cmd *exec.Cmd) error {
763810
}
764811
return err
765812
}
766-
data, err := cmdOutput(cmd, true, nil)
813+
data, err := r.cmdOutput(cmd, true, nil)
767814
defer putBuf(data)
768815
if err != nil {
769816
return fmt.Errorf("%s: %s", err, data.String())
@@ -773,14 +820,18 @@ func (r *Runc) runOrError(cmd *exec.Cmd) error {
773820

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

779826
cmd.Stdout = b
780827
if combined {
781828
cmd.Stderr = b
782829
}
783-
ec, err := Monitor.Start(cmd)
830+
if r.PdeathSignal != 0 {
831+
runtime.LockOSThread()
832+
defer runtime.UnlockOSThread()
833+
}
834+
ec, err := r.startCommand(cmd)
784835
if err != nil {
785836
return nil, err
786837
}

0 commit comments

Comments
 (0)