[PATCH 1/3] terminal: pop signal handler when terminal is restored

["Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When disable_bits() changes the terminal attributes it uses
sigchain_push_common() to restore the terminal if a signal is received
before restore_term() is called. However there is no corresponding
call to sigchain_pop_common() when the settings are restored so the
signal handler is left on the sigchain stack. This leaves the stack
unbalanced so code such as

sigchain_push_common(my_handler);
...
read_key_without_echo(...);
...
sigchain_pop_common();

pops the handler pushed by disable_bits() rather than the one it
intended to. Additionally "git add -p" changes the terminal settings
every time it reads a key press so the stack can grow significantly.

In order to fix this save_term() now sets up the signal handler so
restore_term() can unconditionally call sigchain_pop_common(). There
are no callers of save_term() outside of terminal.c as the only
external caller was removed by e3f7e01b50 ("Revert "editor: save and
reset terminal after calling EDITOR"", 2021-11-22). Any future callers
of save_term() should benefit from having the signal handler set up
for them. For example if we reinstate the code to protect against an
editor failing to restore the terminal attributes we would want that
code to restore the terminal attributes on SIGINT. (As an aside
run_command() installs a signal handler that waits for the child
before re-raising the signal so we would be guaranteed to restore the
settings after the child has exited.)

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 compat/terminal.c | 17 +++++++++++++----
 compat/terminal.h |  8 ++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 5b903e7c7e3..20803badd03 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -11,7 +11,7 @@
 static void restore_term_on_signal(int sig)
 {
 	restore_term();
-	sigchain_pop(sig);
+	/* restore_term calls sigchain_pop_common */
 	raise(sig);
 }
 
@@ -31,14 +31,20 @@ void restore_term(void)
 	tcsetattr(term_fd, TCSAFLUSH, &old_term);
 	close(term_fd);
 	term_fd = -1;
+	sigchain_pop_common();
 }
 
 int save_term(int full_duplex)
 {
 	if (term_fd < 0)
 		term_fd = open("/dev/tty", O_RDWR);
+	if (term_fd < 0)
+		return -1;
+	if (tcgetattr(term_fd, &old_term) < 0)
+		return -1;
+	sigchain_push_common(restore_term_on_signal);
 
-	return (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term);
+	return 0;
 }
 
 static int disable_bits(tcflag_t bits)
@@ -49,12 +55,12 @@ static int disable_bits(tcflag_t bits)
 		goto error;
 
 	t = old_term;
-	sigchain_push_common(restore_term_on_signal);
 
 	t.c_lflag &= ~bits;
 	if (!tcsetattr(term_fd, TCSAFLUSH, &t))
 		return 0;
 
+	sigchain_pop_common();
 error:
 	close(term_fd);
 	term_fd = -1;
@@ -100,6 +106,8 @@ void restore_term(void)
 		return;
 	}
 
+	sigchain_pop_common();
+
 	if (hconin == INVALID_HANDLE_VALUE)
 		return;
 
@@ -134,6 +142,7 @@ int save_term(int full_duplex)
 
 	GetConsoleMode(hconin, &cmode_in);
 	use_stty = 0;
+	sigchain_push_common(restore_term_on_signal);
 	return 0;
 error:
 	CloseHandle(hconin);
@@ -177,10 +186,10 @@ static int disable_bits(DWORD bits)
 	if (save_term(0) < 0)
 		return -1;
 
-	sigchain_push_common(restore_term_on_signal);
 	if (!SetConsoleMode(hconin, cmode_in & ~bits)) {
 		CloseHandle(hconin);
 		hconin = INVALID_HANDLE_VALUE;
+		sigchain_pop_common();
 		return -1;
 	}
 
diff --git a/compat/terminal.h b/compat/terminal.h
index e1770c575b2..0fb9fa147c4 100644
--- a/compat/terminal.h
+++ b/compat/terminal.h
@@ -1,7 +1,15 @@
 #ifndef COMPAT_TERMINAL_H
 #define COMPAT_TERMINAL_H
 
+/*
+ * Save the terminal attributes so they can be restored later by a
+ * call to restore_term(). Note that every successful call to
+ * save_term() must be matched by a call to restore_term() even if the
+ * attributes have not been changed. Returns 0 on success, -1 on
+ * failure.
+ */
 int save_term(int full_duplex);
+/* Restore the terminal attributes that were saved with save_term() */
 void restore_term(void);
 
 char *git_terminal_prompt(const char *prompt, int echo);
-- 
gitgitgadget