Bug#942680: [Pkg-shadow-devel] Bug#942680: passwd: vipw does not resume properly when suspended
Control: fixed -1 4.8-1 Control: forwarded -1 https://github.com/shadow-maint/shadow/issues/185 Control: upstream Todd C. Miller ezt írta (időpont: 2019. nov. 4., H, 19:15): > > On Sat, 26 Oct 2019 07:49:33 -0500, "Serge E. Hallyn" wrote: > > > second option sounds nicer but sure is a lot more code. So I'm > > leaning towards the first. Do you mind creating a github issue > > at github.com/shadow-maint/shadow for this, or would you prefer that > > I do it? > > I opened a github issue and attached the patches: > https://github.com/shadow-maint/shadow/issues/185 > > - todd >
Bug#942680: [Pkg-shadow-devel] Bug#942680: passwd: vipw does not resume properly when suspended
On Sat, 26 Oct 2019 07:49:33 -0500, "Serge E. Hallyn" wrote: > second option sounds nicer but sure is a lot more code. So I'm > leaning towards the first. Do you mind creating a github issue > at github.com/shadow-maint/shadow for this, or would you prefer that > I do it? I opened a github issue and attached the patches: https://github.com/shadow-maint/shadow/issues/185 - todd
Bug#942680: [Pkg-shadow-devel] Bug#942680: passwd: vipw does not resume properly when suspended
Thanks,
second option sounds nicer but sure is a lot more code. So I'm
leaning towards the first. Do you mind creating a github issue
at github.com/shadow-maint/shadow for this, or would you prefer that
I do it?
-serge
On Sat, Oct 19, 2019 at 04:20:11PM -0600, Todd C. Miller wrote:
> Package: passwd
> Version: 1:4.5-1.1
> Severity: normal
> Tags: patch
>
> Dear Maintainer,
>
>* What led up to the situation?
>
> If vipw is suspended (e.g. via control-Z) and then resumed, it
> often gets immediately suspended. This is easier to reproduce
> on a multi-core system.
>
>* What exactly did you do (or not do) that was effective (or
> ineffective)?
>
> root@buster:~# /usr/sbin/vipw
>
> [1]+ Stopped /usr/sbin/vipw
> root@buster:~# fg
> /usr/sbin/vipw
>
> [1]+ Stopped /usr/sbin/vipw
>
> root@buster:~# fg
> [vipw resumes on the second fg]
>
> The problem is that vipw forks a child process and calls waitpid()
> with the WUNTRACED flag. When the child process (running the editor)
> is suspended, the parent sends itself SIGSTOP to suspend the main vipw
> process. However, because the main vipw is in the same process group
> as the editor which received the ^Z, the kernel already sent the
> main vipw SIGTSTP.
>
> If the main vipw receives SIGTSTP before the child, it will be
> suspended and then, once resumed, will proceed to suspend itself
> again.
>
> There are two main ways to fix this. I've included patches for
> each approach.
>
> 1) Don't pass the WUNTRACED flag to waitpid() in vipw.c and
>just assume the main vipw will receive a stop signal from
>the kernel. This is the simplest fix and works fine for
>stop signals generated due to ^Z. However, if someone sends
>SIGSTOP to the vipw child process via the kill command, the
>parent vipw will not notice.
>
> --- vipw.c.orig 2019-10-18 16:19:15.673956247 -0600
> +++ vipw.c.simple_fix 2019-10-18 16:43:04.265583507 -0600
> @@ -325,16 +325,9 @@
> }
>
> for (;;) {
> - pid = waitpid (pid, &status, WUNTRACED);
> - if ((pid != -1) && (WIFSTOPPED (status) != 0)) {
> - /* The child (editor) was suspended.
> - * Suspend vipw. */
> - kill (getpid (), SIGSTOP);
> - /* wake child when resumed */
> - kill (pid, SIGCONT);
> - } else {
> + pid = waitpid (pid, &status, 0);
> + if (pid != -1 || errno != EINTR)
> break;
> - }
> }
>
> if (-1 == pid) {
>
> 2) The other option is to run the child process in its own process
>group as the foregroup process group. That way, control-Z will
>only affect the child process and the parent can use the existing
>logic to suspend the parent.
>
>
> --- vipw.c.orig 2019-10-18 16:19:15.673956247 -0600
> +++ vipw.c.pgrp_fix 2019-10-19 12:55:50.526591299 -0600
> @@ -206,6 +206,8 @@
> struct stat st1, st2;
> int status;
> FILE *f;
> + pid_t orig_pgrp, editor_pgrp = -1;
> + sigset_t mask, omask;
> /* FIXME: the following should have variable sizes */
> char filebackup[1024], fileedit[1024];
> char *to_rename;
> @@ -293,6 +295,8 @@
> editor = DEFAULT_EDITOR;
> }
>
> + orig_pgrp = tcgetpgrp(STDIN_FILENO);
> +
> pid = fork ();
> if (-1 == pid) {
> vipwexit ("fork", 1, 1);
> @@ -302,6 +306,14 @@
> char *buf;
> int status;
>
> + /* Wait for parent to make us the foreground pgrp. */
> + if (orig_pgrp != -1) {
> + pid = getpid();
> + setpgid(0, 0);
> + while (tcgetpgrp(STDIN_FILENO) != pid)
> + continue;
> + }
> +
> buf = (char *) malloc (strlen (editor) + strlen (fileedit) + 2);
> snprintf (buf, strlen (editor) + strlen (fileedit) + 2,
> "%s %s", editor, fileedit);
> @@ -324,19 +336,50 @@
> }
> }
>
> + /* Run child in a new pgrp and make it the foreground pgrp. */
> + if (orig_pgrp != -1) {
> + setpgid(pid, pid);
> + tcsetpgrp(STDIN_FILENO, pid);
> +
> + /* Avoid SIGTTOU when changing foreground pgrp below. */
> + sigemptyset(&mask);
> + sigaddset(&mask, SIGTTOU);
> + sigprocmask(SIG_BLOCK, &mask, &omask);
> + }
> +
> for (;;) {
> pid = waitpid (pid, &status, WUNTRACED);
> if ((pid != -1) && (WIFSTOPPED (status) != 0)) {
> /* The child (editor) was suspended.
> - * Suspend vipw. */
> + * Restore terminal pgr
Bug#942680: passwd: vipw does not resume properly when suspended
Package: passwd
Version: 1:4.5-1.1
Severity: normal
Tags: patch
Dear Maintainer,
* What led up to the situation?
If vipw is suspended (e.g. via control-Z) and then resumed, it
often gets immediately suspended. This is easier to reproduce
on a multi-core system.
* What exactly did you do (or not do) that was effective (or
ineffective)?
root@buster:~# /usr/sbin/vipw
[1]+ Stopped /usr/sbin/vipw
root@buster:~# fg
/usr/sbin/vipw
[1]+ Stopped /usr/sbin/vipw
root@buster:~# fg
[vipw resumes on the second fg]
The problem is that vipw forks a child process and calls waitpid()
with the WUNTRACED flag. When the child process (running the editor)
is suspended, the parent sends itself SIGSTOP to suspend the main vipw
process. However, because the main vipw is in the same process group
as the editor which received the ^Z, the kernel already sent the
main vipw SIGTSTP.
If the main vipw receives SIGTSTP before the child, it will be
suspended and then, once resumed, will proceed to suspend itself
again.
There are two main ways to fix this. I've included patches for
each approach.
1) Don't pass the WUNTRACED flag to waitpid() in vipw.c and
just assume the main vipw will receive a stop signal from
the kernel. This is the simplest fix and works fine for
stop signals generated due to ^Z. However, if someone sends
SIGSTOP to the vipw child process via the kill command, the
parent vipw will not notice.
--- vipw.c.orig 2019-10-18 16:19:15.673956247 -0600
+++ vipw.c.simple_fix 2019-10-18 16:43:04.265583507 -0600
@@ -325,16 +325,9 @@
}
for (;;) {
- pid = waitpid (pid, &status, WUNTRACED);
- if ((pid != -1) && (WIFSTOPPED (status) != 0)) {
- /* The child (editor) was suspended.
-* Suspend vipw. */
- kill (getpid (), SIGSTOP);
- /* wake child when resumed */
- kill (pid, SIGCONT);
- } else {
+ pid = waitpid (pid, &status, 0);
+ if (pid != -1 || errno != EINTR)
break;
- }
}
if (-1 == pid) {
2) The other option is to run the child process in its own process
group as the foregroup process group. That way, control-Z will
only affect the child process and the parent can use the existing
logic to suspend the parent.
--- vipw.c.orig 2019-10-18 16:19:15.673956247 -0600
+++ vipw.c.pgrp_fix 2019-10-19 12:55:50.526591299 -0600
@@ -206,6 +206,8 @@
struct stat st1, st2;
int status;
FILE *f;
+ pid_t orig_pgrp, editor_pgrp = -1;
+ sigset_t mask, omask;
/* FIXME: the following should have variable sizes */
char filebackup[1024], fileedit[1024];
char *to_rename;
@@ -293,6 +295,8 @@
editor = DEFAULT_EDITOR;
}
+ orig_pgrp = tcgetpgrp(STDIN_FILENO);
+
pid = fork ();
if (-1 == pid) {
vipwexit ("fork", 1, 1);
@@ -302,6 +306,14 @@
char *buf;
int status;
+ /* Wait for parent to make us the foreground pgrp. */
+ if (orig_pgrp != -1) {
+ pid = getpid();
+ setpgid(0, 0);
+ while (tcgetpgrp(STDIN_FILENO) != pid)
+ continue;
+ }
+
buf = (char *) malloc (strlen (editor) + strlen (fileedit) + 2);
snprintf (buf, strlen (editor) + strlen (fileedit) + 2,
"%s %s", editor, fileedit);
@@ -324,19 +336,50 @@
}
}
+ /* Run child in a new pgrp and make it the foreground pgrp. */
+ if (orig_pgrp != -1) {
+ setpgid(pid, pid);
+ tcsetpgrp(STDIN_FILENO, pid);
+
+ /* Avoid SIGTTOU when changing foreground pgrp below. */
+ sigemptyset(&mask);
+ sigaddset(&mask, SIGTTOU);
+ sigprocmask(SIG_BLOCK, &mask, &omask);
+ }
+
for (;;) {
pid = waitpid (pid, &status, WUNTRACED);
if ((pid != -1) && (WIFSTOPPED (status) != 0)) {
/* The child (editor) was suspended.
-* Suspend vipw. */
+* Restore terminal pgrp and suspend vipw. */
+ if (orig_pgrp != -1) {
+ editor_pgrp = tcgetpgrp(STDIN_FILENO);
+ if (editor_pgrp == -1) {
+ fprintf (stderr, "%s: %s: %s", Prog,
+"tcgetpgrp", strerror (errno));
+ }
+ if (tcsetpgrp(STDIN_F
