NetBSD Problem Report #55548

From www@netbsd.org  Fri Aug  7 07:03:54 2020
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 66E6E1A923A
	for <gnats-bugs@gnats.NetBSD.org>; Fri,  7 Aug 2020 07:03:54 +0000 (UTC)
Message-Id: <20200807070353.727451A923B@mollari.NetBSD.org>
Date: Fri,  7 Aug 2020 07:03:53 +0000 (UTC)
From: soumendra@tamu.edu
Reply-To: soumendra@tamu.edu
To: gnats-bugs@NetBSD.org
Subject: [PATCH] script(1): Enable proper playback [-p] of curses sessions
X-Send-Pr-Version: www-1.0

>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
>Last-Modified:  Fri Aug 07 13:40:01 +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:/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

>Audit-Trail:
From: Soumendra Ganguly <soumendra@tamu.edu>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/55548: [PATCH] script(1): Enable proper playback [-p] of
 curses sessions
Date: Fri, 7 Aug 2020 02:17:21 -0500

 I meant /src/lib/libc/gen/isatty.c

 On 8/7/20, gnats-admin@netbsd.org <gnats-admin@netbsd.org> wrote:
 > Thank you very much for your problem report.
 > It has the internal identification `bin/55548'.
 > The individual assigned to look at your
 > report is: bin-bug-people.
 >
 >>Category:       bin
 >>Responsible:    bin-bug-people
 >>Synopsis:       [PATCH] script(1): Enable proper playback [-p] of curses
 >> sessions
 >>Arrival-Date:   Fri Aug 07 07:05:00 +0000 2020
 >
 >

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/55548 CVS commit: src/usr.bin/script
Date: Fri, 7 Aug 2020 09:36:29 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Fri Aug  7 13:36:28 UTC 2020

 Modified Files:
 	src/usr.bin/script: script.c

 Log Message:
 PR/55548: Soumendra Ganguly: Since isatty(3) is implemented using
 tcgetattr(3), call it directly to avoid calling it twice. This
 makes error handling more precise. Also don't call err(3) when
 tcsetattr(3) fails.


 To generate a diff of this commit:
 cvs rdiff -u -r1.24 -r1.25 src/usr.bin/script/script.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

Home	PR Database Search