From 48894c82f68b8d12203c0fd35c3dfab827f1417c Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 6 Oct 2026 19:10:19 +0000 Subject: [PATCH] Stop the terminal tests reading a pty nobody opened (closes #126) On Linux, pty.Open of github.com/creack/pty v1.1.24 passes the address of a local variable to ioctl as a plain number, through a function call. When Go moved the goroutine's stack in between, the kernel wrote the pty's number to the old place, and pty.Open opened /dev/pts/0, another terminal. secret rm wrote there, and the test read a terminal no program had open, which never ends. Require the commit on the library's main branch that passes a pointer; no release has it yet. Both terminal tests now stop reading when their one-minute context ends, and fail saying what was still waiting. Model: opus-5-5 --- TODO.md | 12 ++++++++++++ go.mod | 2 +- go.sum | 4 ++-- internal/cli/integration_test.go | 26 ++++++++++++++++++++++---- 4 files changed, 37 insertions(+), 7 deletions(-) diff --git a/TODO.md b/TODO.md index 37ea5a0..287ff4d 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,18 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-06: `TestRemoveIgnoresTerminalOnStdout` and + `TestRemoveAsksAtTerminalOnStdin` no longer wait until Go's test timeout + (https://git.eeqj.de/sneak/secret/issues/126). On Linux, `pty.Open` of + `github.com/creack/pty` v1.1.24 passed the address of a local variable to the + `ioctl` system call as a plain number, through a function call; when Go moved + the goroutine's stack in between, the kernel wrote the terminal's number to + the old place, and `pty.Open` opened `/dev/pts/0` instead of the terminal it + had created. `secret rm` then wrote to that other terminal, and the test read + a terminal no program had open, which never ends. `go.mod` now requires the + commit on that library's main branch that passes a pointer instead; no release + has it yet. Both tests stop reading the terminal when their one-minute context + ends and fail saying so. - 2026-10-06: The tests run quickly with the race detector on (https://git.eeqj.de/sneak/secret/issues/120). Most of their time went to deriving keys from passphrases with scrypt, which is slow on purpose. The new diff --git a/go.mod b/go.mod index 249b9de..b20033a 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,7 @@ require ( github.com/btcsuite/btcd/btcec/v2 v2.1.3 github.com/btcsuite/btcd/btcutil v1.1.6 github.com/btcsuite/btcutil v0.0.0-20190425235716-9e5f4b9a998d - github.com/creack/pty v1.1.24 + github.com/creack/pty v1.1.25-0.20260601142114-9246436fffe8 // v1.1.24's Open can return another pty's terminal github.com/dustin/go-humanize v1.0.1 github.com/fatih/color v1.18.0 github.com/keybase/go-keychain v0.0.0-20230307172405-3e4884637dd1 diff --git a/go.sum b/go.sum index 4f68997..251c09f 100644 --- a/go.sum +++ b/go.sum @@ -35,8 +35,8 @@ github.com/btcsuite/snappy-go v1.0.0/go.mod h1:8woku9dyThutzjeg+3xrA5iCpBRH8XEEg github.com/btcsuite/websocket v0.0.0-20150119174127-31079b680792/go.mod h1:ghJtEyQwv5/p4Mg4C0fgbePVuGr935/5ddU9Z3TmDRY= github.com/btcsuite/winsvc v1.0.0/go.mod h1:jsenWakMcC0zFBFurPLEAyrnc/teJEM1O46fmI40EZs= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= -github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= -github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= +github.com/creack/pty v1.1.25-0.20260601142114-9246436fffe8 h1:CY3gjC7naqYGLMiywvj3suPfa1i0p/QEr7o8ujxL/2M= +github.com/creack/pty v1.1.25-0.20260601142114-9246436fffe8/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/davecgh/go-spew v0.0.0-20171005155431-ecdeabc65495/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= diff --git a/internal/cli/integration_test.go b/internal/cli/integration_test.go index 9e66629..df140a8 100644 --- a/internal/cli/integration_test.go +++ b/internal/cli/integration_test.go @@ -2601,7 +2601,12 @@ func TestRemoveWithoutTerminalFailsAtOnce(t *testing.T) { // and stderr, not both: whether it asks must depend on stdin alone, where // the answer is read from. pty.Open returns the two ends of a new terminal: // tty is the end a program uses as its terminal, and ptmx the end the test -// reads what the terminal shows from and types into. +// reads what the terminal shows from and types into. Reading ptmx stops at +// the context's deadline, when secret rm is killed too, so a terminal that +// stays open fails the test then instead of hanging it. The deadline works +// only while ptmx stays non-blocking, as pty.Open of the github.com/creack/pty +// commit in go.mod leaves it: calling ptmx.Fd() or going back to v1.1.24 +// makes the read ignore the deadline, without any error. // TestRemoveIgnoresTerminalOnStdout runs `echo y | secret rm x` at a // terminal. stdin is a pipe, so nobody can answer there, and the command @@ -2619,6 +2624,9 @@ func TestRemoveIgnoresTerminalOnStdout(t *testing.T) { defer func() { _ = ptmx.Close() }() + deadline, _ := ctx.Deadline() + require.NoError(t, ptmx.SetReadDeadline(deadline)) + cmd.Stdin = strings.NewReader("y\n") cmd.Stdout = tty cmd.Stderr = tty @@ -2628,9 +2636,15 @@ func TestRemoveIgnoresTerminalOnStdout(t *testing.T) { _ = tty.Close() // The read ends once secret rm has exited and so closed the terminal. - shown, _ := io.ReadAll(ptmx) + shown, err := io.ReadAll(ptmx) + require.NotErrorIs(t, err, os.ErrDeadlineExceeded, + "the terminal was still open a minute after secret rm started: %s", + shown) - require.Error(t, cmd.Wait()) + err = cmd.Wait() + + require.NoError(t, ctx.Err(), "secret rm did not exit within a minute") + require.Error(t, err) assert.Contains(t, string(shown), "pass --force") assert.DirExists(t, secretDir) } @@ -2650,6 +2664,9 @@ func TestRemoveAsksAtTerminalOnStdin(t *testing.T) { defer func() { _ = ptmx.Close() }() + deadline, _ := ctx.Deadline() + require.NoError(t, ptmx.SetReadDeadline(deadline)) + cmd.Stdin = tty // Not a file, so exec.Cmd connects stdout through a pipe. cmd.Stdout = io.Discard @@ -2667,7 +2684,8 @@ func TestRemoveAsksAtTerminalOnStdin(t *testing.T) { terminal := bufio.NewReader(ptmx) for !bytes.HasSuffix(shown, []byte("[y/N] ")) { char, err = terminal.ReadByte() - require.NoError(t, err, "secret rm ended without asking: %s", shown) + require.NoError(t, err, "secret rm did not ask on the terminal: %s", + shown) shown = append(shown, char) }