[PATCH] Don't trap INT or TERM; they are redundant and can break `pass edit`.

[Nick]

Hello pass(1)!

I'm using vis(1) (https://github.com/martanne/vis) and whenever I try to edit my passwords I get
stumped:

[kousu at requiem ~]$ EDITOR=vis pass edit lol
New password not saved.

Linux vi(1) too:

[kousu at requiem ~]$ EDITOR=vi pass edit lol
New password not saved.

But these editors all work:
[kousu at requiem ~]$ EDITOR=vim pass edit lol
[master 4c687b5] Edit password for lol using vim.
1 file changed, 0 insertions(+), 0 deletions(-)
rewrite lol.gpg (100%)
[kousu at requiem ~]$ EDITOR=nano pass edit lol
[master b8dc02b] Edit password for lol using nano.
1 file changed, 0 insertions(+), 0 deletions(-)
rewrite lol.gpg (100%)
[kousu at requiem ~]$ EDITOR=mg pass edit lol
[master 28e9d7c] Edit password for lol using mg.
1 file changed, 0 insertions(+), 0 deletions(-)
rewrite lol.gpg (100%)
[kousu at requiem ~]$ 

I've installed pass(1) from ArchLinux repos:

[kousu at requiem pass]$ pacman -Qi pass
Name : pass
Version : 1.7.1-1
Description : Stores, retrieves, generates, and synchronizes passwords securely
Architecture : any
URL : https://www.passwordstore.org
Licenses : GPL2
Groups : None
Provides : passmenu
Depends On : xclip bash gnupg tree
Optional Deps : git: for Git support [installed]
dmenu: for passmenu [installed]
qrencode: for QR code support
Required By : None
Optional For : None
Conflicts With : passmenu
Replaces : passmenu
Installed Size : 45.00 KiB
Packager : Lukas Fleischer <lfleischer at archlinux.org>
Build Date : Fri 14 Apr 2017 04:31:38 AM EDT
Install Date : Mon 22 Jan 2018 04:23:59 PM EST
Install Reason : Explicitly installed
Install Script : No
Validated By : Signature

(ignore the install date; I tried reinstalling to see if the bug was just me, but I've actually had
it installed since August or earlier).

`pass edit` is:

cmd_edit() {
...
tmpdir #Defines $SECURE_TMPDIR
local tmp_file="$(mktemp -u "$SECURE_TMPDIR/XXXXXX")-${path//\//-}.txt"
...
${EDITOR:-vi} "$tmp_file"
# < -- the file is deleted here -->
[[ -f $tmp_file ]] || die "New password not saved."

What's going on is that `tmpdir` says

tmpdir() {
...
local template="$PROGRAM.XXXXXXXXXXXXX"
if [[ -d /dev/shm && -w /dev/shm && -x /dev/shm ]]; then
SECURE_TMPDIR="$(mktemp -d "/dev/shm/$template")"
remove_tmpfile() {
rm -rf "$SECURE_TMPDIR"
}
trap remove_tmpfile INT TERM EXIT
...

And that trap is running *when vi exits*. I don't know why vis(1) and vi(1) trigger it but the
others don't, and why all the other intermediate commands don't also trigger it, but they do.

Here's a refined test case script which reproduces the behaviour:

```
#!/bin/sh
# test_traps.sh
# debugging why pass(1) can't edit any passwords; it always says "Password not saved"
# I suspect it's because 'trap remove_tmpfile' is running when the editor quits, even though it's
meant to run when pass(1) quits

tmpdir() {
SECURE_TMPDIR="$(mktemp -d "/dev/shm/test_traps.XXXXX")"
remove_tmpfile() {
echo "remove_tmpfile"
rm -rf "$SECURE_TMPDIR"
}
trap remove_tmpfile INT TERM EXIT # this runs twice: once immediately after vi gets done, and once
at the end of the script
#trap remove_tmpfile EXIT # behaves sensible; even cleans up on INT when INT triggers EXIT
#trap remove_tmpfile TERM # this *never* runs
#trap remove_tmpfile INT # this runs *after* vi, and only after vi
}

tmpdir
tmpfile=$SECURE_TMPDIR/test
date > "$tmpfile"

echo $tmpfile
cat $tmpfile

sleep 3
echo "launching editor"
${EDITOR:-vi} $SECURE_TMPDIR/test
echo "done editor"

# this should *succeed*: the file should be readable here
echo $tmpfile
cat $tmpfile

# but after this script, the file should be gone, so test with test_traps.sh ; ls -l
/dev/shm/test_traps.*
```

Here is how this behaves for me. It seems, actually, that this may be a bashism, since under sh(1)
`remove_tmpfile` only runs once, but under bash(1) it runs twice:

[kousu at requiem pass]$ bash test_traps.sh 
/dev/shm/test_traps.ge5j6/test
Mon Jan 22 16:42:16 EST 2018
vi
remove_tmpfile
done vi
/dev/shm/test_traps.ge5j6/test
cat: /dev/shm/test_traps.ge5j6/test: No such file or directory
remove_tmpfile

[kousu at requiem pass]$ sh test_traps.sh 
/dev/shm/test_traps.Noqry/test
Mon Jan 22 16:41:54 EST 2018
vi
done vi
/dev/shm/test_traps.Noqry/test
Mon Jan 22 16:41:54 EST 2018
remove_tmpfile

I think a sufficient patch is just to *not* trap INT or TERM; EXIT should be enough. In my testing
it seems to be enough. Another patch might be to make sure to run bash in posixly-correct mode.

Here is my patch below (and attached too). I'm not sure if this is the correct way to submit a
patch; I'm too used to github :$. If you want it in a different format, let me know.

[kousu at requiem password-store]$ cat 0001-Don-t-trap-INT-or-TERM-they-are-redundant-and-can-br.patch

>From cefb03e4fb75abdd878d96e1982e9723e8e7f280 Mon Sep 17 00:00:00 2001
From: kousu <nick at kousu.ca>
Date: Mon, 22 Jan 2018 16:52:06 -0500
Subject: [PATCH] Don't trap INT or TERM; they are redundant and can break
`pass edit`.

Some EDITORs, notably Linux vi(1), which is the fallback default in pass,
apparently send INT when they exit, and when pass is run under bash
(which is also its default)--if you have /dev/shm/ available--bash catches
this and cleans up your edited password file *before* it can be reencrypted
and saved.

This only happens with `pass edit`; none of the other commands combine
tmpdir and $EDITOR.
---
src/password-store.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/password-store.sh b/src/password-store.sh
index e3cd145..99d2d65 100755
--- a/src/password-store.sh
+++ b/src/password-store.sh
@@ -203,7 +203,7 @@ tmpdir() {
remove_tmpfile() {
rm -rf "$SECURE_TMPDIR"
}
- trap remove_tmpfile INT TERM EXIT
+ trap remove_tmpfile EXIT
else
[[ $warn -eq 1 ]] && yesno "$(cat <<-_EOF
Your system does not have /dev/shm, which means that it may
@@ -218,7 +218,7 @@ tmpdir() {
find "$SECURE_TMPDIR" -type f -exec $SHRED {} +
rm -rf "$SECURE_TMPDIR"
}
- trap shred_tmpfile INT TERM EXIT
+ trap shred_tmpfile EXIT
fi

}
-- 
2.16.0