NetBSD Problem Report #56254

From www@netbsd.org  Wed Jun 16 23:55:48 2021
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_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 F1B8B1A921F
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 16 Jun 2021 23:55:47 +0000 (UTC)
Message-Id: <20210616235546.B36BA1A923C@mollari.NetBSD.org>
Date: Wed, 16 Jun 2021 23:55:46 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: script(1) abuses non-async-signal-safe functions in signal handlers
X-Send-Pr-Version: www-1.0

>Number:         56254
>Category:       bin
>Synopsis:       script(1) abuses non-async-signal-safe functions in signal handlers
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Jun 17 00:00:01 +0000 2021
>Last-Modified:  Fri Feb 11 21:20:01 +0000 2022
>Originator:     Taylor R Campbell
>Release:        9.99.x
>Organization:
>Environment:
>Description:
The script(1) command sets up signal handlers that call functions which are not async-signal-safe, such as exit():

https://nxr.netbsd.org/xref/src/usr.bin/script/script.c?r=1.28#182
https://nxr.netbsd.org/xref/src/usr.bin/script/script.c?r=1.28#232
https://nxr.netbsd.org/xref/src/usr.bin/script/script.c?r=1.28#333

This can cause a deadlock in rtld:

https://mail-index.netbsd.org/current-users/2021/06/14/msg041121.html

script(1) should use only async-signal-safe functions in signal handlers -- for example, it could set a flag in the signal handler and check the flags whenever any blocking syscall fails with EINTR.
>How-To-Repeat:
code inspection
>Fix:
Yes, please!

>Audit-Trail:
From: RVP <rvp@SDF.ORG>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/56254: script(1) abuses non-async-signal-safe functions in
 signal handlers
Date: Thu, 17 Jun 2021 07:17:31 +0000 (UTC)

 On Thu, 17 Jun 2021, campbell+netbsd@mumble.net wrote:

 > The script(1) command sets up signal handlers that call functions which are not async-signal-safe, such as exit():
 >
 > https://nxr.netbsd.org/xref/src/usr.bin/script/script.c?r=1.28#182
 > https://nxr.netbsd.org/xref/src/usr.bin/script/script.c?r=1.28#232
 > https://nxr.netbsd.org/xref/src/usr.bin/script/script.c?r=1.28#333
 >
 > This can cause a deadlock in rtld:
 >
 > https://mail-index.netbsd.org/current-users/2021/06/14/msg041121.html
 >
 > script(1) should use only async-signal-safe functions in signal handlers -- for example, it could set a flag in the signal handler and check the flags whenever any blocking syscall fails with EINTR.
 >

 Since script(1) a) isn't using atexit() and b) is flushing the script
 file, I think we can just use _exit() instead of exit() here:

 diff -urN a/script/script.c b/script/script.c
 --- a/script/script.c	2020-08-31 15:32:15.000000000 +0000
 +++ b/script/script.c	2021-06-17 06:51:13.253662302 +0000
 @@ -330,7 +330,7 @@
   		if (!quiet)
   			(void)printf("Script done, output file is %s\n", fname);
   	}
 -	exit(EXIT_SUCCESS);
 +	_exit(EXIT_SUCCESS);
   }

   static void

 Otherwise, I have a more elaborate version (the std. sig_t() -> flag ->
 check_flag_in_main -> exit() stuff) which I can supply.

 Much thanks to riastradh@ for spotting the _real issue_ causing this PR.

 -RVP

From: RVP <rvp@SDF.ORG>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/56254: script(1) abuses non-async-signal-safe functions in
 signal handlers
Date: Thu, 17 Jun 2021 09:50:36 +0000 (UTC)

 On Thu, 17 Jun 2021, RVP wrote:

 > Since script(1) a) isn't using atexit() and b) is flushing the script
 > file, I think we can just use _exit() instead of exit() here:
 >

 On reflection, probably not: since printf & cohorts are being called.

 Here's one I made earlier:

 --- START PATCH ---
 diff -urN a/src/usr.bin/script/script.c b/src/usr.bin/script/script.c
 --- a/src/usr.bin/script/script.c	2020-08-31 15:32:15.000000000 +0000
 +++ b/src/usr.bin/script/script.c	2021-06-17 00:38:33.578014997 +0000
 @@ -81,19 +81,21 @@
   static int	quiet, flush;
   static const char *fname;

 +static volatile sig_atomic_t doexit;
   static int	isterm;
   static struct	termios tt;

 -__dead static void	done(void);
 +static void	start(void);
 +static void	done(void);
   __dead static void	dooutput(void);
   __dead static void	doshell(const char *);
 -__dead static void	fail(void);
   static void	finish(int);
   static void	scriptflush(int);
   static void	record(FILE *, char *, size_t, int);
   static void	consume(FILE *, off_t, char *, int);
 -static void	childwait(void);
   __dead static void	playback(FILE *);
 +static sig_t	xsignal(int, sig_t);
 +static void	killchild(int);

   int
   main(int argc, char *argv[])
 @@ -179,18 +181,18 @@
   		(void)tcsetattr(STDIN_FILENO, TCSAFLUSH, &rtt);
   	}

 -	(void)signal(SIGCHLD, finish);
 +	(void)xsignal(SIGCHLD, finish);
 +	(void)xsignal(SIGHUP, killchild);
 +	(void)xsignal(SIGINT, killchild);
 +	(void)xsignal(SIGQUIT, killchild);
 +	(void)xsignal(SIGTERM, killchild);
   	child = fork();
 -	if (child == -1) {
 -		warn("fork");
 -		fail();
 -	}
 +	if (child == -1)
 +		err(EXIT_FAILURE, "fork");
   	if (child == 0) {
   		subchild = child = fork();
 -		if (child == -1) {
 -			warn("fork");
 -			fail();
 -		}
 +		if (child == -1)
 +			err(EXIT_FAILURE, "fork");
   		if (child)
   			dooutput();
   		else
 @@ -199,37 +201,50 @@

   	if (!rawout)
   		(void)fclose(fscript);
 -	while ((scc = read(STDIN_FILENO, ibuf, BUFSIZ)) > 0) {
 +	while (!doexit && (scc = read(STDIN_FILENO, ibuf, BUFSIZ)) > 0) {
   		cc = (size_t)scc;
   		if (rawout)
   			record(fscript, ibuf, cc, 'i');
   		(void)write(master, ibuf, cc);
   	}
 -	childwait();
 +	done();
   	return EXIT_SUCCESS;
   }

 -static void
 -childwait(void)
 +/**
 + * wrapper around sigaction() because we want POSIX semantics:
 + * no auto-restarting of interrupted slow syscalls.
 + */
 +static sig_t
 +xsignal(int signo, sig_t handler)
   {
 -	sigset_t set;
 +	struct sigaction sa, osa;

 -	sigemptyset(&set);
 -	sigsuspend(&set);
 +	sa.sa_handler = handler;
 +	sa.sa_flags = 0;
 +	sigemptyset(&sa.sa_mask);
 +	if (sigaction(signo, &sa, &osa) < 0)
 +		return SIG_ERR;
 +	return osa.sa_handler;
   }

   static void
 -finish(int signo)
 +killchild(int signo)
   {
 -	int die, pid, status;
 +	if (child > 1)
 +		(void)kill(child, signo);
 +}

 -	die = 0;
 -	while ((pid = wait3(&status, WNOHANG, 0)) > 0)
 -		if (pid == child)
 -			die = 1;
 +static void
 +finish(int signo)
 +{
 +	int pid, status;

 -	if (die)
 -		done();
 +	while ((pid = wait3(&status, WNOHANG, NULL)) > 0)
 +		if (pid == child) {
 +			doexit = 1;
 +			break;
 +		}
   }

   static void
 @@ -238,22 +253,15 @@
   	struct itimerval value;
   	ssize_t scc;
   	size_t cc;
 -	time_t tvec;
   	char obuf[BUFSIZ];

 -	(void)close(STDIN_FILENO);
 -	tvec = time(NULL);
 -	if (rawout)
 -		record(fscript, NULL, 0, 's');
 -	else if (!quiet)
 -		(void)fprintf(fscript, "Script started on %s", ctime(&tvec));
 -
 +	start();
   	(void)signal(SIGALRM, scriptflush);
   	value.it_interval.tv_sec = SECSPERMIN / 2;
   	value.it_interval.tv_usec = 0;
   	value.it_value = value.it_interval;
   	(void)setitimer(ITIMER_REAL, &value, NULL);
 -	for (;;) {
 +	while (!doexit) {
   		scc = read(master, obuf, sizeof(obuf));
   		if (scc <= 0)
   			break;
 @@ -267,7 +275,7 @@
   		if (flush)
   			(void)fflush(fscript);
   	}
 -	childwait();
 +	done();
   	exit(EXIT_SUCCESS);
   }

 @@ -293,30 +301,31 @@
   		if (shell == NULL)
   			shell = _PATH_BSHELL;
   		execl(shell, shell, "-i", NULL);
 -		warn("execl `%s'", shell);
 +		err(EXIT_FAILURE, "execl `%s'", shell);
   	} else {
   		if (system(command) == -1)
 -			warn("system `%s'", command);
 +			err(EXIT_FAILURE, "system `%s'", command);
   	}
 -
 -	fail();
   }

   static void
 -fail(void)
 +start(void)
   {
 +	time_t tvec;

 -	(void)kill(0, SIGTERM);
 -	done();
 +	(void)close(STDIN_FILENO);
 +	tvec = time(NULL);
 +	if (rawout)
 +		record(fscript, NULL, 0, 's');
 +	else if (!quiet)
 +		(void)fprintf(fscript, "Script started on %s", ctime(&tvec));
   }

   static void
   done(void)
   {
 -	time_t tvec;
 -
   	if (subchild) {
 -		tvec = time(NULL);
 +		time_t tvec = time(NULL);
   		if (rawout)
   			record(fscript, NULL, 0, 'e');
   		else if (!quiet)
 @@ -330,7 +339,6 @@
   		if (!quiet)
   			(void)printf("Script done, output file is %s\n", fname);
   	}
 -	exit(EXIT_SUCCESS);
   }

   static void
 --- END PATCH ---

 -RVP

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56254 CVS commit: src/usr.bin/script
Date: Fri, 11 Feb 2022 16:15:25 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Fri Feb 11 21:15:25 UTC 2022

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

 Log Message:
 PR/56254: RVP: Don't call non-async-signal-safe functions from signal handlers.
 Establish a non-restart signal handler to avoid blocking in long I/Os.


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