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 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_FILENO, orig_pgrp) == -1) { > + fprintf (stderr, "%s: %s: %s", Prog, > + "tcsetpgrp", strerror (errno)); > + } > + } > kill (getpid (), SIGSTOP); > /* wake child when resumed */ > - kill (pid, SIGCONT); > + if (editor_pgrp != -1) { > + if (tcsetpgrp(STDIN_FILENO, editor_pgrp) == -1) > { > + fprintf (stderr, "%s: %s: %s", Prog, > + "tcsetpgrp", strerror (errno)); > + } > + } > + killpg (pid, SIGCONT); > } else { > break; > } > } > > + if (orig_pgrp != -1) > + sigprocmask(SIG_SETMASK, &omask, NULL); > + > if (-1 == pid) { > vipwexit (editor, 1, 1); > } else if ( WIFEXITED (status) > > -- System Information: > Debian Release: 10.1 > APT prefers stable-updates > APT policy: (500, 'stable-updates'), (500, 'stable') > Architecture: amd64 (x86_64) > > Kernel: Linux 4.19.0-6-amd64 (SMP w/2 CPU cores) > Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), > LANGUAGE=en_US.UTF-8 (charmap=UTF-8) > Shell: /bin/sh linked to /usr/bin/dash > Init: systemd (via /run/systemd/system) > LSM: AppArmor: enabled > > Versions of packages passwd depends on: > ii libaudit1 1:2.8.4-3 > ii libc6 2.28-10 > ii libpam-modules 1.3.1-5 > ii libpam0g 1.3.1-5 > ii libselinux1 2.8-1+b1 > ii libsemanage1 2.8-2 > > passwd recommends no packages. > > passwd suggests no packages. > > -- no debconf information > > _______________________________________________ > Pkg-shadow-devel mailing list > pkg-shadow-de...@alioth-lists.debian.net > https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-shadow-devel