Skip to content

Commit f2d2ff7

Browse files
EtiennePerotgvisor-bot
authored andcommitted
Ignore sysmsg tgkill errors if the sysmsg thread is already dead.
This can happen in case the entire sandbox is being torn down unceremoniously such that the order of who dies first isn't controlled for. I am not sure if this should also set `sc.subprocess.dead`, as the `tgkill` seems to be specifically about a sysmsg thread associated with this subprocess, not about the subprocess itself. PiperOrigin-RevId: 767338087
1 parent b14059d commit f2d2ff7

File tree

4 files changed

+31
-7
lines changed

4 files changed

+31
-7
lines changed

pkg/sentry/platform/systrap/shared_context.go

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -129,11 +129,12 @@ func (sc *sharedContext) NotifyInterrupt() {
129129
if sc.threadID() == invalidThreadID {
130130
return
131131
}
132-
sc.subprocess.sysmsgThreadsMu.Lock()
133-
defer sc.subprocess.sysmsgThreadsMu.Unlock()
132+
s := sc.subprocess
133+
s.sysmsgThreadsMu.Lock()
134+
defer s.sysmsgThreadsMu.Unlock()
134135

135136
threadID := atomic.LoadUint32(&sc.shared.ThreadID)
136-
sysmsgThread, ok := sc.subprocess.sysmsgThreads[threadID]
137+
sysmsgThread, ok := s.sysmsgThreads[threadID]
137138
if !ok {
138139
// This is either an invalidThreadID or another garbage value; either way we
139140
// don't know which thread to interrupt; best we can do is mark the context.
@@ -142,7 +143,18 @@ func (sc *sharedContext) NotifyInterrupt() {
142143

143144
t := sysmsgThread.thread
144145
if e := hostsyscall.RawSyscallErrno(unix.SYS_TGKILL, uintptr(t.tgid), uintptr(t.tid), uintptr(platform.SignalInterrupt)); e != 0 {
145-
panic(fmt.Sprintf("failed to interrupt the child process %d: %v", t.tid, e))
146+
if e == unix.ESRCH { // Stub thread already killed?
147+
s.dead.Store(true)
148+
if !sc.shared.State.CompareAndSwap(sysmsg.ContextStateNone, sysmsg.ContextStateUnexpectedDeath) {
149+
s.syscallThread.thread.Warningf("failed to set context state to ContextStateUnexpectedDeath; context state was %v", sc.state())
150+
}
151+
s.syscallThreadMu.Lock()
152+
defer s.syscallThreadMu.Unlock()
153+
s.syscallThread.thread.Warningf("Cannot interrupt stub thread %sas it no longer exists; killing syscall thread.", *t.loadLogPrefix())
154+
s.syscallThread.thread.kill()
155+
} else {
156+
panic(fmt.Sprintf("failed to interrupt the child process %d: %v", t.tid, e))
157+
}
146158
}
147159
}
148160

pkg/sentry/platform/systrap/subprocess.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -685,7 +685,7 @@ func (t *thread) wait(outcome waitOutcome) unix.Signal {
685685
}
686686
}
687687

688-
// kill kills the thread;
688+
// kill kills the thread.
689689
func (t *thread) kill() {
690690
unix.Tgkill(int(t.tgid), int(t.tid), unix.Signal(unix.SIGKILL))
691691
}
@@ -863,13 +863,17 @@ func (s *subprocess) switchToApp(c *platformContext, ac *arch.Context64) (isSysc
863863
ctxState = sysmsg.ContextStateSyscall
864864
shouldPatchSyscall = true
865865
}
866-
867866
if ctxState == sysmsg.ContextStateSyscall || ctxState == sysmsg.ContextStateSyscallTrap {
868867
if maybePatchSignalInfo(regs, &c.signalInfo) {
869868
return false, false, hostarch.Execute, nil
870869
}
871870
updateSyscallRegs(regs)
872871
return true, shouldPatchSyscall, hostarch.NoAccess, nil
872+
} else if ctxState == sysmsg.ContextStateUnexpectedDeath {
873+
return false, shouldPatchSyscall, hostarch.NoAccess, &platform.ContextError{
874+
Err: fmt.Errorf("systrap unexpected death"),
875+
Errno: unix.ECHILD,
876+
}
873877
} else if ctxState != sysmsg.ContextStateFault {
874878
return false, false, hostarch.NoAccess, corruptedSharedMemoryErr(fmt.Sprintf("unknown context state: %v", ctxState))
875879
}

pkg/sentry/platform/systrap/sysmsg/sysmsg.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ type Msg struct {
172172
// or ContextStateNone if running/ready-to-run.
173173
type ContextState uint32
174174

175-
// Set atomicaly sets the state value.
175+
// Set atomically sets the state value.
176176
func (s *ContextState) Set(state ContextState) {
177177
atomic.StoreUint32((*uint32)(s), uint32(state))
178178
}
@@ -184,6 +184,11 @@ func (s *ContextState) Get() ContextState {
184184
return ContextState(atomic.LoadUint32((*uint32)(s)))
185185
}
186186

187+
// CompareAndSwap performs a compare-and-swap operation on the state value.
188+
func (s *ContextState) CompareAndSwap(old, new ContextState) bool {
189+
return atomic.CompareAndSwapUint32((*uint32)(s), uint32(old), uint32(new))
190+
}
191+
187192
// Context State types.
188193
const (
189194
// ContextStateNone means that is either running in the user task or is ready
@@ -201,6 +206,8 @@ const (
201206
// ContextStateSyscallCanBePatched means that the syscall can be replaced
202207
// with a function call.
203208
ContextStateSyscallCanBePatched
209+
// ContextStateUnexpectedDeath means a stub thread died unexpectedly.
210+
ContextStateUnexpectedDeath
204211
// ContextStateInvalid is an invalid state that the sentry should never see.
205212
ContextStateInvalid
206213
)

pkg/sentry/platform/systrap/sysmsg/sysmsg.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ enum context_state {
7575
CONTEXT_STATE_FAULT,
7676
CONTEXT_STATE_SYSCALL_TRAP,
7777
CONTEXT_STATE_SYSCALL_NEED_TRAP,
78+
CONTEXT_STATE_UNEXPECTED_DEATH,
7879
CONTEXT_STATE_INVALID,
7980
};
8081

0 commit comments

Comments
 (0)