{"thread":{"id":"57016","subject":"[RFC PATCH 0/3] editor: teach it to protect itself from rogue editors","startedAt":"2021-12-02T03:55:10Z","lastAt":"2021-12-02T06:35:31Z","messageCount":5,"participants":["Carlo Marcelo Arenas Belón","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"442845","messageId":"20211202035446.1154-1-carenas@gmail.com","threadId":"57016","inReplyTo":null,"subject":"[RFC PATCH 0/3] editor: teach it to protect itself from rogue editors","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-12-02T03:54:43Z","receivedAt":"2021-12-02T03:55:10Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This series reimplements a problematic series that was partially\nreverted in 2.34.1, but addressing all raised concerns and being\na lot more conservative.\n\nThe first commit addresses directly the issue reported and that\nrequired the revert, but punts several changes to future series\nto make sure it is safe enough for backporting into maint if\nneeded, it also tries (probably too hard) to not introduce any\nbehaviour changes, so it might seem \"incomplete\", but expect\nfurther changes that will clean it up further.\n\nThe the second one is taken from a draft[1] and is ammended by\nthe third, hence why this is all still an RFC.\n\nCarlo Marcelo Arenas Belón (2):\n terminal: teach save_term to fail when not foreground\n fixup! editor: allow for saving/restoring terminal state\n\nJunio C Hamano (1):\n editor: allow for saving/restoring terminal state\n\n Documentation/config.txt | 2 ++\n Documentation/config/editor.txt | 8 ++++++++\n compat/terminal.c | 17 ++++++++++++++++-\n editor.c | 22 ++++++++++++++++++++--\n 4 files changed, 46 insertions(+), 3 deletions(-)\n create mode 100644 Documentation/config/editor.txt\n\n[1] https://lore.kernel.org/git/xmqq7dcnyh5o.fsf@gitster.g/\n-- \n2.34.1.460.g364565cfab\n\n"},{"id":"442846","messageId":"20211202035446.1154-2-carenas@gmail.com","threadId":"57016","inReplyTo":"20211202035446.1154-1-carenas@gmail.com","subject":"[RFC PATCH 1/3] terminal: teach save_term to fail when not foreground","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-12-02T03:54:44Z","receivedAt":"2021-12-02T03:55:12Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"e22b245ea5 (terminal: teach git how to save/restore its terminal\nsettings, 2021-10-05) allows external calls to the termios code,\nbut kept the assumption that all operations were done with\nforeground processes, which was proven incorrect.\n\nAdd a check to validate that the current process is indeed in the\nforeground and in control of the terminal and fail early if not the\ncase.\n\nTo avoid changing behaviour from the other users of save_term() the\nfull_duplex parameter has been overloaded to restrict the new check\nto only future callers, as it is set to 0 for all current users.\n\nThe detection is done in a helper function so it can be reused by\nall other functions that might benefit from it later, and once that\nis done that overloading might be unnecessary and cleaned up, but\ndoing so has been punted from this series as it is not needed and\nmight require backward incompatible changes.\n\nHelped-by: Phillip Wood \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n compat/terminal.c | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 5b903e7c7e..509f2518d1 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -29,16 +29,31 @@ void restore_term(void)\n \t\treturn;\n \n \ttcsetattr(term_fd, TCSAFLUSH, &old_term);\n+\n \tclose(term_fd);\n \tterm_fd = -1;\n }\n \n+static int is_controlling_terminal(int fd)\n+{\n+\treturn (getpgid(0) == tcgetpgrp(fd));\n+}\n+\n int save_term(int full_duplex)\n {\n \tif (term_fd < 0)\n \t\tterm_fd = open(\"/dev/tty\", O_RDWR);\n \n-\treturn (term_fd < 0) ? -1 : tcgetattr(term_fd, &old_term);\n+\tif (term_fd < 0)\n+\t\treturn -1;\n+\n+\tif (full_duplex && !is_controlling_terminal(term_fd)) {\n+\t\tclose(term_fd);\n+\t\tterm_fd = -1;\n+\t\treturn -1;\n+\t}\n+\n+\treturn tcgetattr(term_fd, &old_term);\n }\n \n static int disable_bits(tcflag_t bits)\n-- \n2.34.1.460.g364565cfab\n\n"},{"id":"442847","messageId":"20211202035446.1154-3-carenas@gmail.com","threadId":"57016","inReplyTo":"20211202035446.1154-1-carenas@gmail.com","subject":"[RFC PATCH 2/3] editor: allow for saving/restoring terminal state","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-12-02T03:54:45Z","receivedAt":"2021-12-02T03:55:12Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: Junio C Hamano \n\nWhen EDITOR is invoked to modify a commit message (or do some\nother editing), and it is a terminal mode editor, it will need to\nchange the terminal settings, and if it misbehaves could leave the\nterminal output damaged as shown in recent reports from Windows\nTerminal[1]\n\nInstead use the functions provided by compat/terminal to save the\nsettings of the terminal and recover safely, but only do so if\nthe editor is known to have issues and unless the user has told\nus through a boolean configuration \"editor.stty\" that it is safe\nnot to do so.\n\n[1] https://github.com/microsoft/terminal/issues/9359\n---\nThe commit message was contributed by me, but the rest comes from:\nhttps://lore.kernel.org/git/xmqq7dcnyh5o.fsf@gitster.g/\nSigned-of-by is missing as I didn't know what would be preferred\nor if the Author will be ok with my grammar (ginger free grammar\ncheck seems to approve, otherwise)\n\n editor.c | 18 +++++++++++++++++-\n 1 file changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/editor.c b/editor.c\nindex fdd3eeafa9..70d3f80966 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -3,6 +3,7 @@\n #include \"strbuf.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\n+#include \"compat/terminal.h\"\n \n #ifndef DEFAULT_EDITOR\n #define DEFAULT_EDITOR \"vi\"\n@@ -47,6 +48,16 @@ const char *git_sequence_editor(void)\n \treturn editor;\n }\n \n+static int prepare_term(const char *editor)\n+{\n+\tint need_saverestore = !strcmp(editor, \"vi\");\n+\n+\tgit_config_get_bool(\"editor.stty\", &need_saverestore);\n+\tif (need_saverestore)\n+\t\treturn save_term(1);\n+\treturn 0;\n+}\n+\n static int launch_specified_editor(const char *editor, const char *path,\n \t\t\t\t struct strbuf *buffer, const char *const *env)\n {\n@@ -57,7 +68,7 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tstruct strbuf realpath = STRBUF_INIT;\n \t\tconst char *args[] = { editor, NULL, NULL };\n \t\tstruct child_process p = CHILD_PROCESS_INIT;\n-\t\tint ret, sig;\n+\t\tint ret, sig, need_restore = 0;\n \t\tint print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2);\n \n \t\tif (print_waiting_for_editor) {\n@@ -83,7 +94,10 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tp.env = env;\n \t\tp.use_shell = 1;\n \t\tp.trace2_child_class = \"editor\";\n+\t\tneed_restore = prepare_term(editor);\n \t\tif (start_command(&p) < 0) {\n+\t\t\tif (need_restore)\n+\t\t\t\trestore_term();\n \t\t\tstrbuf_release(&realpath);\n \t\t\treturn error(\"unable to start editor '%s'\", editor);\n \t\t}\n@@ -91,6 +105,8 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tsigchain_push(SIGINT, SIG_IGN);\n \t\tsigchain_push(SIGQUIT, SIG_IGN);\n \t\tret = finish_command(&p);\n+\t\tif (need_restore)\n+\t\t\trestore_term();\n \t\tstrbuf_release(&realpath);\n \t\tsig = ret - 128;\n \t\tsigchain_pop(SIGINT);\n-- \n2.34.1.460.g364565cfab\n\n"},{"id":"442848","messageId":"20211202035446.1154-4-carenas@gmail.com","threadId":"57016","inReplyTo":"20211202035446.1154-1-carenas@gmail.com","subject":"[RFC PATCH 3/3] fixup! editor: allow for saving/restoring terminal state","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-12-02T03:54:46Z","receivedAt":"2021-12-02T03:55:15Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Use DEFAULT_EDITOR instead of another hardcoding of \"vi\".\n\nDo a minor refactoring to avoid having to call isatty(2) twice,\nand while at it, a related reformatting that avoids an overlong\nline.\n\nOnly check if we should enable saving/restoring the terminal if\nthe session is interactive.\n\nAdd documentation for the configuration variable.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nAll of the commit message is meant to be thrown away once/if\nthis is squashed into the previous commit.\n\nSigned-off-by could be transformed into a Helped-by if anything\ncould be of use here otherwise.\n\n Documentation/config.txt | 2 ++\n Documentation/config/editor.txt | 8 ++++++++\n editor.c | 10 ++++++----\n 3 files changed, 16 insertions(+), 4 deletions(-)\n create mode 100644 Documentation/config/editor.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1167e88e34..342f3cc183 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -361,6 +361,8 @@ include::config/diff.txt[]\n \n include::config/difftool.txt[]\n \n+include::config/editor.txt[]\n+\n include::config/extensions.txt[]\n \n include::config/fastimport.txt[]\ndiff --git a/Documentation/config/editor.txt b/Documentation/config/editor.txt\nnew file mode 100644\nindex 0000000000..b65c2810fa\n--- /dev/null\n+++ b/Documentation/config/editor.txt\n@@ -0,0 +1,8 @@\n+editor.stty::\n+\tA boolean variable that tells git to save and restore its\n+\tterminal settings when the editor is invoked, to avoid\n+\tbeing affected by garbled input/output, if it misbehaves.\n++\n+It defaults to 'true' if your editor is \"vi\" (which is\n+also the default if there is no `core.editor`), or to\n+'false' otherwise.\ndiff --git a/editor.c b/editor.c\nindex 70d3f80966..12180039d2 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -50,7 +50,7 @@ const char *git_sequence_editor(void)\n \n static int prepare_term(const char *editor)\n {\n-\tint need_saverestore = !strcmp(editor, \"vi\");\n+\tint need_saverestore = !strcmp(editor, DEFAULT_EDITOR);\n \n \tgit_config_get_bool(\"editor.stty\", &need_saverestore);\n \tif (need_saverestore)\n@@ -68,8 +68,10 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tstruct strbuf realpath = STRBUF_INIT;\n \t\tconst char *args[] = { editor, NULL, NULL };\n \t\tstruct child_process p = CHILD_PROCESS_INIT;\n-\t\tint ret, sig, need_restore = 0;\n-\t\tint print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2);\n+\t\tint ret, sig, need_restore;\n+\t\tint is_interactive = isatty(2);\n+\t\tint print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) &&\n+\t\t\t\t\t\tis_interactive;\n \n \t\tif (print_waiting_for_editor) {\n \t\t\t/*\n@@ -94,7 +96,7 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tp.env = env;\n \t\tp.use_shell = 1;\n \t\tp.trace2_child_class = \"editor\";\n-\t\tneed_restore = prepare_term(editor);\n+\t\tneed_restore = is_interactive ? prepare_term(editor) : 0;\n \t\tif (start_command(&p) < 0) {\n \t\t\tif (need_restore)\n \t\t\t\trestore_term();\n-- \n2.34.1.460.g364565cfab\n\n"},{"id":"442853","messageId":"xmqqtufrwm38.fsf@gitster.g","threadId":"57016","inReplyTo":"20211202035446.1154-3-carenas@gmail.com","subject":"Re: [RFC PATCH 2/3] editor: allow for saving/restoring terminal state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-02T06:35:23Z","receivedAt":"2021-12-02T06:35:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> From: Junio C Hamano \n>\n> When EDITOR is invoked to modify a commit message (or do some\n> other editing), and it is a terminal mode editor, it will need to\n> change the terminal settings, and if it misbehaves could leave the\n> terminal output damaged as shown in recent reports from Windows\n> Terminal[1]\n>\n> Instead use the functions provided by compat/terminal to save the\n> settings of the terminal and recover safely, but only do so if\n> the editor is known to have issues and unless the user has told\n> us through a boolean configuration \"editor.stty\" that it is safe\n> not to do so.\n>\n> [1] https://github.com/microsoft/terminal/issues/9359\n> ---\n> The commit message was contributed by me, but the rest comes from:\n> https://lore.kernel.org/git/xmqq7dcnyh5o.fsf@gitster.g/\n> Signed-of-by is missing as I didn't know what would be preferred\n> or if the Author will be ok with my grammar (ginger free grammar\n> check seems to approve, otherwise)\n\nIt was merely an illustration patch, without 3/3 it would not be\ncomplete, and most importantly, finishing the last mile like 3/3 did\nis 80% of the work. I'd rather see you take the authorship of the\ncommit that results from the squashing of 2 and 3.\n\nAny patch I send here is by default signed off by me.\n\nThanks.\n"}]}