dd: Fix SIGINT handling.

Currently, we handle SIGINT by calling summary() and _exit() directly from the signal handler, which we install after setup().  There are several issues with this:

* summary() is not signal safe;
* the parent is not informed about the signal;
* setup() can block on open(), and catching SIGINT at that stage will produce the correct exit status but will not print anything to stderr as POSIX demands.

Fix this by making SIGINT non-restartable, changing our signal handler to only set a flag, installing it before setup(), and checking the termination flag before and after every blocking operation, i.e. open(), read(), write().

Also add two test cases, one for catching SIGINT while opening the input and one for catching it while reading.  I couldn't think of an easy way to test catching SIGINT while writing (it's certainly feasible, but perhaps not from a shell script).

MFC after:	1 week
Sponsored by:	Klara, Inc.
Reviewed by:	cracauer, ngie, imp
Differential Revision:	https://reviews.freebsd.org/D39641

bin/dd/dd.c

@@ -64,6 +64,7 @@ __FBSDID("$FreeBSD$");
 #include <fcntl.h>
 #include <inttypes.h>
 #include <locale.h>
+#include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -91,12 +92,16 @@ char	fill_char;		/* Character to fill with if defined */
 size_t	speed = 0;		/* maximum speed, in bytes per second */
 volatile sig_atomic_t need_summary;
 volatile sig_atomic_t need_progress;
+volatile sig_atomic_t kill_signal;
 
 int
 main(int argc __unused, char *argv[])
 {
 	struct itimerval itv = { { 1, 0 }, { 1, 0 } }; /* SIGALARM every second, if needed */
 
+	(void)siginterrupt(SIGINT, 1);
+	(void)signal(SIGINT, terminate);
+
 	(void)setlocale(LC_CTYPE, "");
 	jcl(argv);
 	setup();
@@ -110,7 +115,6 @@ main(int argc __unused, char *argv[])
 		(void)signal(SIGALRM, sigalarm_handler);
 		setitimer(ITIMER_REAL, &itv, NULL);
 	}
-	(void)signal(SIGINT, terminate);
 
 	atexit(summary);
 
@@ -154,7 +158,9 @@ setup(void)
 		iflags = 0;
 		if (ddflags & C_IDIRECT)
 			iflags |= O_DIRECT;
+		check_terminate();
 		in.fd = open(in.name, O_RDONLY | iflags, 0);
+		check_terminate();
 		if (in.fd == -1)
 			err(1, "%s", in.name);
 	}
@@ -191,7 +197,9 @@ setup(void)
 			oflags |= O_FSYNC;
 		if (ddflags & C_ODIRECT)
 			oflags |= O_DIRECT;
+		check_terminate();
 		out.fd = open(out.name, O_RDWR | oflags, DEFFILEMODE);
+		check_terminate();
 		/*
 		 * May not have read access, so try again with write only.
 		 * Without read we may have a problem if output also does
@@ -199,6 +207,7 @@ setup(void)
 		 */
 		if (out.fd == -1) {
 			out.fd = open(out.name, O_WRONLY | oflags, DEFFILEMODE);
+			check_terminate();
 			out.flags |= NOREAD;
 			cap_rights_clear(&rights, CAP_READ);
 		}
@@ -415,7 +424,9 @@ dd_in(void)
 
 		in.dbrcnt = 0;
 fill:
+		check_terminate();
 		n = read(in.fd, in.dbp + in.dbrcnt, in.dbsz - in.dbrcnt);
+		check_terminate();
 
 		/* EOF */
 		if (n == 0 && in.dbrcnt == 0)
@@ -596,7 +607,9 @@ dd_out(int force)
 					pending = 0;
 				}
 				if (cnt) {
+					check_terminate();
 					nw = write(out.fd, outp, cnt);
+					check_terminate();
 					out.seek_offset = 0;
 				} else {
 					return;

bin/dd/extern.h

@@ -50,6 +50,7 @@ void summary(void);
 void sigalarm_handler(int);
 void siginfo_handler(int);
 void terminate(int);
+void check_terminate(void);
 void unblock(void);
 void unblock_close(void);
 
@@ -69,3 +70,4 @@ extern u_char casetab[];
 extern char fill_char;
 extern volatile sig_atomic_t need_summary;
 extern volatile sig_atomic_t need_progress;
+extern volatile sig_atomic_t kill_signal;

bin/dd/misc.c

@@ -147,11 +147,23 @@ sigalarm_handler(int signo __unused)
 	need_progress = 1;
 }
 
-/* ARGSUSED */
 void
-terminate(int sig)
+terminate(int signo)
 {
 
-	summary();
-	_exit(sig == 0 ? 0 : 1);
+	kill_signal = signo;
+}
+
+void
+check_terminate(void)
+{
+
+	if (kill_signal) {
+		summary();
+		(void)fflush(stderr);
+		signal(kill_signal, SIG_DFL);
+		raise(kill_signal);
+		/* NOT REACHED */
+		_exit(128 + kill_signal);
+	}
 }

bin/dd/position.c

@@ -191,9 +191,10 @@ pos_out(void)
 
 	/* Read it. */
 	for (cnt = 0; cnt < out.offset; ++cnt) {
+		check_terminate();
 		if ((n = read(out.fd, out.db, out.dbsz)) > 0)
 			continue;
-
+		check_terminate();
 		if (n == -1)
 			err(1, "%s", out.name);
 
@@ -208,7 +209,9 @@ pos_out(void)
 			err(1, "%s", out.name);
 
 		while (cnt++ < out.offset) {
+			check_terminate();
 			n = write(out.fd, out.db, out.dbsz);
+			check_terminate();
 			if (n == -1)
 				err(1, "%s", out.name);
 			if (n != out.dbsz)

bin/dd/tests/dd2_test.sh

@@ -1,5 +1,6 @@
 #
 # Copyright (c) 2017 Spectra Logic Corporation
+# Copyright (c) 2023 Klara, Inc.
 #
 # SPDX-License-Identifier: BSD-2-Clause
 #
@@ -46,8 +47,51 @@ seek_overflow_body()
 		dd if=f.in of=f.out bs=4096 seek=-1
 }
 
+atf_test_case sigint
+sigint_open_head()
+{
+	atf_set "descr" "SIGINT while opening destination"
+}
+sigint_open_body()
+{
+	atf_check mkfifo fifo
+	set -m
+	dd if=fifo of=/dev/null 2>stderr &
+	pid=$!
+	sleep 3
+	kill -INT $pid
+	wait $pid
+	rv=$?
+	atf_check test "$rv" -gt 128
+	atf_check -o inline:"INT\n" kill -l $((rv-128))
+	atf_check test -s stderr
+}
+
+atf_test_case sigint
+sigint_read_head()
+{
+	atf_set "descr" "SIGINT while reading source"
+}
+sigint_read_body()
+{
+	atf_check mkfifo fifo
+	(sleep 30 >fifo &) # ensures that dd does not block on open
+	set -m
+	dd if=fifo of=/dev/null 2>stderr &
+	pid=$!
+	sleep 3
+	kill -INT $pid
+	wait $pid
+	rv=$?
+	atf_check test "$rv" -gt 128
+	atf_check -o inline:"INT\n" kill -l $((rv-128))
+	atf_check test -s stderr
+}
+
 atf_init_test_cases()
 {
 	atf_add_test_case max_seek
 	atf_add_test_case seek_overflow
+	atf_add_test_case sigint_open
+	atf_add_test_case sigint_read
 }