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.
(Contact us)
$NetBSD: query-full-pr,v 1.46 2020/01/03 16:35:01 leot Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2020
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.