bin/55548: [PATCH] script(1): Enable proper playback [-p] of curses sessions

[soumendra%tamu.edu@localhost]

>Number:         55548
>Category:       bin
>Synopsis:       [PATCH] script(1): Enable proper playback [-p] of curses sessions
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Aug 07 07:05:00 +0000 2020
>Originator:     Soumendra Ganguly
>Release:        9.0
>Organization:
Texas A&M University
>Environment:
NetBSD localhost 9.0 NetBSD 9.0 (GENERIC) #0: Fri Feb 14 00:06:28 UTC 2020  mkrepro%mkrepro.NetBSD.org@localhost:/usr/src/sys/arch/amd64/compile/GENERIC amd64
>Description:
/src/blob/trunk/lib/libc/gen/isatty.c suggests that isatty(3) is implemented using tcgetattr(3). This patch makes script(1) avoid the use of isatty(3). Instead, tcgetattr(3) and ioctl(2) (TIOCCGWINSZ) are directly used with more rigorous/fine-tuned error handling based on errno. This seems much more elegant. Also, err(3) is not called upon tcsetattr(3) failure.

Two other requests which are not implemented by this patch.

1. Use sigaction instead of signal.
2. Register a SIGWINCH handler if(isterm)

Note: I am running NetBSD in a virtualbox vm with no X. I currently have no need for #2. However, I have written and tested a similar program on Debian 10 [ GNU/Linux ], which, in the absence of a proper SIGWINCH handler, would incorrectly render the output of certain programs upon resizing the xterm window. For example, the output of ls(1) would become scattered and hard to visually parse.
>How-To-Repeat:

>Fix:
--- src/usr.bin/script/script.c	2020-08-06 22:00:28.907091259 -0500
+++ script.c	2020-08-07 01:33:35.387992609 -0500
@@ -156,17 +156,22 @@
 	if (pflg)
 		playback(fscript);
 
-	isterm = isatty(STDIN_FILENO);
-	if (isterm) {
-		if (tcgetattr(STDIN_FILENO, &tt) == -1)
-			err(EXIT_FAILURE, "tcgetattr");
-		if (ioctl(STDIN_FILENO, TIOCGWINSZ, &win) == -1)
+	if (tcgetattr(STDIN_FILENO, &tt) == -1 ||
+	    ioctl(STDIN_FILENO, TIOCGWINSZ, &win) == -1) {
+		switch (errno) {
+		case ENOTTY:
+			if (openpty(&master, &slave, NULL, NULL, NULL) == -1)
+				err(EXIT_FAILURE, "openpty");
+			break;
+		case EBADF:
+			err(EXIT_FAILURE, "%d not valid fd", STDIN_FILENO);
+		default: /* errno == EFAULT or EINVAL for ioctl. Not reached in practice. */
 			err(EXIT_FAILURE, "ioctl");
-		if (openpty(&master, &slave, NULL, &tt, &win) == -1)
-			err(EXIT_FAILURE, "openpty");
+		}
 	} else {
-		if (openpty(&master, &slave, NULL, NULL, NULL) == -1)
+		if (openpty(&master, &slave, NULL, &tt, &win) == -1)
 			err(EXIT_FAILURE, "openpty");
+		isterm = 1;
 	}
 
 	if (!quiet)
@@ -377,18 +382,17 @@
 {
 	struct termios traw;
 
-	isterm = isatty(STDOUT_FILENO);
-	if (!isterm)
+	if (tcgetattr(STDOUT_FILENO, &tt) == -1) {
+		if (errno == EBADF)
+			err(EXIT_FAILURE, "%d not valid fd", STDOUT_FILENO);
+		/* errno == ENOTTY */
 		return;
-
-	if (tcgetattr(STDOUT_FILENO, &tt) == -1)
-		err(EXIT_FAILURE, "tcgetattr");
-
+	}
+	isterm = 1;
 	traw = tt;
 	cfmakeraw(&traw);
 	traw.c_lflag |= ISIG;
-	if (tcsetattr(STDOUT_FILENO, TCSANOW, &traw) == -1)
-		err(EXIT_FAILURE, "tcsetattr");
+        (void)tcsetattr(STDOUT_FILENO, TCSANOW, &traw);
 }
 
 static void