Re: bin/56254: script(1) abuses non-async-signal-safe functions in signal handlers

[RVP <rvp%SDF.ORG@localhost>]

The following reply was made to PR bin/56254; it has been noted by GNATS.

From: RVP <rvp%SDF.ORG@localhost>
To: gnats-bugs%netbsd.org@localhost
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