Date: Fri, 5 May 2023 12:45:23 GMT From: =?utf-8?Q?Dag-Erling=20Sm=C3=B8rgrav?= <des@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 5807f35c541c - main - dd: Fix SIGINT handling. Message-ID: <202305051245.345CjNLE048275@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by des: URL: https://cgit.FreeBSD.org/src/commit/?id=5807f35c541c26bbd91a3ae12506cd8dd8f20688 commit 5807f35c541c26bbd91a3ae12506cd8dd8f20688 Author: Dag-Erling Smørgrav <des@FreeBSD.org> AuthorDate: 2023-05-05 11:17:44 +0000 Commit: Dag-Erling Smørgrav <des@FreeBSD.org> CommitDate: 2023-05-05 12:42:32 +0000 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 | 15 ++++++++++++++- bin/dd/extern.h | 2 ++ bin/dd/misc.c | 20 ++++++++++++++++---- bin/dd/position.c | 5 ++++- bin/dd/tests/dd2_test.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 80 insertions(+), 6 deletions(-) diff --git a/bin/dd/dd.c b/bin/dd/dd.c index c43645fa0073..78a9e8b06720 100644 --- a/bin/dd/dd.c +++ b/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; diff --git a/bin/dd/extern.h b/bin/dd/extern.h index 07c08e2ef8f2..e801722560f7 100644 --- a/bin/dd/extern.h +++ b/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; diff --git a/bin/dd/misc.c b/bin/dd/misc.c index 405448eb1cb0..5fbea20b7049 100644 --- a/bin/dd/misc.c +++ b/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); + } } diff --git a/bin/dd/position.c b/bin/dd/position.c index 21a100cd3b24..a7dd733f0bae 100644 --- a/bin/dd/position.c +++ b/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) diff --git a/bin/dd/tests/dd2_test.sh b/bin/dd/tests/dd2_test.sh index 7632de3bde22..c4a3065763cd 100755 --- a/bin/dd/tests/dd2_test.sh +++ b/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 }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202305051245.345CjNLE048275>