NetBSD Problem Report #55531

From www@netbsd.org  Sun Aug  2 08:26:20 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 052CF1A9217
	for <gnats-bugs@gnats.NetBSD.org>; Sun,  2 Aug 2020 08:26:20 +0000 (UTC)
Message-Id: <20200802082619.086FB1A923A@mollari.NetBSD.org>
Date: Sun,  2 Aug 2020 08:26:19 +0000 (UTC)
From: soumendra@tamu.edu
Reply-To: soumendra@tamu.edu
To: gnats-bugs@NetBSD.org
Subject: [PATCH] script(1): Check if stdin is a terminal + other error handling
X-Send-Pr-Version: www-1.0

>Number:         55531
>Category:       bin
>Synopsis:       [PATCH] script(1): Check if stdin is a terminal + other error handling
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Aug 02 08:30:00 +0000 2020
>Last-Modified:  Mon Aug 03 03:50: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:
If not using [-p], script(1) assumes that stdin is a terminal. However, that might not be the case. This patch fixes this. It also adds other error handling.

It also adds an extra call of termreset before printing "\nScript done on ..." in playback() [ see PR #55529 ].
>How-To-Repeat:

>Fix:
--- src/usr.bin/script/script.c	2020-08-02 02:06:56.406214345 -0500
+++ script.c	2020-08-02 03:06:46.020439568 -0500
@@ -155,17 +155,28 @@
 	if (pflg)
 		playback(fscript);

-	(void)tcgetattr(STDIN_FILENO, &tt);
-	(void)ioctl(STDIN_FILENO, TIOCGWINSZ, &win);
-	if (openpty(&master, &slave, NULL, &tt, &win) == -1)
-		err(1, "openpty");
+	isterm = isatty(STDIN_FILENO);
+	if (isterm) {
+		if (tcgetattr(STDIN_FILENO, &tt) == -1)
+			err(1, "tcgetattr");
+		if (ioctl(STDIN_FILENO, TIOCGWINSZ, &win) == -1)
+			err(1, "ioctl");
+		if (openpty(&master, &slave, NULL, &tt, &win) == -1)
+			err(1, "openpty");
+	} else {
+		if (openpty(&master, &slave, NULL, NULL, NULL) == -1)
+			err(1, "openpty");		
+	}

 	if (!quiet)
 		(void)printf("Script started, output file is %s\n", fname);
-	rtt = tt;
-	cfmakeraw(&rtt);
-	rtt.c_lflag &= ~ECHO;
-	(void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &rtt);
+
+	if (isterm) {
+		rtt = tt;
+		cfmakeraw(&rtt);
+		rtt.c_lflag &= ~ECHO;
+		(void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &rtt);
+	}

 	(void)signal(SIGCHLD, finish);
 	child = fork();
@@ -301,7 +312,8 @@
 		(void)fclose(fscript);
 		(void)close(master);
 	} else {
-		(void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &tt);
+		if (isterm)
+			(void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &tt);
 		if (!quiet)
 			(void)printf("Script done, output file is %s\n", fname);
 	}
@@ -365,13 +377,13 @@
 	if (!isterm)
 		return;

-	if (tcgetattr(STDOUT_FILENO, &tt) != 0)
+	if (tcgetattr(STDOUT_FILENO, &tt) == -1)
 		err(1, "tcgetattr");

 	traw = tt;
 	cfmakeraw(&traw);
 	traw.c_lflag |= ISIG;
-	if (tcsetattr(STDOUT_FILENO, TCSANOW, &traw) != 0)
+	if (tcsetattr(STDOUT_FILENO, TCSANOW, &traw) == -1)
 		err(1, "tcsetattr");
 }

@@ -431,6 +443,7 @@
 			atexit(termreset);
 			break;
 		case 'e':
+			termreset();
 			if (!quiet)
 				(void)printf("\nScript done on %s",
 				    ctime(&tclock));

>Audit-Trail:
From: Soumendra Ganguly <soumendra@tamu.edu>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/55531: [PATCH] script(1): Check if stdin is a terminal +
 other error handling
Date: Sun, 2 Aug 2020 03:44:03 -0500

 This follows #55529

 On 8/2/20, gnats-admin@netbsd.org <gnats-admin@netbsd.org> wrote:
 > Thank you very much for your problem report.
 > It has the internal identification `bin/55531'.
 > The individual assigned to look at your
 > report is: bin-bug-people.
 >
 >>Category:       bin
 >>Responsible:    bin-bug-people
 >>Synopsis:       [PATCH] script(1): Check if stdin is a terminal + other
 >> error handling
 >>Arrival-Date:   Sun Aug 02 08:30:00 +0000 2020
 >
 >

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/55531 CVS commit: src/usr.bin/script
Date: Sun, 2 Aug 2020 12:23:33 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Sun Aug  2 16:23:33 UTC 2020

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

 Log Message:
 PR/55531: Soumendra Ganguly:
 - add more error handling
 - handle -p when not a terminal
 - call termreset() before printing script done, so that it is printed correctly

 Also:
 - use ssize_t/size_t instead of int
 - use EXIT_SUCCESS/EXIT_FAILURE
 - check result of fork() against -1


 To generate a diff of this commit:
 cvs rdiff -u -r1.22 -r1.23 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.

From: Soumendra Ganguly <soumendra@tamu.edu>
To: gnats-bugs <gnats-bugs@netbsd.org>
Cc: 
Subject: Re: PR/55531 CVS commit: src/usr.bin/script
Date: Sun, 2 Aug 2020 22:11:47 -0500

 Mr. Christos,
          Thank you for accepting the patch and for making the
 additional improvements. Two more things.

 1. err(EXIT_FAILURE, "fstat failed") in line #416 has an unnecessary
 tab character after it.

 2. Are static variables [ such as isterm ] initialized to 0 by the
 compiler automatically?

 Thank you.
 Soumendra

 On 8/2/20, Christos Zoulas <christos@netbsd.org> wrote:
 > The following reply was made to PR bin/55531; it has been noted by GNATS.
 >
 > From: "Christos Zoulas" <christos@netbsd.org>
 > To: gnats-bugs@gnats.NetBSD.org
 > Cc:
 > Subject: PR/55531 CVS commit: src/usr.bin/script
 > Date: Sun, 2 Aug 2020 12:23:33 -0400
 >
 >  Module Name:	src
 >  Committed By:	christos
 >  Date:		Sun Aug  2 16:23:33 UTC 2020
 >
 >  Modified Files:
 >  	src/usr.bin/script: script.c
 >
 >  Log Message:
 >  PR/55531: Soumendra Ganguly:
 >  - add more error handling
 >  - handle -p when not a terminal
 >  - call termreset() before printing script done, so that it is printed
 > correctly
 >
 >  Also:
 >  - use ssize_t/size_t instead of int
 >  - use EXIT_SUCCESS/EXIT_FAILURE
 >  - check result of fork() against -1
 >
 >
 >  To generate a diff of this commit:
 >  cvs rdiff -u -r1.22 -r1.23 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.
 >
 >

From: Christos Zoulas <christos@zoulas.com>
To: gnats-bugs@netbsd.org
Cc: gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 soumendra@tamu.edu
Subject: Re: PR/55531 CVS commit: src/usr.bin/script
Date: Sun, 2 Aug 2020 23:35:40 -0400

 --Apple-Mail=_93D0E84E-E9E5-460F-A9DF-E9564C1C67D9
 Content-Transfer-Encoding: 7bit
 Content-Type: text/plain;
 	charset=us-ascii


 > Mr. Christos,
 >          Thank you for accepting the patch and for making the
 > additional improvements. Two more things.
 > 
 > 1. err(EXIT_FAILURE, "fstat failed") in line #416 has an unnecessary
 > tab character after it.

 There was more than that line, I fixed them all.

 > 2. Are static variables [ such as isterm ] initialized to 0 by the
 > compiler automatically?

 Yes

 Thanks for the patches!

 christos

 --Apple-Mail=_93D0E84E-E9E5-460F-A9DF-E9564C1C67D9
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
 	filename=signature.asc
 Content-Type: application/pgp-signature;
 	name=signature.asc
 Content-Description: Message signed with OpenPGP

 -----BEGIN PGP SIGNATURE-----
 Comment: GPGTools - http://gpgtools.org

 iF0EARECAB0WIQS+BJlbqPkO0MDBdsRxESqxbLM7OgUCXyeGDAAKCRBxESqxbLM7
 OpOTAKCvFnyzPAmRHYQs7JgKjtn+YMpYSwCeOpdK09XAGGoNXQlUwJGzQDl/R/s=
 =Hmmv
 -----END PGP SIGNATURE-----

 --Apple-Mail=_93D0E84E-E9E5-460F-A9DF-E9564C1C67D9--

From: Soumendra Ganguly <soumendra@tamu.edu>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: PR/55531 CVS commit: src/usr.bin/script
Date: Sun, 2 Aug 2020 22:45:59 -0500

 Perfect! You're welcome.

 Soumendra

 On 8/2/20, Christos Zoulas <christos@zoulas.com> wrote:
 > The following reply was made to PR bin/55531; it has been noted by GNATS.
 >
 > From: Christos Zoulas <christos@zoulas.com>
 > To: gnats-bugs@netbsd.org
 > Cc: gnats-admin@netbsd.org,
 >  netbsd-bugs@netbsd.org,
 >  soumendra@tamu.edu
 > Subject: Re: PR/55531 CVS commit: src/usr.bin/script
 > Date: Sun, 2 Aug 2020 23:35:40 -0400
 >
 >  --Apple-Mail=_93D0E84E-E9E5-460F-A9DF-E9564C1C67D9
 >  Content-Transfer-Encoding: 7bit
 >  Content-Type: text/plain;
 >  	charset=us-ascii
 >
 >
 >  > Mr. Christos,
 >  >          Thank you for accepting the patch and for making the
 >  > additional improvements. Two more things.
 >  >
 >  > 1. err(EXIT_FAILURE, "fstat failed") in line #416 has an unnecessary
 >  > tab character after it.
 >
 >  There was more than that line, I fixed them all.
 >
 >  > 2. Are static variables [ such as isterm ] initialized to 0 by the
 >  > compiler automatically?
 >
 >  Yes
 >
 >  Thanks for the patches!
 >
 >  christos
 >
 >  --Apple-Mail=_93D0E84E-E9E5-460F-A9DF-E9564C1C67D9
 >  Content-Transfer-Encoding: 7bit
 >  Content-Disposition: attachment;
 >  	filename=signature.asc
 >  Content-Type: application/pgp-signature;
 >  	name=signature.asc
 >  Content-Description: Message signed with OpenPGP
 >
 >  -----BEGIN PGP SIGNATURE-----
 >  Comment: GPGTools -
 > https://urldefense.com/v3/__http://gpgtools.org__;!!KwNVnqRv!Sq1jHE4-a-Rm33zDFvKmVwtDSNIO04JAxVNwRVBhYGZFVFm8v3NnZeg_PL-fvB3v$
 >
 >
 >  iF0EARECAB0WIQS+BJlbqPkO0MDBdsRxESqxbLM7OgUCXyeGDAAKCRBxESqxbLM7
 >  OpOTAKCvFnyzPAmRHYQs7JgKjtn+YMpYSwCeOpdK09XAGGoNXQlUwJGzQDl/R/s=
 >  =Hmmv
 >  -----END PGP SIGNATURE-----
 >
 >  --Apple-Mail=_93D0E84E-E9E5-460F-A9DF-E9564C1C67D9--
 >
 >

Home	PR Database Search