From f82d97c9ee1a120965d9ec589ca3de8dd547acb3 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Tue, 23 Jan 2024 20:18:41 -0800 Subject: [PATCH] Only reset tty.tg to nil when its controlling process is being released. This means that when tg is being released, IFF tg.tty.tg == tg (which means tg was tg.tty's controlling process), then we can reset tty.tg to nil. Otherwise, as shown in reproducers of #9898, when a non-controlling process exits, it resets the TTY's tg field (which indicates the controlling thread group) and subsequently the alive controlling thread group can no longer receive signals from the TTY. Fixes #9898 PiperOrigin-RevId: 600987817 --- pkg/sentry/kernel/thread_group.go | 4 ++- test/syscalls/linux/pty.cc | 56 ++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go index fba6dbb80..dbd0f6f15 100644 --- a/pkg/sentry/kernel/thread_group.go +++ b/pkg/sentry/kernel/thread_group.go @@ -325,7 +325,9 @@ func (tg *ThreadGroup) Release(ctx context.Context) { // Disassociate from the tty if we have one. if tg.tty != nil { tg.tty.mu.Lock() - tg.tty.tg = nil + if tg.tty.tg == tg { + tg.tty.tg = nil + } tg.tty.mu.Unlock() tg.tty = nil } diff --git a/test/syscalls/linux/pty.cc b/test/syscalls/linux/pty.cc index 6e910a536..b2ee0bcc9 100644 --- a/test/syscalls/linux/pty.cc +++ b/test/syscalls/linux/pty.cc @@ -27,15 +27,16 @@ #include #include +#include #include +#include "gmock/gmock.h" #include "gtest/gtest.h" #include "absl/base/macros.h" #include "absl/strings/str_cat.h" #include "absl/synchronization/notification.h" #include "absl/time/clock.h" #include "absl/time/time.h" -#include "test/util/capability_util.h" #include "test/util/cleanup.h" #include "test/util/file_descriptor.h" #include "test/util/fs_util.h" @@ -1673,6 +1674,59 @@ TEST_F(JobControlTest, ReleaseTTYSignals) { EXPECT_THAT(kill(diff_pgrp_child, SIGKILL), SyscallSucceeds()); } +// Used by the child process spawned in +// ControllingProcessPersistsAfterChildExists to track received signals. +static int received2; + +void sig_handler2(int signum) { received2 |= signum; } + +// NOTE(gvisor.dev/issue/9898): Regression test. Tests that a TTY's controlling +// process is not cleared when a non-controlling process exits. +TEST_F(JobControlTest, ControllingProcessPersistsAfterChildExists) { + // Set the controlling process for the PTY. + ASSERT_THAT(ioctl(replica_.get(), TIOCSCTTY, 0), SyscallSucceeds()); + + // Fork a child, which does nothing and exits. We expect that this process + // is still the controlling process, so is capable of receiving signals. + ASSERT_NO_ERRNO(RunInChild([=]() {})); + + // Install handler for SIGINT. + received2 = 0; + struct sigaction sa = {}; + sa.sa_handler = sig_handler2; + sa.sa_flags = 0; + sigemptyset(&sa.sa_mask); + sigaddset(&sa.sa_mask, SIGINT); + ASSERT_THAT(sigaction(SIGINT, &sa, NULL), SyscallSucceeds()); + + // Send ^C. + constexpr char kInput = ControlCharacter('C'); + ASSERT_THAT(WriteFd(master_.get(), &kInput, 1), SyscallSucceedsWithValue(1)); + + // Ensure we got the signal. Wait at most for kTimeout. + absl::Time end = absl::Now() + kTimeout; + while (received2 != SIGINT) { + absl::SleepFor(absl::Seconds(1)); + if (end < absl::Now()) { + FAIL() << "Timed out waiting for SIGINT signal."; + break; + } + } + + // Make sure we're ignoring SIGHUP, which will be sent to this process once we + // disconnect the TTY. + struct sigaction sighup_sa = {}; + sighup_sa.sa_handler = SIG_IGN; + sighup_sa.sa_flags = 0; + sigemptyset(&sighup_sa.sa_mask); + struct sigaction old_sa; + EXPECT_THAT(sigaction(SIGHUP, &sighup_sa, &old_sa), SyscallSucceeds()); + + // Release the controlling terminal and restore the old SIGHUP handler. + EXPECT_THAT(ioctl(replica_.get(), TIOCNOTTY), SyscallSucceeds()); + EXPECT_THAT(sigaction(SIGHUP, &old_sa, NULL), SyscallSucceeds()); +} + TEST_F(JobControlTest, GetForegroundProcessGroup) { auto res = RunInChild([=]() { pid_t pid, foreground_pgid;