Put the terminal back when a signal ends the mnemonic prompt (closes #91)

A SIGINT, SIGTERM or SIGHUP at the mnemonic prompt killed the tool
with echo still off. The prompt now catches them with signals.Notify
from before echo goes off, and reads the line in a goroutine. Once the
line is read or a signal arrives, it puts the terminal settings back
and stops catching; if a signal has arrived by then, it sends the tool
the same signal again, so the tool is still ended by it (130 for SIGINT
in a shell). A test sends SIGINT at the prompt on a pseudo-terminal.

The signals package moves from internal/cli to internal/signals, since
the mnemonic prompt now uses it as well.

Unchanged: the Windows prompt, which uses term.ReadPassword.

Model: opus-5-5

README.md

@@ -65,10 +65,12 @@ calls into `internal/`. The packages there are:
   and decrypts with it.
 - `internal/childmnemonic` derives a child mnemonic from the main one using
   BIP-85's own mnemonic application.
+- `internal/signals` catches SIGINT, SIGTERM and SIGHUP for the commands that
+  clean up before they end, and for the mnemonic prompt, which puts the terminal
+  back before the signal ends the tool.
 - `internal/cli` builds the cobra command tree and runs it. Under it,
-  `cli/options` holds the flags every command shares, `cli/signals` catches
-  SIGINT, SIGTERM and SIGHUP for the commands that clean up before they end, and
-  `cli/ssh`, `cli/age` and `cli/mnemonic` are the command groups.
+  `cli/options` holds the flags every command shares, and `cli/ssh`, `cli/age`
+  and `cli/mnemonic` are the command groups.
 - `internal/bip39` is a copy of `github.com/tyler-smith/go-bip39` v1.1.0,
   trimmed to what keyfunc uses.
 
@@ -328,10 +330,11 @@ passes through `ssh`'s own exit status.
 
 SIGINT, SIGTERM and SIGHUP end any command at once, at the mnemonic prompt too,
 with the status a shell gives a program killed by that signal (130 for SIGINT).
-While `age encrypt -o` or `age decrypt -o` is writing a new file or replacing a
-regular one, the signal makes it remove the unfinished file, leave a file
-already at the named path as it was, and exit with status 1. That holds for a
-signal that has reached `keyfunc` when its input ends; a later one leaves the
+At the prompt the terminal's settings are put back as they were first, so echo
+is on again. While `age encrypt -o` or `age decrypt -o` is writing a new file or
+replacing a regular one, the signal makes it remove the unfinished file, leave a
+file already at the named path as it was, and exit with status 1. That holds for
+a signal that has reached `keyfunc` when its input ends; a later one leaves the
 whole file in place. Ctrl-C on a pipeline ends the input at the same moment, and
 on Linux `keyfunc` sees the signal first, though no system promises that. A
 named pipe or a device at the `-o` path, or a path that is the same file as

internal/cli/age/age.go

@@ -16,8 +16,8 @@ import (
 	"github.com/spf13/cobra"
 	"sneak.berlin/go/keyfunc/internal/agekey"
 	"sneak.berlin/go/keyfunc/internal/cli/options"
-	"sneak.berlin/go/keyfunc/internal/cli/signals"
 	"sneak.berlin/go/keyfunc/internal/derive"
+	"sneak.berlin/go/keyfunc/internal/signals"
 )
 
 // ErrInterrupted is returned when SIGINT, SIGTERM or SIGHUP has been

internal/cli/cli.go

@@ -87,6 +87,9 @@ func init() {
 // still runs, and "age encrypt -o" and "age decrypt -o" while they
 // write a new file to rename over the named one, so the unfinished file
 // is removed.
+//
+// At the mnemonic prompt the signal is caught only to put the terminal
+// back, and then ends the tool as above.
 func Main() int {
 	err := Root().Execute()
 	if err == nil {

internal/cli/prompt_test.go

@@ -6,26 +6,25 @@ import (
 	"os"
 	"os/exec"
 	"strings"
+	"syscall"
 	"testing"
 	"time"
 
 	"github.com/creack/pty"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/term"
 	"sneak.berlin/go/keyfunc/internal/mnemonic"
 )
 
-// TestEndOfInputAtThePromptEndsTheTool runs "age pub" with no mnemonic
-// given and a terminal on all three streams, waits for the prompt, and
-// types Ctrl-D. The tool has to end with status 1, say that the
+// TestEndOfInputAtThePromptEndsTheTool starts "age pub" and, at the
+// prompt, types Ctrl-D. The tool has to end with status 1, say that the
 // mnemonic is empty, and leave the terminal set as it found it, so with
 // echo on again.
 //
 // pty.Open returns the two ends of a new terminal: tty is the end the
 // tool uses as its terminal, and ptmx the end the test reads what the
-// terminal shows from and types into. Reading ptmx stops at the
-// deadline, so a prompt that never comes fails the test instead of
-// hanging it.
+// terminal shows from and types into.
 func TestEndOfInputAtThePromptEndsTheTool(t *testing.T) {
 	t.Setenv(mnemonic.Variable, "")
 	t.Setenv(mnemonic.CommandVariable, "")
@@ -36,33 +35,10 @@ func TestEndOfInputAtThePromptEndsTheTool(t *testing.T) {
 	defer func() { _ = ptmx.Close() }()
 	defer func() { _ = tty.Close() }()
 
-	require.NoError(t, ptmx.SetReadDeadline(time.Now().Add(10*time.Second)))
-
 	before, err := term.GetState(int(tty.Fd()))
 	require.NoError(t, err)
 
-	//nolint:gosec // this test's own binary as the tool
-	command := exec.CommandContext(t.Context(), os.Args[0], "age", "pub")
-
-	command.Env = append(os.Environ(), runAsTool+"=1")
-	command.Stdin = tty
-	command.Stdout = tty
-	command.Stderr = tty
-
-	require.NoError(t, command.Start())
-
-	var (
-		shown string
-		char  byte
-	)
-
-	terminal := bufio.NewReader(ptmx)
-	for !strings.HasSuffix(shown, "mnemonic: ") {
-		char, err = terminal.ReadByte()
-		require.NoError(t, err, "the tool did not prompt: %q", shown)
-
-		shown += string(char)
-	}
+	command, terminal := prompted(t, ptmx, tty)
 
 	_, err = ptmx.WriteString("\x04") // Ctrl-D
 	require.NoError(t, err)
@@ -81,3 +57,77 @@ func TestEndOfInputAtThePromptEndsTheTool(t *testing.T) {
 	said, _ := io.ReadAll(terminal)
 	require.Contains(t, string(said), mnemonic.ErrEmpty.Error())
 }
+
+// TestASignalAtThePromptPutsTheTerminalBack starts "age pub" and, at
+// the prompt, once the tool has turned echo off, sends it SIGINT. The
+// tool has to be ended by SIGINT, which a shell reports as status 130,
+// and leave the terminal set as it found it, so with echo on again.
+func TestASignalAtThePromptPutsTheTerminalBack(t *testing.T) {
+	t.Setenv(mnemonic.Variable, "")
+	t.Setenv(mnemonic.CommandVariable, "")
+
+	ptmx, tty, err := pty.Open()
+	require.NoError(t, err)
+
+	defer func() { _ = ptmx.Close() }()
+	defer func() { _ = tty.Close() }()
+
+	before, err := term.GetState(int(tty.Fd()))
+	require.NoError(t, err)
+
+	command, _ := prompted(t, ptmx, tty)
+
+	// The prompt is shown just before echo is turned off, and the
+	// signal has to come while it is off.
+	require.Eventually(t, func() bool {
+		now, err := term.GetState(int(tty.Fd()))
+
+		return err == nil && !assert.ObjectsAreEqual(before, now)
+	}, 5*time.Second, 5*time.Millisecond, "the tool did not turn echo off")
+
+	require.NoError(t, command.Process.Signal(syscall.SIGINT))
+	waitForTool(t, "SIGINT at the prompt", command)
+
+	status, ok := command.ProcessState.Sys().(syscall.WaitStatus)
+	require.True(t, ok)
+	require.True(t, status.Signaled(), "the tool was not ended by the signal")
+	require.Equal(t, syscall.SIGINT, status.Signal())
+
+	after, err := term.GetState(int(tty.Fd()))
+	require.NoError(t, err)
+	require.Equal(t, before, after, "the terminal settings were not restored")
+}
+
+// prompted starts "age pub" as a subprocess, for a test that has set no
+// mnemonic, with the terminal's tty end on all three streams, and
+// returns once the tool has shown the prompt. The reader returned reads
+// what the terminal shows from its ptmx end. Reading ptmx stops at a
+// deadline, so a prompt that never comes fails the test instead of
+// hanging it.
+func prompted(t *testing.T, ptmx, tty *os.File) (*exec.Cmd, *bufio.Reader) {
+	t.Helper()
+
+	require.NoError(t, ptmx.SetReadDeadline(time.Now().Add(10*time.Second)))
+
+	//nolint:gosec // this test's own binary as the tool
+	command := exec.CommandContext(t.Context(), os.Args[0], "age", "pub")
+
+	command.Env = append(os.Environ(), runAsTool+"=1")
+	command.Stdin = tty
+	command.Stdout = tty
+	command.Stderr = tty
+
+	require.NoError(t, command.Start())
+
+	var shown string
+
+	terminal := bufio.NewReader(ptmx)
+	for !strings.HasSuffix(shown, "mnemonic: ") {
+		char, err := terminal.ReadByte()
+		require.NoError(t, err, "the tool did not prompt: %q", shown)
+
+		shown += string(char)
+	}
+
+	return command, terminal
+}

internal/cli/ssh/install.go

@@ -15,8 +15,8 @@ import (
 	"time"
 
 	"github.com/spf13/cobra"
-	"sneak.berlin/go/keyfunc/internal/cli/signals"
 	"sneak.berlin/go/keyfunc/internal/mnemonic"
+	"sneak.berlin/go/keyfunc/internal/signals"
 )
 
 // Where the key goes on the host and what the file it arrives in is

internal/cli/ssh/to.go

@@ -10,8 +10,8 @@ import (
 	"syscall"
 
 	"github.com/spf13/cobra"
-	"sneak.berlin/go/keyfunc/internal/cli/signals"
 	"sneak.berlin/go/keyfunc/internal/mnemonic"
+	"sneak.berlin/go/keyfunc/internal/signals"
 )
 
 // StatusError says the tool should end with the status ssh ended with.

internal/mnemonic/prompt_unix.go

@@ -8,8 +8,10 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"os/signal"
 
 	"golang.org/x/sys/unix"
+	"sneak.berlin/go/keyfunc/internal/signals"
 )
 
 // readWithoutEcho reads one line from the terminal on standard input
@@ -17,6 +19,11 @@ import (
 // input (Ctrl-D). The terminal is set as term.ReadPassword sets it;
 // ReadPassword itself is not used here because it never sees the end
 // of input and goes on waiting.
+//
+// Meanwhile SIGINT, SIGTERM and SIGHUP are caught, as signals.Notify
+// catches them. One that has arrived by the time the line is read puts
+// the terminal back as it was and then ends the tool, as it would have
+// ended it if it were not caught.
 func readWithoutEcho() (string, error) {
 	fd := int(os.Stdin.Fd())
 
@@ -25,6 +32,13 @@ func readWithoutEcho() (string, error) {
 		return "", fmt.Errorf("reading the terminal settings: %w", err)
 	}
 
+	// Caught from before echo is turned off, so that no signal can end
+	// the tool with echo off.
+	received := make(chan os.Signal, 1)
+	signals.Notify(received)
+
+	defer signal.Stop(received)
+
 	hidden := *settings
 	hidden.Lflag &^= unix.ECHO
 	hidden.Lflag |= unix.ICANON | unix.ISIG
@@ -35,14 +49,63 @@ func readWithoutEcho() (string, error) {
 		return "", fmt.Errorf("turning echo off: %w", err)
 	}
 
-	defer func() { _ = unix.IoctlSetTermios(fd, ioctlWriteTermios, settings) }()
+	var (
+		typed  string
+		failed error
+	)
 
-	// With ICANON set, a read from the terminal returns at most one
-	// line, so the buffer takes nothing past the newline.
-	typed, err := bufio.NewReader(os.Stdin).ReadString('\n')
-	if errors.Is(err, io.EOF) {
+	read := make(chan struct{})
+
+	go func() {
+		// With ICANON set, a read from the terminal returns at most one
+		// line, so the buffer takes nothing past the newline.
+		typed, failed = bufio.NewReader(os.Stdin).ReadString('\n')
+
+		close(read)
+	}()
+
+	var ending os.Signal
+
+	select {
+	case <-read:
+	case ending = <-received:
+	}
+
+	// The terminal is put back while the signals are still caught, so
+	// that none can end the tool with echo off. Stop returns only once
+	// every signal the tool has received has been handed over, so one
+	// that came as the line was read is in received by then.
+	_ = unix.IoctlSetTermios(fd, ioctlWriteTermios, settings)
+
+	signal.Stop(received)
+
+	if len(received) > 0 {
+		ending = <-received
+	}
+
+	if ending != nil {
+		return "", endBy(ending)
+	}
+
+	if errors.Is(failed, io.EOF) {
 		return typed, nil
 	}
 
-	return typed, err
+	return typed, failed
+}
+
+// endBy sends the tool the signal again, now that nothing catches it,
+// so that it ends the tool, and waits for that. It returns only if the
+// signal could not be sent.
+func endBy(ending os.Signal) error {
+	tool, err := os.FindProcess(os.Getpid())
+	if err == nil {
+		err = tool.Signal(ending)
+	}
+
+	if err != nil {
+		return fmt.Errorf("ending the tool by %s: %w", ending, err)
+	}
+
+	select {}
 }

internal/signals/signals.go

@@ -1,5 +1,9 @@
-// Package signals catches the signals that end the tool, for the
-// commands that clean up before they end.
+// Package signals catches SIGINT, SIGTERM and SIGHUP, the signals that
+// end the tool, for the commands that clean up before they end and for
+// the mnemonic prompt, which puts the terminal back before the signal
+// ends the tool. Go keeps an inherited ignore only for SIGHUP and
+// SIGINT, so a SIGTERM the tool was started with set to be ignored is
+// not left ignored.
 package signals
 
 import (
@@ -11,8 +15,8 @@ import (
 
 // Context is signal.NotifyContext for SIGINT, SIGTERM and SIGHUP: the
 // context it returns is cancelled when one of them arrives, and stop
-// stops catching them. It leaves out any of the three the tool was
-// started with set to be ignored, as nohup does with SIGHUP, because
+// stops catching them. It leaves out SIGHUP or SIGINT if the tool was
+// started with it set to be ignored, as nohup does with SIGHUP, because
 // catching a signal turns an ignored one back on and would end a run
 // that was meant to survive it.
 func Context(parent context.Context) (context.Context, context.CancelFunc) {
@@ -37,8 +41,8 @@ func Notify(c chan<- os.Signal) {
 	}
 }
 
-// caught returns those of SIGINT, SIGTERM and SIGHUP that the tool was
-// not started with set to be ignored.
+// caught returns SIGINT, SIGTERM and SIGHUP, less any that
+// signal.Ignored reports as ignored.
 func caught() []os.Signal {
 	endings := []os.Signal{syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP}
 	kept := make([]os.Signal, 0, len(endings))