From f971396c6515fc71d3cc8cdfbf60845c1d1e9e03 Mon Sep 17 00:00:00 2001 From: gystemd Date: Tue, 10 Aug 2021 13:06:51 +0200 Subject: fix missing SIGTTOU signal in SetForegroundProcessGroup --- pkg/sentry/kernel/thread_group.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go index 2eda15303..6d865b814 100644 --- a/pkg/sentry/kernel/thread_group.go +++ b/pkg/sentry/kernel/thread_group.go @@ -489,11 +489,6 @@ func (tg *ThreadGroup) SetForegroundProcessGroup(tty *TTY, pgid ProcessGroupID) tg.signalHandlers.mu.Lock() defer tg.signalHandlers.mu.Unlock() - // TODO(gvisor.dev/issue/6148): "If tcsetpgrp() is called by a member of a - // background process group in its session, and the calling process is not - // blocking or ignoring SIGTTOU, a SIGTTOU signal is sent to all members of - // this background process group." - // tty must be the controlling terminal. if tg.tty != tty { return -1, linuxerr.ENOTTY @@ -516,6 +511,11 @@ func (tg *ThreadGroup) SetForegroundProcessGroup(tty *TTY, pgid ProcessGroupID) return -1, linuxerr.EPERM } + //if the calling process is a member of a background group, a SIGTTOU signal is sent to all members of this background process group. + if tg.processGroup.session.foreground.id != tg.processGroup.id { + tg.processGroup.SendSignal(&linux.SignalInfo{Signo: int32(linux.SIGTTOU)}) + } + tg.processGroup.session.foreground.id = pgid return 0, nil } -- cgit v1.2.3 From 7c5ab794f140dda031ba015d79892a79dd523d50 Mon Sep 17 00:00:00 2001 From: gystemd Date: Mon, 16 Aug 2021 16:33:57 +0200 Subject: fix sending of SIGTTOU signal in SetForegroundProcessGroup Changed sendSignal to sendSignalLocked because tg.pidns.owner.mu and tg.signalHandlers.mu are already locked in SetForegroundProcess Added a control to verify whether the calling process is ignoring SIGTTOU before sending the signal --- pkg/sentry/kernel/thread_group.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go index 6d865b814..c16b9eb37 100644 --- a/pkg/sentry/kernel/thread_group.go +++ b/pkg/sentry/kernel/thread_group.go @@ -511,9 +511,11 @@ func (tg *ThreadGroup) SetForegroundProcessGroup(tty *TTY, pgid ProcessGroupID) return -1, linuxerr.EPERM } - //if the calling process is a member of a background group, a SIGTTOU signal is sent to all members of this background process group. - if tg.processGroup.session.foreground.id != tg.processGroup.id { - tg.processGroup.SendSignal(&linux.SignalInfo{Signo: int32(linux.SIGTTOU)}) + sa:= tg.signalHandlers.actions[linux.SIGTTOU] + //if the calling process is a member of a background group, a SIGTTOU + //signal is sent to all members of this background process group. + if tg.processGroup.id != tg.processGroup.session.foreground.id && sa.Handler!=linux.SIG_IGN{ + tg.leader.sendSignalLocked(SignalInfoPriv(linux.SIGTTOU),true) } tg.processGroup.session.foreground.id = pgid -- cgit v1.2.3 From a5f2ef66d3c7e5ed1fd90cc8670b3e3891ad8af9 Mon Sep 17 00:00:00 2001 From: gystemd Date: Mon, 16 Aug 2021 16:37:00 +0200 Subject: added two system call tests for setForegroundProcessGroup Test the correct sending of the SIGTTOU in setForegroundProcess --- test/syscalls/linux/pty.cc | 51 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/test/syscalls/linux/pty.cc b/test/syscalls/linux/pty.cc index 5ff1f12a0..2e0dc6b3e 100644 --- a/test/syscalls/linux/pty.cc +++ b/test/syscalls/linux/pty.cc @@ -1640,6 +1640,57 @@ TEST_F(JobControlTest, DISABLED_SetForegroundProcessGroup) { ASSERT_NO_ERRNO(res); } +// This test verify if a SIGTTOU signal is sent to the calling process'group +// when tcsetpgrp is called by a background process +TEST_F(JobControlTest, SetForegroundProcessGroupSIGTTOUBackground) { + auto res = RunInChild([=]() { + setsid(); + TEST_PCHECK(!ioctl(replica_.get(), TIOCSCTTY, 0)); + pid_t grandchild = fork(); + if (!grandchild) { + //assign a different pgid to the child so it will result as + //a background process + setpgid(grandchild, getpid()); + tcsetpgrp(replica_.get(), getpgid(0)); + // We should never reach this. + _exit(1); + } + int wstatus; + TEST_PCHECK(waitpid(grandchild, &wstatus, WSTOPPED) == grandchild); + TEST_PCHECK(WSTOPSIG(wstatus)==SIGTTOU); + }); + ASSERT_NO_ERRNO(res); +} + +// This test verify that a SIGTTOU signal is not delivered to +// a background process which calls tcsetpgrp and is ignoring SIGTTOU +TEST_F(JobControlTest, SetForegroundProcessGroupSIGTTOUIgnored) { + auto res = RunInChild([=]() { + setsid(); + TEST_PCHECK(!ioctl(replica_.get(), TIOCSCTTY, 0)); + pid_t grandchild = fork(); + if (!grandchild) { + // ignore SIGTTOU so the child in background won't + // be stopped when it will call tcsetpgrp + struct sigaction sa = {}; + sa.sa_handler = SIG_IGN; + sa.sa_flags = 0; + sigemptyset(&sa.sa_mask); + sigaction(SIGTTOU, &sa, NULL); + //assign a different pgid to the child so it will result as + //a background process + setpgid(grandchild, getpid()); + tcsetpgrp(replica_.get(), getpgid(0)); + _exit(0); + } + int wstatus; + TEST_PCHECK(waitpid(grandchild, &wstatus, WSTOPPED) == grandchild); + TEST_PCHECK(WSTOPSIG(wstatus)!=SIGTTOU); + TEST_PCHECK(WIFEXITED(wstatus)); + }); + ASSERT_NO_ERRNO(res); +} + TEST_F(JobControlTest, SetForegroundProcessGroupWrongTTY) { pid_t pid = getpid(); ASSERT_THAT(ioctl(replica_.get(), TIOCSPGRP, &pid), -- cgit v1.2.3 From 482de52b60d9ac0bc40f264c82c8eb094401a119 Mon Sep 17 00:00:00 2001 From: gystemd Date: Tue, 17 Aug 2021 22:57:39 +0200 Subject: Added a SIGTTOU block check in SetForegroundProcessGroup --- pkg/sentry/kernel/thread_group.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go index c16b9eb37..0f741602d 100644 --- a/pkg/sentry/kernel/thread_group.go +++ b/pkg/sentry/kernel/thread_group.go @@ -511,10 +511,13 @@ func (tg *ThreadGroup) SetForegroundProcessGroup(tty *TTY, pgid ProcessGroupID) return -1, linuxerr.EPERM } - sa:= tg.signalHandlers.actions[linux.SIGTTOU] - //if the calling process is a member of a background group, a SIGTTOU - //signal is sent to all members of this background process group. - if tg.processGroup.id != tg.processGroup.session.foreground.id && sa.Handler!=linux.SIG_IGN{ + signalAction:= tg.signalHandlers.actions[linux.SIGTTOU] + // If the calling process is a member of a background group, a SIGTTOU + // signal is sent to all members of this background process group. + // We need also need to check whether it is ignoring or blocking SIGTTOU. + ignored:= signalAction.Handler == linux.SIG_IGN + blocked:= tg.leader.signalMask == linux.SignalSetOf(linux.SIGTTOU) + if tg.processGroup.id != tg.processGroup.session.foreground.id && !ignored && !blocked{ tg.leader.sendSignalLocked(SignalInfoPriv(linux.SIGTTOU),true) } -- cgit v1.2.3 From ce4e54186e7c4a59bcdd9a74d89c5d4c2a7174f9 Mon Sep 17 00:00:00 2001 From: gystemd Date: Tue, 17 Aug 2021 23:25:47 +0200 Subject: Added a system call test for SetForegroundProcessGroup -Added a test to check if the process in background is blocking SIGTTOU -Some minor formatting fixes --- test/syscalls/linux/pty.cc | 79 +++++++++++++++++++++++++++++++--------------- 1 file changed, 53 insertions(+), 26 deletions(-) diff --git a/test/syscalls/linux/pty.cc b/test/syscalls/linux/pty.cc index 2e0dc6b3e..c008c89e7 100644 --- a/test/syscalls/linux/pty.cc +++ b/test/syscalls/linux/pty.cc @@ -1640,57 +1640,84 @@ TEST_F(JobControlTest, DISABLED_SetForegroundProcessGroup) { ASSERT_NO_ERRNO(res); } -// This test verify if a SIGTTOU signal is sent to the calling process'group +// This test verifies if a SIGTTOU signal is sent to the calling process's group // when tcsetpgrp is called by a background process TEST_F(JobControlTest, SetForegroundProcessGroupSIGTTOUBackground) { auto res = RunInChild([=]() { - setsid(); + TEST_PCHECK(setsid() >= 0); TEST_PCHECK(!ioctl(replica_.get(), TIOCSCTTY, 0)); pid_t grandchild = fork(); if (!grandchild) { - //assign a different pgid to the child so it will result as - //a background process - setpgid(grandchild, getpid()); - tcsetpgrp(replica_.get(), getpgid(0)); - // We should never reach this. - _exit(1); - } + // Assign a different pgid to the child so it will result as + // a background process. + TEST_PCHECK(!setpgid(grandchild, getpid())); + TEST_PCHECK(!tcsetpgrp(replica_.get(), getpgid(0))); + // We should never reach this. + _exit(1); + } int wstatus; TEST_PCHECK(waitpid(grandchild, &wstatus, WSTOPPED) == grandchild); - TEST_PCHECK(WSTOPSIG(wstatus)==SIGTTOU); + TEST_PCHECK(WSTOPSIG(wstatus) == SIGTTOU); }); ASSERT_NO_ERRNO(res); } -// This test verify that a SIGTTOU signal is not delivered to +// This test verifies that a SIGTTOU signal is not delivered to // a background process which calls tcsetpgrp and is ignoring SIGTTOU TEST_F(JobControlTest, SetForegroundProcessGroupSIGTTOUIgnored) { auto res = RunInChild([=]() { - setsid(); + TEST_PCHECK(setsid() >= 0); TEST_PCHECK(!ioctl(replica_.get(), TIOCSCTTY, 0)); pid_t grandchild = fork(); if (!grandchild) { - // ignore SIGTTOU so the child in background won't - // be stopped when it will call tcsetpgrp - struct sigaction sa = {}; - sa.sa_handler = SIG_IGN; - sa.sa_flags = 0; - sigemptyset(&sa.sa_mask); - sigaction(SIGTTOU, &sa, NULL); - //assign a different pgid to the child so it will result as - //a background process - setpgid(grandchild, getpid()); - tcsetpgrp(replica_.get(), getpgid(0)); - _exit(0); - } + // Ignore SIGTTOU so the child in background won't + // be stopped when it will call tcsetpgrp + struct sigaction sa = {}; + sa.sa_handler = SIG_IGN; + sa.sa_flags = 0; + sigemptyset(&sa.sa_mask); + sigaction(SIGTTOU, &sa, NULL); + // Assign a different pgid to the child so it will result as + // a background process. + TEST_PCHECK(!setpgid(grandchild, getpid())); + TEST_PCHECK(!tcsetpgrp(replica_.get(), getpgid(0))); + _exit(0); + } int wstatus; TEST_PCHECK(waitpid(grandchild, &wstatus, WSTOPPED) == grandchild); - TEST_PCHECK(WSTOPSIG(wstatus)!=SIGTTOU); + TEST_PCHECK(WSTOPSIG(wstatus) != SIGTTOU); TEST_PCHECK(WIFEXITED(wstatus)); }); ASSERT_NO_ERRNO(res); } +// This test verifies that a SIGTTOU signal is not delivered to +// a background process which calls tcsetpgrp and is blocking SIGTTOU +TEST_F(JobControlTest, SetForegroundProcessGroupSIGTTOUBlocked) { + auto res = RunInChild([=]() { + TEST_PCHECK(setsid() >= 0); + TEST_PCHECK(!ioctl(replica_.get(), TIOCSCTTY, 0)); + pid_t grandchild = fork(); + if (!grandchild) { + // Block SIGTTOU so the child in background won't + // be stopped when it will call tcsetpgrp + sigset_t signal_set; + sigemptyset(&signal_set); + sigaddset(&signal_set, SIGTTOU); + sigprocmask(SIG_BLOCK, &signal_set, NULL); + // Assign a different pgid to the child so it will result as + // a background process. + TEST_PCHECK(!setpgid(grandchild, getpid())); + TEST_PCHECK(!tcsetpgrp(replica_.get(), getpgid(0))); + _exit(0); + } + int wstatus; + TEST_PCHECK(waitpid(grandchild, &wstatus, WSTOPPED) == grandchild); + TEST_PCHECK(WSTOPSIG(wstatus) != SIGTTOU); + }); + ASSERT_NO_ERRNO(res); +} + TEST_F(JobControlTest, SetForegroundProcessGroupWrongTTY) { pid_t pid = getpid(); ASSERT_THAT(ioctl(replica_.get(), TIOCSPGRP, &pid), -- cgit v1.2.3