Date: Fri, 2 Jun 2023 22:51:48 GMT From: Konstantin Belousov <kib@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 8dad5ece4947 - main - dd(1): neutralize SIGINT while non-async-signal safe code is executing Message-ID: <202306022251.352Mpmug017518@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
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)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202306022251.352Mpmug017518>