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
