[pacman-dev] [PATCH 1/3] repo-add: Avoid race condition in signal handlers
There is a small chance that a user sends SIGINT (or any other signal that is trapped) when we're already in clean_up() which used to lead to trap_exit() being executed and the remaining code in clean_up() being skipped due to the bash signal/trap handler blocking EXIT (since its handler is already being executed, even if it's interrupted). In practice, this behaviour caused unexpected results (primarily because pressing ^C at the wrong time left a lock file behind): $ ./repo-add extra.db.tar.gz foobar ==> Extracting database to a temporary location... ^C ==> ERROR: Aborted by user! Exiting... $ ./repo-add extra.db.tar.gz foobar ==> Extracting database to a temporary location... ==> ERROR: File 'foobar' not found. ==> No packages modified, nothing to do. ^C ==> ERROR: Aborted by user! Exiting... $ ./repo-add extra.db.tar.gz foobar ==> ERROR: Failed to acquire lockfile: extra.db.tar.gz.lck. ==> ERROR: Held by process 18522 Fix this and reduce the chance of race conditions in signal handlers by: * Unhooking all traps in both clean_up() and trap_exit(). * Call clean_up() explicitly in trap_exit() to make sure we remove the lock file and the temporary directory even if we send SIGINT when clean_up() is already being executed but didn't reach the unhook code yet. Also, add an optional parameter to clean_up() to allow for setting an explicit exit code when we call clean_up() from trap_exit(). Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- scripts/repo-add.sh.in | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-) diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 29b150c..d147b87 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -520,13 +520,19 @@ remove() { } trap_exit() { + # unhook all traps to avoid race conditions + trap '' EXIT TERM HUP QUIT INT ERR + echo error "$@" - exit 1 + clean_up 1 } clean_up() { - local exit_code=$? + local exit_code=${1:-$?} + + # unhook all traps to avoid race conditions + trap '' EXIT TERM HUP QUIT INT ERR [[ -d $tmpdir ]] && rm -rf "$tmpdir" (( CLEAN_LOCK )) && [[ -f $LOCKFILE ]] && rm -f "$LOCKFILE" -- 1.7.7
This includes some fixes to the messages that are displayed when a signal is caught in makepkg or repo-add: * Instead of always showing "==> ERROR: TERM signal caught. Exiting...", replace "TERM" by whatever signal is actually caught. * Fix a typo in the SIGERR error message in repo-add ("occurred" instead of "occured"). Francois already fixed this for makepkg in 1e51b81c. Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- scripts/makepkg.sh.in | 4 +++- scripts/repo-add.sh.in | 6 ++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 50cf272..09c1e96 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -179,7 +179,9 @@ clean_up() { ## set -E trap 'clean_up' 0 -trap 'trap_exit "$(gettext "TERM signal caught. Exiting...")"' TERM HUP QUIT +for signal in TERM HUP QUIT; do + trap "trap_exit \"$(gettext "%s signal caught. Exiting...")\" \"$signal\"" "$signal" +done trap 'trap_exit "$(gettext "Aborted by user! Exiting...")"' INT trap 'trap_exit "$(gettext "An unknown error has occurred. Exiting...")"' ERR diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index d147b87..5e1d770 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -572,9 +572,11 @@ tmpdir=$(mktemp -d /tmp/repo-tools.XXXXXXXXXX) || (\ mkdir $tmpdir/tree trap 'clean_up' EXIT -trap 'trap_exit "$(gettext "TERM signal caught. Exiting...")"' TERM HUP QUIT +for signal in TERM HUP QUIT; do + trap "trap_exit \"$(gettext "%s signal caught. Exiting...")\" \"$signal\"" "$signal" +done trap 'trap_exit "$(gettext "Aborted by user! Exiting...")"' INT -trap 'trap_exit "$(gettext "An unknown error has occured. Exiting...")"' ERR +trap 'trap_exit "$(gettext "An unknown error has occurred. Exiting...")"' ERR declare -a args success=0 -- 1.7.7
Replace "/tmp" with "${TMPDIR:-/tmp}" to allow for overriding the hardcoded path. Since we only use "/tmp" in conjunction with mktemp(1), we could also have used "--tmpdir", which is GNU-ish, however (and the BSD counterpart "-t" has been deprecated in GNU mktemp). Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- scripts/pacman-optimize.sh.in | 2 +- scripts/repo-add.sh.in | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/pacman-optimize.sh.in b/scripts/pacman-optimize.sh.in index 5ff302e..154f982 100644 --- a/scripts/pacman-optimize.sh.in +++ b/scripts/pacman-optimize.sh.in @@ -113,7 +113,7 @@ fi # do not let pacman run while we do this touch "$lockfile" -workdir=$(mktemp -d /tmp/pacman-optimize.XXXXXXXXXX) || +workdir=$(mktemp -d "${TMPDIR:-/tmp}/pacman-optimize.XXXXXXXXXX") || die_r "$(gettext "Can not create temp directory for database building.")\n" >&2 # step 1: sum the old db diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index 5e1d770..fc008fc 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -566,7 +566,7 @@ if [[ $cmd != "repo-add" && $cmd != "repo-remove" ]]; then exit 1 fi -tmpdir=$(mktemp -d /tmp/repo-tools.XXXXXXXXXX) || (\ +tmpdir=$(mktemp -d "${TMPDIR:-/tmp}/repo-tools.XXXXXXXXXX") || (\ error "$(gettext "Cannot create temp directory for database building.")"; \ exit 1) mkdir $tmpdir/tree -- 1.7.7
participants (1)
-
Lukas Fleischer