diff --git a/input.go b/input.go index 4b5d96a..5ab8e03 100644 --- a/input.go +++ b/input.go @@ -1,5 +1,11 @@ package main +// input.go — the y/n prompts that guard the destructive commands. +// +// These questions are the only thing standing between `init` and a wiped +// server repository, so the terminal handling here has to be exactly right: a +// question that cannot be answered is worse than no question at all. + import ( "fmt" "os" @@ -13,50 +19,86 @@ import ( // variable because it guards the destructive operations: tests replace it to // drive those paths without a terminal, and to assert that the question was // asked at all. +// +// When the keypress cannot be read at all the answer is no, whatever the +// default says — a question nobody saw must never be taken as consent. var yesno = func(prompt string, def bool) bool { suffix := " y/N ? " if def { suffix = " Y/n ? " } - ans := strings.ToLower(strings.TrimSpace(getkey(prompt + suffix))) + key, ok := getkey(prompt + suffix) + if !ok { + errorln("could not read an answer from the terminal — assuming no") + return false + } + ans := strings.ToLower(strings.TrimSpace(key)) if ans == "" { return def } return ans == "y" } -// getkey reads a single keypress from the terminal without echo. It reads a -// single byte directly from stdin; readline is not reading at this point (we -// are inside command execution), so there is no reader to desync with. +// keyModeArgs put the terminal into single-key input: one keypress is delivered +// as it is typed, and it is not echoed. +// +// MIN and TIME are set explicitly, and that is not decoration. They are not +// part of the canonical/non-canonical switch — they live in their own slots of +// the control-character array and survive `stty icanon`. drainTTY leaves them +// at "min 0 time 0" ("return what is buffered, do not wait"), so without this +// the *second* question of a session read zero bytes, answered itself with its +// default, and left the keypress queued for the next prompt line. +var keyModeArgs = []string{"-icanon", "-echo", "min", "1", "time", "0"} + +// getkey reads a single keypress from the terminal without echo, and reports +// whether it got one. It reads a single byte directly from stdin; readline only +// reads while it is inside Readline(), and we are inside command execution +// here, so there is no reader to desync with. // // Anything else already typed on the same line is discarded: answering "yes" // to a y/n prompt must not leave "es\n" queued for the next readline call, // where it would come back as a bogus command. -func getkey(prompt string) string { +func getkey(prompt string) (string, bool) { fmt.Print(prompt) - tty := readline.IsTerminal(int(os.Stdin.Fd())) - if tty { - stty("-icanon", "-echo") + + restore := func() {} + if stdinIsTTY() { + restore = singleKeyMode() } var buf [1]byte n, err := os.Stdin.Read(buf[:]) - if tty { - drainTTY() - stty("icanon", "echo") - } - key := "" - if err == nil && n > 0 { - key = strings.Trim(string(buf[:n]), "\r\n\t") + restore() + + if err != nil || n == 0 { + fmt.Println() + return "", false } + key := strings.Trim(string(buf[:n]), "\r\n\t") fmt.Println(key) - return key + return key, true +} + +// singleKeyMode switches the terminal to single-key input and returns the +// function that puts it back. The previous settings are restored verbatim from +// `stty -g` rather than by naming the flags we changed: naming them is how the +// MIN/TIME above were left behind in the first place, and mgsh should hand the +// terminal back exactly as it found it. +func singleKeyMode() func() { + saved, err := sttyRun("-g") + saved = strings.TrimSpace(saved) + sttyRun(keyModeArgs...) + + if err != nil || saved == "" { + return func() { drainTTY(); sttyRun("icanon", "echo") } // best effort + } + return func() { drainTTY(); sttyRun(strings.Fields(saved)...) } } // drainTTY discards input already queued on the terminal. `min 0 time 0` makes // a read return whatever is buffered without waiting, so this cannot block when -// nothing is pending. +// nothing is pending. Its caller restores the terminal afterwards. func drainTTY() { - stty("-icanon", "-echo", "min", "0", "time", "0") + sttyRun("-icanon", "-echo", "min", "0", "time", "0") buf := make([]byte, 256) for { n, err := os.Stdin.Read(buf) @@ -66,10 +108,16 @@ func drainTTY() { } } -func stty(args ...string) { +// stdinIsTTY reports whether keypresses come from a terminal. A variable so the +// tests can exercise the terminal path without one. +var stdinIsTTY = func() bool { return readline.IsTerminal(int(os.Stdin.Fd())) } + +// sttyRun runs stty on the terminal and returns its output. Errors are silent: +// every caller has a fallback, and a stray "stty: ..." line in the middle of a +// half-printed question helps nobody. +var sttyRun = func(args ...string) (string, error) { c := exec.Command("stty", args...) c.Stdin = os.Stdin - c.Stdout = os.Stdout - c.Stderr = os.Stderr - c.Run() + out, err := c.Output() + return string(out), err } diff --git a/input_test.go b/input_test.go new file mode 100644 index 0000000..b1f955f --- /dev/null +++ b/input_test.go @@ -0,0 +1,226 @@ +package main + +// input_test.go — the y/n prompt is the last thing between `init` and a wiped +// server repository, so the terminal handling around it is pinned here. + +import ( + "os" + "strings" + "sync" + "testing" + "time" +) + +// fakeTTY stands in for the terminal driver. It models the one detail that made +// the skipped-question bug possible: MIN and TIME are not part of the +// canonical/non-canonical switch, so they survive `stty icanon` and carry over +// into the next prompt. +type fakeTTY struct { + mu sync.Mutex + state map[string]string + calls [][]string + ready chan struct{} // signalled once single-key mode is in effect +} + +func newFakeTTY() *fakeTTY { + return &fakeTTY{ + state: map[string]string{"icanon": "on", "echo": "on", "min": "1", "time": "0"}, + ready: make(chan struct{}, 4), + } +} + +func (f *fakeTTY) run(args ...string) (string, error) { + f.mu.Lock() + defer f.mu.Unlock() + f.calls = append(f.calls, args) + + switch { + case len(args) == 1 && args[0] == "-g": + return f.serializeLocked(), nil + case len(args) == 1 && strings.HasPrefix(args[0], "saved:"): + for _, kv := range strings.Split(strings.TrimPrefix(args[0], "saved:"), ",") { + if k, v, ok := strings.Cut(kv, "="); ok { + f.state[k] = v + } + } + return "", nil + } + for i := 0; i < len(args); i++ { + switch a := args[i]; a { + case "min", "time": + if i+1 < len(args) { + f.state[a] = args[i+1] + i++ + } + default: + f.state[strings.TrimPrefix(a, "-")] = boolWord(!strings.HasPrefix(a, "-")) + } + } + if strings.Join(args, " ") == strings.Join(keyModeArgs, " ") { + f.ready <- struct{}{} // single-key mode is set; the read comes next + } + return "", nil +} + +func boolWord(on bool) string { + if on { + return "on" + } + return "off" +} + +// serializeLocked renders the settings as one token, the way `stty -g` does. +func (f *fakeTTY) serializeLocked() string { + return "saved:icanon=" + f.state["icanon"] + ",echo=" + f.state["echo"] + + ",min=" + f.state["min"] + ",time=" + f.state["time"] +} + +func (f *fakeTTY) get(k string) string { + f.mu.Lock() + defer f.mu.Unlock() + return f.state[k] +} + +func (f *fakeTTY) snapshot() string { + f.mu.Lock() + defer f.mu.Unlock() + return f.serializeLocked() +} + +// installFakeTTY points getkey at the fake terminal for the duration of a test. +func installFakeTTY(t *testing.T) *fakeTTY { + t.Helper() + f := newFakeTTY() + oldRun, oldIsTTY, oldStdin := sttyRun, stdinIsTTY, os.Stdin + sttyRun = f.run + stdinIsTTY = func() bool { return true } + t.Cleanup(func() { + sttyRun, stdinIsTTY, os.Stdin = oldRun, oldIsTTY, oldStdin + }) + return f +} + +// askOnce runs one getkey against the fake terminal, answering with keys once +// the terminal is actually in single-key mode. It reports the MIN in effect at +// the moment of the read — the value the old code got wrong. +func askOnce(t *testing.T, f *fakeTTY, keys string) (key string, ok bool, minAtRead string) { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + os.Stdin = r + + done := make(chan struct{}) + go func() { + key, ok = getkey("question? ") + close(done) + }() + + select { + case <-f.ready: + case <-time.After(5 * time.Second): + t.Fatal("the terminal was never put into single-key mode") + } + minAtRead = f.get("min") + w.WriteString(keys) + w.Close() // so the drain that follows the keypress sees EOF instead of blocking + <-done + r.Close() + return key, ok, minAtRead +} + +// TestEveryQuestionWaitsForAnAnswer is the regression test for the bug where the +// second y/n question of a session was skipped: drainTTY left the terminal at +// "min 0" ("return what is buffered, do not wait"), the restore only named +// icanon and echo, and so the next read returned zero bytes and answered the +// question with its default. +func TestEveryQuestionWaitsForAnAnswer(t *testing.T) { + f := installFakeTTY(t) + + for i, want := range []string{"y", "n", "y"} { + key, ok, minAtRead := askOnce(t, f, want) + if !ok || key != want { + t.Fatalf("question %d: got (%q, %v), want (%q, true)", i+1, key, ok, want) + } + if minAtRead != "1" { + t.Errorf("question %d read the terminal at min=%s, want min=1 — "+ + "min 0 returns without waiting and answers the question by itself", + i+1, minAtRead) + } + } +} + +// TestPromptHandsTheTerminalBackUnchanged: mgsh must not leave the terminal in +// a mode it chose. The old restore named only icanon and echo and left "min 0" +// behind, which outlived mgsh itself and broke the next program's single-key +// reads too. +func TestPromptHandsTheTerminalBackUnchanged(t *testing.T) { + f := installFakeTTY(t) + initial := f.snapshot() + + if _, ok, _ := askOnce(t, f, "y"); !ok { + t.Fatal("getkey did not read the key") + } + if got := f.snapshot(); got != initial { + t.Errorf("terminal left as %s, want it back at %s", got, initial) + } +} + +// TestPromptRestoresFromTheSavedState pins *how* the terminal is restored: from +// the state captured with `stty -g`, not by naming the flags we changed. +func TestPromptRestoresFromTheSavedState(t *testing.T) { + f := installFakeTTY(t) + if _, ok, _ := askOnce(t, f, "y"); !ok { + t.Fatal("getkey did not read the key") + } + + f.mu.Lock() + defer f.mu.Unlock() + if len(f.calls) == 0 { + t.Fatal("stty was never called") + } + if first := f.calls[0]; len(first) != 1 || first[0] != "-g" { + t.Errorf("first stty call was %v, want [-g]: the state has to be captured "+ + "before it is changed", first) + } + last := f.calls[len(f.calls)-1] + if len(last) != 1 || !strings.HasPrefix(last[0], "saved:") { + t.Errorf("last stty call was %v, want the saved state played back", last) + } +} + +// TestSingleKeyModeSetsMinExplicitly: MIN and TIME are inherited, so they have +// to be named on the way in. Without that, a terminal left at "min 0" by an +// earlier program (or an earlier mgsh) skips the question. +func TestSingleKeyModeSetsMinExplicitly(t *testing.T) { + args := strings.Join(keyModeArgs, " ") + for _, want := range []string{"-icanon", "-echo", "min 1", "time 0"} { + if !strings.Contains(args, want) { + t.Errorf("single-key mode is %q, missing %q", args, want) + } + } +} + +// TestQuestionSkippedByAPoisonedTerminalIsNotAYes: even if the terminal is +// already in the broken state when mgsh starts, the answer must not be the +// default — nobody saw the question, so nobody agreed to anything. +func TestQuestionSkippedByAPoisonedTerminalIsNotAYes(t *testing.T) { + oldRun, oldIsTTY, oldStdin := sttyRun, stdinIsTTY, os.Stdin + defer func() { sttyRun, stdinIsTTY, os.Stdin = oldRun, oldIsTTY, oldStdin }() + + sttyRun = func(args ...string) (string, error) { return "", nil } + stdinIsTTY = func() bool { return true } + + r, w, _ := os.Pipe() + w.Close() // a terminal that returns nothing at all + os.Stdin = r + defer r.Close() + + if key, ok := getkey("question? "); ok { + t.Fatalf("getkey reported a key %q from a terminal that gave none", key) + } + if yesno("destroy everything?", true) { + t.Error("an unanswerable question was taken as yes") + } +} diff --git a/version.txt b/version.txt index 5aa6b8c..7d92098 100644 --- a/version.txt +++ b/version.txt @@ -1 +1 @@ -4.0.56 +4.0.57