[RFC PATCH 0/3] editor: teach it to protect itself from rogue editors

[RFC PATCH 0/3] editor: teach it to protect itself from rogue editors

[Carlo Marcelo Arenas Belón]

This series reimplements a problematic series that was partially
reverted in 2.34.1, but addressing all raised concerns and being
a lot more conservative.

The first commit addresses directly the issue reported and that
required the revert, but punts several changes to future series
to make sure it is safe enough for backporting into maint if
needed, it also tries (probably too hard) to not introduce any
behaviour changes, so it might seem "incomplete", but expect
further changes that will clean it up further.

The the second one is taken from a draft[1] and is ammended by
the third, hence why this is all still an RFC.

Carlo Marcelo Arenas Belón (2):
  terminal: teach save_term to fail when not foreground
  fixup! editor: allow for saving/restoring terminal state

Junio C Hamano (1):
  editor: allow for saving/restoring terminal state

 Documentation/config.txt        |  2 ++
 Documentation/config/editor.txt |  8 ++++++++
 compat/terminal.c               | 17 ++++++++++++++++-
 editor.c                        | 22 ++++++++++++++++++++--
 4 files changed, 46 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/config/editor.txt

[1] https://lore.kernel.org/git/xmqq7dcnyh5o.fsf@gitster.g/
-- 
2.34.1.460.g364565cfab

[RFC PATCH 1/3] terminal: teach save_term to fail when not foreground

[Carlo Marcelo Arenas Belón]

e22b245ea5 (terminal: teach git how to save/restore its terminal
settings, 2021-10-05) allows external calls to the termios code,
but kept the assumption that all operations were done with
foreground processes, which was proven incorrect.

Add a check to validate that the current process is indeed in the
foreground and in control of the terminal and fail early if not the
case.

To avoid changing behaviour from the other users of save_term() the
full_duplex parameter has been overloaded to restrict the new check
to only future callers, as it is set to 0 for all current users.

The detection is done in a helper function so it can be reused by
all other functions that might benefit from it later, and once that
is done that overloading might be unnecessary and cleaned up, but
doing so has been punted from this series as it is not needed and
might require backward incompatible changes.

Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 compat/terminal.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 5b903e7c7e..509f2518d1 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -29,16 +29,31 @@ void restore_term(void)
 		return;
 
 	tcsetattr(term_fd, TCSAFLUSH, &old_term);
+
 	close(term_fd);
 	term_fd = -1;
 }
 
+static int is_controlling_terminal(int fd)
+{
+	return (getpgid(0) == tcgetpgrp(fd));
+}
+
 int save_term(int full_duplex)
 {
 	if (term_fd < 0)
 		term_fd = open("/dev/tty", O_RDWR);
 
-	return (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term);
+	if (term_fd < 0)
+		return -1;
+
+	if (full_duplex && !is_controlling_terminal(term_fd)) {
+		close(term_fd);
+		term_fd = -1;
+		return -1;
+	}
+
+	return tcgetattr(term_fd, &old_term);
 }
 
 static int disable_bits(tcflag_t bits)
-- 
2.34.1.460.g364565cfab

[RFC PATCH 2/3] editor: allow for saving/restoring terminal state

[Carlo Marcelo Arenas Belón]

From: Junio C Hamano <gitster@pobox.com>

When EDITOR is invoked to modify a commit message (or do some
other editing), and it is a terminal mode editor, it will need to
change the terminal settings, and if it misbehaves could leave the
terminal output damaged as shown in recent reports from Windows
Terminal[1]

Instead use the functions provided by compat/terminal to save the
settings of the terminal and recover safely, but only do so if
the editor is known to have issues and unless the user has told
us through a boolean configuration "editor.stty" that it is safe
not to do so.

[1] https://github.com/microsoft/terminal/issues/9359
---
The commit message was contributed by me, but the rest comes from:
https://lore.kernel.org/git/xmqq7dcnyh5o.fsf@gitster.g/
Signed-of-by is missing as I didn't know what would be preferred
or if the Author will be ok with my grammar (ginger free grammar
check seems to approve, otherwise)

 editor.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/editor.c b/editor.c
index fdd3eeafa9..70d3f80966 100644
--- a/editor.c
+++ b/editor.c
@@ -3,6 +3,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "compat/terminal.h"
 
 #ifndef DEFAULT_EDITOR
 #define DEFAULT_EDITOR "vi"
@@ -47,6 +48,16 @@ const char *git_sequence_editor(void)
 	return editor;
 }
 
+static int prepare_term(const char *editor)
+{
+	int need_saverestore = !strcmp(editor, "vi");
+
+	git_config_get_bool("editor.stty", &need_saverestore);
+	if (need_saverestore)
+		return save_term(1);
+	return 0;
+}
+
 static int launch_specified_editor(const char *editor, const char *path,
 				   struct strbuf *buffer, const char *const *env)
 {
@@ -57,7 +68,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 		struct strbuf realpath = STRBUF_INIT;
 		const char *args[] = { editor, NULL, NULL };
 		struct child_process p = CHILD_PROCESS_INIT;
-		int ret, sig;
+		int ret, sig, need_restore = 0;
 		int print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2);
 
 		if (print_waiting_for_editor) {
@@ -83,7 +94,10 @@ static int launch_specified_editor(const char *editor, const char *path,
 		p.env = env;
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
+		need_restore = prepare_term(editor);
 		if (start_command(&p) < 0) {
+			if (need_restore)
+				restore_term();
 			strbuf_release(&realpath);
 			return error("unable to start editor '%s'", editor);
 		}
@@ -91,6 +105,8 @@ static int launch_specified_editor(const char *editor, const char *path,
 		sigchain_push(SIGINT, SIG_IGN);
 		sigchain_push(SIGQUIT, SIG_IGN);
 		ret = finish_command(&p);
+		if (need_restore)
+			restore_term();
 		strbuf_release(&realpath);
 		sig = ret - 128;
 		sigchain_pop(SIGINT);
-- 
2.34.1.460.g364565cfab

Re: [RFC PATCH 2/3] editor: allow for saving/restoring terminal state

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> From: Junio C Hamano <gitster@pobox.com>
>
> When EDITOR is invoked to modify a commit message (or do some
> other editing), and it is a terminal mode editor, it will need to
> change the terminal settings, and if it misbehaves could leave the
> terminal output damaged as shown in recent reports from Windows
> Terminal[1]
>
> Instead use the functions provided by compat/terminal to save the
> settings of the terminal and recover safely, but only do so if
> the editor is known to have issues and unless the user has told
> us through a boolean configuration "editor.stty" that it is safe
> not to do so.
>
> [1] https://github.com/microsoft/terminal/issues/9359
> ---
> The commit message was contributed by me, but the rest comes from:
> https://lore.kernel.org/git/xmqq7dcnyh5o.fsf@gitster.g/
> Signed-of-by is missing as I didn't know what would be preferred
> or if the Author will be ok with my grammar (ginger free grammar
> check seems to approve, otherwise)

It was merely an illustration patch, without 3/3 it would not be
complete, and most importantly, finishing the last mile like 3/3 did
is 80% of the work.  I'd rather see you take the authorship of the
commit that results from the squashing of 2 and 3.

Any patch I send here is by default signed off by me.

Thanks.

[RFC PATCH 3/3] fixup! editor: allow for saving/restoring terminal state

[Carlo Marcelo Arenas Belón]

Use DEFAULT_EDITOR instead of another hardcoding of "vi".

Do a minor refactoring to avoid having to call isatty(2) twice,
and while at it, a related reformatting that avoids an overlong
line.

Only check if we should enable saving/restoring the terminal if
the session is interactive.

Add documentation for the configuration variable.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
All of the commit message is meant to be thrown away once/if
this is squashed into the previous commit.

Signed-off-by could be transformed into a Helped-by if anything
could be of use here otherwise.

 Documentation/config.txt        |  2 ++
 Documentation/config/editor.txt |  8 ++++++++
 editor.c                        | 10 ++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/config/editor.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1167e88e34..342f3cc183 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -361,6 +361,8 @@ include::config/diff.txt[]
 
 include::config/difftool.txt[]
 
+include::config/editor.txt[]
+
 include::config/extensions.txt[]
 
 include::config/fastimport.txt[]
diff --git a/Documentation/config/editor.txt b/Documentation/config/editor.txt
new file mode 100644
index 0000000000..b65c2810fa
--- /dev/null
+++ b/Documentation/config/editor.txt
@@ -0,0 +1,8 @@
+editor.stty::
+	A boolean variable that tells git to save and restore its
+	terminal settings when the editor is invoked, to avoid
+	being affected by garbled input/output, if it misbehaves.
++
+It defaults to 'true' if your editor is "vi" (which is
+also the default if there is no `core.editor`), or to
+'false' otherwise.
diff --git a/editor.c b/editor.c
index 70d3f80966..12180039d2 100644
--- a/editor.c
+++ b/editor.c
@@ -50,7 +50,7 @@ const char *git_sequence_editor(void)
 
 static int prepare_term(const char *editor)
 {
-	int need_saverestore = !strcmp(editor, "vi");
+	int need_saverestore = !strcmp(editor, DEFAULT_EDITOR);
 
 	git_config_get_bool("editor.stty", &need_saverestore);
 	if (need_saverestore)
@@ -68,8 +68,10 @@ static int launch_specified_editor(const char *editor, const char *path,
 		struct strbuf realpath = STRBUF_INIT;
 		const char *args[] = { editor, NULL, NULL };
 		struct child_process p = CHILD_PROCESS_INIT;
-		int ret, sig, need_restore = 0;
-		int print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2);
+		int ret, sig, need_restore;
+		int is_interactive = isatty(2);
+		int print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) &&
+						is_interactive;
 
 		if (print_waiting_for_editor) {
 			/*
@@ -94,7 +96,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 		p.env = env;
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
-		need_restore = prepare_term(editor);
+		need_restore = is_interactive ? prepare_term(editor) : 0;
 		if (start_command(&p) < 0) {
 			if (need_restore)
 				restore_term();
-- 
2.34.1.460.g364565cfab