[Pkg-shadow-devel] Bug#942680: passwd: vipw does not resume properly when suspended

[Todd C. Miller]

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 at buster:~# /usr/sbin/vipw

    [1]+  Stopped                 /usr/sbin/vipw
    root at buster:~# fg
    /usr/sbin/vipw

    [1]+  Stopped                 /usr/sbin/vipw

    root at 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