git: 8dad5ece4947 - main - dd(1): neutralize SIGINT while non-async-signal safe code is executing

The branch main has been updated by kib:

URL: https://cgit.FreeBSD.org/src/commit/?id=8dad5ece49479ba6cdcd5bb4c2799bbd61add3e6

commit 8dad5ece49479ba6cdcd5bb4c2799bbd61add3e6
Author:     Konstantin Belousov <kib@FreeBSD.org>
AuthorDate: 2023-05-26 10:27:02 +0000
Commit:     Konstantin Belousov <kib@FreeBSD.org>
CommitDate: 2023-06-02 22:06:27 +0000

    dd(1): neutralize SIGINT while non-async-signal safe code is executing
    
    making the SIGINT handler (the terminate() function) safe to execute at
    any interruption moment.  This fixes a race in
    5807f35c541c26bbd91a3ae12506cd8dd8f20688 where SIGINT delivered right
    after the check_terminate() but before a blocking syscall would not
    cause abort.
    
    Do it by setting the in_io flag around potentially blocking io syscalls.
    If handler sees the flag, it terminates the program.  Otherwise,
    termination is delegated to the before_io/after_io fences.
    
    Reviewed by:    Andrew Gierth <andrew@tao146.riddles.org.uk>
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week
    Differential revision:  https://reviews.freebsd.org/D40281
---
 bin/dd/dd.c       | 22 ++++++++++-----------
 bin/dd/extern.h   |  5 +++--
 bin/dd/misc.c     | 58 ++++++++++++++++++++++++++++++++++++++++++++-----------
 bin/dd/position.c | 11 ++++++-----
 4 files changed, 67 insertions(+), 29 deletions(-)

diff --git a/bin/dd/dd.c b/bin/dd/dd.c
index 78a9e8b06720..84d955b235f8 100644
--- a/bin/dd/dd.c
+++ b/bin/dd/dd.c
@@ -99,8 +99,7 @@ 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);
+	prepare_io();
 
 	(void)setlocale(LC_CTYPE, "");
 	jcl(argv);
@@ -158,9 +157,9 @@ setup(void)
 		iflags = 0;
 		if (ddflags & C_IDIRECT)
 			iflags |= O_DIRECT;
-		check_terminate();
+		before_io();
 		in.fd = open(in.name, O_RDONLY | iflags, 0);
-		check_terminate();
+		after_io();
 		if (in.fd == -1)
 			err(1, "%s", in.name);
 	}
@@ -197,17 +196,18 @@ setup(void)
 			oflags |= O_FSYNC;
 		if (ddflags & C_ODIRECT)
 			oflags |= O_DIRECT;
-		check_terminate();
+		before_io();
 		out.fd = open(out.name, O_RDWR | oflags, DEFFILEMODE);
-		check_terminate();
+		after_io();
 		/*
 		 * May not have read access, so try again with write only.
 		 * Without read we may have a problem if output also does
 		 * not support seeks.
 		 */
 		if (out.fd == -1) {
+			before_io();
 			out.fd = open(out.name, O_WRONLY | oflags, DEFFILEMODE);
-			check_terminate();
+			after_io();
 			out.flags |= NOREAD;
 			cap_rights_clear(&rights, CAP_READ);
 		}
@@ -424,9 +424,9 @@ dd_in(void)
 
 		in.dbrcnt = 0;
 fill:
-		check_terminate();
+		before_io();
 		n = read(in.fd, in.dbp + in.dbrcnt, in.dbsz - in.dbrcnt);
-		check_terminate();
+		after_io();
 
 		/* EOF */
 		if (n == 0 && in.dbrcnt == 0)
@@ -607,9 +607,9 @@ dd_out(int force)
 					pending = 0;
 				}
 				if (cnt) {
-					check_terminate();
+					before_io();
 					nw = write(out.fd, outp, cnt);
-					check_terminate();
+					after_io();
 					out.seek_offset = 0;
 				} else {
 					return;
diff --git a/bin/dd/extern.h b/bin/dd/extern.h
index e801722560f7..c9de42a152d5 100644
--- a/bin/dd/extern.h
+++ b/bin/dd/extern.h
@@ -49,8 +49,9 @@ void progress(void);
 void summary(void);
 void sigalarm_handler(int);
 void siginfo_handler(int);
-void terminate(int);
-void check_terminate(void);
+void prepare_io(void);
+void before_io(void);
+void after_io(void);
 void unblock(void);
 void unblock_close(void);
 
diff --git a/bin/dd/misc.c b/bin/dd/misc.c
index 5fbea20b7049..c814d926d884 100644
--- a/bin/dd/misc.c
+++ b/bin/dd/misc.c
@@ -48,6 +48,7 @@ __FBSDID("$FreeBSD$");
 #include <inttypes.h>
 #include <libutil.h>
 #include <signal.h>
+#include <stdatomic.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -147,23 +148,58 @@ sigalarm_handler(int signo __unused)
 	need_progress = 1;
 }
 
-void
+static void terminate(int signo) __dead2;
+static void
 terminate(int signo)
 {
-
 	kill_signal = signo;
+	summary();
+	(void)fflush(stderr);
+	raise(kill_signal);
+	/* NOT REACHED */
+	_exit(1);
+}
+
+static sig_atomic_t in_io = 0;
+static sig_atomic_t sigint_seen = 0;
+
+static void
+sigint_handler(int signo __unused)
+{
+	atomic_signal_fence(memory_order_acquire);
+	if (in_io)
+		terminate(SIGINT);
+	sigint_seen = 1;
+}
+
+void
+prepare_io(void)
+{
+	struct sigaction sa;
+	int error;
+
+	memset(&sa, 0, sizeof(sa));
+	sa.sa_flags = SA_NODEFER | SA_RESETHAND;
+	sa.sa_handler = sigint_handler;
+	error = sigaction(SIGINT, &sa, 0);
+	if (error != 0)
+		err(1, "sigaction");
 }
 
 void
-check_terminate(void)
+before_io(void)
 {
+	in_io = 1;
+	atomic_signal_fence(memory_order_seq_cst);
+	if (sigint_seen)
+		terminate(SIGINT);
+}
 
-	if (kill_signal) {
-		summary();
-		(void)fflush(stderr);
-		signal(kill_signal, SIG_DFL);
-		raise(kill_signal);
-		/* NOT REACHED */
-		_exit(128 + kill_signal);
-	}
+void
+after_io(void)
+{
+	in_io = 0;
+	atomic_signal_fence(memory_order_seq_cst);
+	if (sigint_seen)
+		terminate(SIGINT);
 }
diff --git a/bin/dd/position.c b/bin/dd/position.c
index a7dd733f0bae..6cb6643982dc 100644
--- a/bin/dd/position.c
+++ b/bin/dd/position.c
@@ -191,10 +191,11 @@ pos_out(void)
 
 	/* Read it. */
 	for (cnt = 0; cnt < out.offset; ++cnt) {
-		check_terminate();
-		if ((n = read(out.fd, out.db, out.dbsz)) > 0)
+		before_io();
+		n = read(out.fd, out.db, out.dbsz);
+		after_io();
+		if (n > 0)
 			continue;
-		check_terminate();
 		if (n == -1)
 			err(1, "%s", out.name);
 
@@ -209,9 +210,9 @@ pos_out(void)
 			err(1, "%s", out.name);
 
 		while (cnt++ < out.offset) {
-			check_terminate();
+			before_io();
 			n = write(out.fd, out.db, out.dbsz);
-			check_terminate();
+			after_io();
 			if (n == -1)
 				err(1, "%s", out.name);
 			if (n != out.dbsz)