bug#62093: [PATCH] Let processes read nothing from stdin in tramp

bug#62093: [PATCH] Let processes read nothing from stdin in tramp

[Aleksander Trofimowicz]

[-- Attachment #1: Type: text/plain, Size: 200 bytes --]

This minor patch is about adjustments in the terminal line settings.

There are programs, which control flow depends on receiving 0
from a read call on stdin. A notable example is git.

--
Thanks,
at

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Let-processes-read-nothing-from-stdin-in-tramp.patch --]
[-- Type: text/x-patch, Size: 1257 bytes --]

From c4397c3261b9188262a1adee278075893410fb60 Mon Sep 17 00:00:00 2001
From: Aleksander Trofimowicz <trof@n90.eu>
Date: Wed, 25 Oct 2023 11:02:00 +0000
Subject: [PATCH] Let processes read nothing from stdin in tramp

There are programs, which control flow depends on receiving 0
from a read call on stdin. A notable example is git.

* lisp/net/tramp-sh.el (tramp-sh-handle-make-process): Use read
timeout instead of a minimal amount of data to be read in the
terminal line settings. (Bug#62093)
---
 lisp/net/tramp-sh.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/net/tramp-sh.el b/lisp/net/tramp-sh.el
index ba6dbdf0c39..a26c1e3fcc0 100644
--- a/lisp/net/tramp-sh.el
+++ b/lisp/net/tramp-sh.el
@@ -3093,9 +3093,9 @@ tramp-sh-handle-make-process
 			      ;; FIXME: Shall we rather use "stty raw"?
 			      (if (tramp-check-remote-uname v "Darwin")
 				  (tramp-send-command
-				   v "stty -icanon min 1 time 0")
+				   v "stty -icanon min 0 time 1")
 				(tramp-send-command
-				 v "stty -icrnl -icanon min 1 time 0")))
+				 v "stty -icrnl -icanon min 0 time 1")))
 			    ;; `tramp-maybe-open-connection' and
 			    ;; `tramp-send-command-and-read' could
 			    ;; have trashed the connection buffer.
-- 
2.42.0

bug#62093: [PATCH] Let processes read nothing from stdin in tramp

[Michael Albinus]

Aleksander Trofimowicz <trof@n90.eu> writes:

Hi Alexander,

> This minor patch is about adjustments in the terminal line settings.
>
> There are programs, which control flow depends on receiving 0
> from a read call on stdin. A notable example is git.

I wanted to let you know that I found time to check this. Thanks for the
patch.

I can confirm, that I could reproduce the problem with the recipe given
by Benjamin. And your patch fixes this in my local environment.

Since it is a very low level change, I ran a full test campaign with
recent Tramp 2.6 and your patch over the last days. I've used
tramp-tests.el for this, targeting many different remote hosts. 60
different configurations have passed, but unfortunately, 3
configurations have failed. All of them did fail like

--8<---------------cut here---------------start------------->8---
Running 92 tests (2023-11-04 07:23:29+0100, selector ‘(not (tag :unstable))’)
Remote directory: ‘/sshx:ford|su:albinus@|ssh:albinus@detlef|sudo::/root/tmp’
...
Test tramp-test30-make-process condition:
    (ert-test-failed "`tramp-test30-make-process' timed out")
   FAILED  56/92  tramp-test30-make-process (31.354531 sec) at tramp-tests.el:5354
--8<---------------cut here---------------end--------------->8---

The failed test runs have in common that there is a complex Tramp target
for the test using multi-hops, like above. I suspect a problem with
timing due to the several hops, but I don't know exactly what's up.

Will continue to debug. Just to let you know.

> Thanks,
> at

Best regards, Michael.

bug#62093: [PATCH] Let processes read nothing from stdin in tramp

[Aleksander Trofimowicz via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Michael,

Michael Albinus <michael.albinus@gmx.de> writes:

> I wanted to let you know that I found time to check this. Thanks for the
> patch.
>
Thank you for looking into this issue.

> The failed test runs have in common that there is a complex Tramp target
> for the test using multi-hops, like above. I suspect a problem with
> timing due to the several hops, but I don't know exactly what's up.
>
Triggered by your multi-hop tests (host B via host A) I run git-apply
actions over Tramp with the patch I submitted. I settled with 3
different test environment configurations; all of them should provide
the same functionality:

1. An explicit multi-hop target "/ssh:host_A|/ssh:host_B:..."

2. The SSH client option ProxyCommand set to "ssh host_A nc host_B 22" and
simple "/ssh:host_B"

3. The SSH client option ProxyJump set to "host_A" and simple "/ssh:host_B"

tramp-use-connection-share was set to nil in each case.

It worked as expected only in the first two cases. As far as the last one
is concerned, the workflow wasn't stalled, but it turned out to be no-op
after all.

It seems the last test environment enables the most responsive data
stream: no additional userland process is forked on a middle box and
Nagle's algorithm is disabled for all TCP connections involved (which is
not true for the case no 2.). In the end such results might corroborate
your theory.

--
at

bug#62093: [PATCH] Let processes read nothing from stdin in tramp

[Michael Albinus]

[-- Attachment #1: Type: text/plain, Size: 2399 bytes --]

Aleksander Trofimowicz <trof@n90.eu> writes:

> Hi Michael,

Hi Alexander,

>> The failed test runs have in common that there is a complex Tramp target
>> for the test using multi-hops, like above. I suspect a problem with
>> timing due to the several hops, but I don't know exactly what's up.
>>
> Triggered by your multi-hop tests (host B via host A) I run git-apply
> actions over Tramp with the patch I submitted. I settled with 3
> different test environment configurations; all of them should provide
> the same functionality:
>
> 1. An explicit multi-hop target "/ssh:host_A|/ssh:host_B:..."
>
> 2. The SSH client option ProxyCommand set to "ssh host_A nc host_B 22" and
> simple "/ssh:host_B"
>
> 3. The SSH client option ProxyJump set to "host_A" and simple "/ssh:host_B"
>
> tramp-use-connection-share was set to nil in each case.
>
> It worked as expected only in the first two cases. As far as the last one
> is concerned, the workflow wasn't stalled, but it turned out to be no-op
> after all.
>
> It seems the last test environment enables the most responsive data
> stream: no additional userland process is forked on a middle box and
> Nagle's algorithm is disabled for all TCP connections involved (which is
> not true for the case no 2.). In the end such results might corroborate
> your theory.

Well, I've tried deeper debugging with this. But for whatever values
I've taken for stty's min and time in tramp-sh-handle-make-process,
there were always cases it didn't work. Rare cases, and not always
reproducible due to race conditions, but they exist.

Going a step back to magit, I see the following comment in magit-start-process:

--8<---------------cut here---------------start------------->8---
               ;; Don't use a pty, because it would set icrnl
               ;; which would modify the input (issue #20).
--8<---------------cut here---------------end--------------->8---

I cannot speak for the local case. But in Tramp, we need to set "stty
-icanon ..." for the pipe connection type in order avoid blocking
situations with larger hunks of data, see the comment there. And the use
case of magit-start-process would be better with the connection type
pty, at least when calling Tramp.

The appended (rather naïve) patch fixes the reported use case in my
local enviroment.

> at

Best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1146 bytes --]

*** /home/albinus/.emacs.d/elpa/magit-20231103.1516/magit-process.el~	2023-11-04 11:43:11.848351502 +0100
--- /home/albinus/.emacs.d/elpa/magit-20231103.1516/magit-process.el	2023-11-11 13:26:23.212027014 +0100
***************
*** 583,589 ****
          (let ((process-connection-type
                 ;; Don't use a pty, because it would set icrnl
                 ;; which would modify the input (issue #20).
!                (and (not input) magit-process-connection-type))
                (process-environment (magit-process-environment))
                (default-process-coding-system (magit--process-coding-system)))
            (apply #'start-file-process
--- 583,590 ----
          (let ((process-connection-type
                 ;; Don't use a pty, because it would set icrnl
                 ;; which would modify the input (issue #20).
! 	       (or (and (file-remote-p default-directory) t)
! 		   (and (not input) magit-process-connection-type)))
                (process-environment (magit-process-environment))
                (default-process-coding-system (magit--process-coding-system)))
            (apply #'start-file-process

bug#62093: [PATCH] Let processes read nothing from stdin in tramp

[Aleksander Trofimowicz via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Michael,

Michael Albinus <michael.albinus@gmx.de> writes:

> I cannot speak for the local case. But in Tramp, we need to set "stty
> -icanon ..." for the pipe connection type in order avoid blocking
> situations with larger hunks of data, see the comment there. And the use
> case of magit-start-process would be better with the connection type
> pty, at least when calling Tramp.
>
Re-configuring a TTY device via "-icanon" does not only lift the 4k input
line limit; there are other far-reaching consequences. Most notably: a
handful of the terminal special characters are delivered straight to
stdin of a managed process, which more often than not is unprepared to
handle such a situation (e.g. the VEOF character). I would argue in the
Tramp context doubly so, since the connection-type is set to 'pipe'
somewhere up in a stack. It also invalidates certain expectations most
people have while invoking process-send-eof.

These are reasons magit stopped working, and my patch was a half-baked
attempt to address the problem by other means. Please drop it. At the
same time I would encourage you to revert the change that introduced the
noncanonical mode, since the current state is very confusing and breaks
existing code. A dedicated defcustom should cater for those who deal
with large input data.

Ideally no pty should be allocated if one asks for 'pipe' (by employing
ssh connection multiplexing?).
--
Kind regards,
at

bug#62093: [PATCH] Let processes read nothing from stdin in tramp

[Michael Albinus]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

Aleksander Trofimowicz <trof@n90.eu> writes:

> Hi Michael,

Hi Aleksander,

> These are reasons magit stopped working, and my patch was a half-baked
> attempt to address the problem by other means. Please drop it. At the
> same time I would encourage you to revert the change that introduced the
> noncanonical mode, since the current state is very confusing and breaks
> existing code. A dedicated defcustom should cater for those who deal
> with large input data.

I've tried several approaches, none of them did satisfy me
completely. So I have introduced a new user option
tramp-pipe-stty-settings, which you can use for different stty
settings. See appended patch. Let-binding this to "-icanon min 0 time 1"
in magit-start-process has at least solved the problem as shown here in
this bug report. I'm not very happy with this, but it is the best I
could do now. Could you please check?

> Ideally no pty should be allocated if one asks for 'pipe' (by employing
> ssh connection multiplexing?).

We have already ssh multiplexing. If you like to avoid pty's at all, you
might try to use direct async processes:

(info "(tramp)Improving performance of asynchronous remote processes")

> Kind regards,
> at

Best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1991 bytes --]

diff --git a/lisp/tramp-sh.el b/lisp/tramp-sh.el
index aa1d025b..f0e23950 100644
--- a/lisp/tramp-sh.el
+++ b/lisp/tramp-sh.el
@@ -2876,7 +2876,16 @@ the result will be a local, non-Tramp, file name."
 		(tramp-run-real-handler
 		 #'expand-file-name (list localname))))))))))

-;;; Remote commands:
+;;; Remote processes:
+
+(defcustom tramp-pipe-stty-settings "-icanon min 1 time 0"
+  "How to prevent blocking read in pipeline processes.
+This is used in `make-process' with `connection-type' `pipe'."
+  :group 'tramp
+  :version "29.2"
+  :type '(choice (const :tag "Use size limit" "-icanon min 1 time 0")
+		 (const :tag "Use timeout" "-icanon min 0 time 1")
+		 string))

 ;; We use BUFFER also as connection buffer during setup.  Because of
 ;; this, its original contents must be saved, and restored once
@@ -3087,12 +3096,21 @@ implementation will be used."
 			      ;; otherwise strings larger than 4096
 			      ;; bytes, sent by the process, could
 			      ;; block, see termios(3) and Bug#61341.
+			      ;; In order to prevent blocking read
+			      ;; from pipe processes, "stty -icanon"
+			      ;; is used.  By default, it expects at
+			      ;; least one character to read.  When a
+			      ;; process does not read from stdin,
+			      ;; like magit, it should set a timeout
+			      ;; instead. See`tramp-pipe-stty-settings'.
+			      ;; (Bug#62093)
 			      ;; FIXME: Shall we rather use "stty raw"?
-			      (if (tramp-check-remote-uname v "Darwin")
-				  (tramp-send-command
-				   v "stty -icanon min 1 time 0")
-				(tramp-send-command
-				 v "stty -icrnl -icanon min 1 time 0")))
+			      (tramp-send-command
+			       v (format
+				  "stty %s %s"
+				  (if (tramp-check-remote-uname v "Darwin")
+				      "" "-icrnl")
+				  tramp-pipe-stty-settings)))
 			    ;; `tramp-maybe-open-connection' and
 			    ;; `tramp-send-command-and-read' could
 			    ;; have trashed the connection buffer.

bug#62093: [PATCH] Let processes read nothing from stdin in tramp

[Aleksander Trofimowicz via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Michael,

Michael Albinus <michael.albinus@gmx.de> writes:

> I've tried several approaches, none of them did satisfy me
> completely. So I have introduced a new user option
> tramp-pipe-stty-settings, which you can use for different stty
> settings. See appended patch. Let-binding this to "-icanon min 0 time 1"
> in magit-start-process has at least solved the problem as shown here in
> this bug report. I'm not very happy with this, but it is the best I
> could do now. Could you please check?
>
The patch works as expected. However given the default value of
tramp-pipe-stty-settings you opted for, as well as the contents of
additional commentary in the code provided by this patch, I have got an
irresistible feeling I failed to convey the consequences of maintaining
the status quo. So let me show two simple cases:

1) open a terminal window on your local host

1.1. confirm it works in the canonical mode

$ stty -a|grep icanon

1.2. write 4 "E" characters to a random file, type ^D once, then write 4
more "E"s, and finally type ^D twice:

$ cat > /tmp/testfile
EEEEEEEE

1.3. check the contents of the target file with a hex dump
utility. Assuming you don't use fancy locale settings, this should look
like the following:

$ xxd /tmp/testfile
00000000: 4545 4545 4545 4545                      EEEEEEEE

2) now the same workflow but in the non-canonical mode

2.1. disable the canonical mode:

$ stty -icanon; stty -a|grep icanon

2. write 4 "E" characters to a random file, type ^D once, then write 4
more "E"s, and finally type ^D twice, and observe that the special
characters are echoed back and a cat process refuses to terminate. Type
^C to actually terminate the process:

$ cat > /tmp/testfile2
EEEE^DEEEE^D^D^C

2.3 check the contents of the file

$ xxd /tmp/testfile2
00000000: 4545 4545 0445 4545 4504 04              EEEE.EEEE..

As you can see there are three additional bytes (carrying 0x04 values),
each one represents the VEOF character which in the canonical mode is
intercepted by the kernel TTY subsystem, and alters the way kernel
interacts with a user process. In the non-canonical mode it is simply
passed to userspace.

Indeed very few programs are prepared to handle those terminal special
characters, and if you use one that doesn't then at best it hangs. In
less favourable circumstances the process might silently corrupt data
along the way or produce other unpleasant results.

In the Magit case it is not Magit/Emacs itself that hangs, but a git
process upon a read syscall on pts/0 on a remote host:

$ ps axjf # filtered for brevity

   PPID     PID    PGID     SID TTY        TPGID STAT   UID   TIME COMMAND
      1     977     977     977 ?             -1 Ss       0   0:00 sshd: /usr/bin/sshd -D [listener] 0 of 10-100 startups
    977  251082  251082  251082 ?             -1 Ss       0   0:00  \_ sshd: guest [priv]
 251082  251087  251082  251082 ?             -1 S     1000   0:00  |   \_ sshd: guest@pts/0
 251087  251089  251089  251089 pts/0     251089 Ss+   1000   0:00  |       \_ git --no-pager --literal-pathspecs -c core.preloadindex=true -c log.showSignature=false -c color.ui=false -c color.diff=false apply --cached -p0 --ignore-space-change -

$ cat /proc/251089/status|head -n7
Name:	git
Umask:	0022
State:	S (sleeping)
Tgid:	251089
Ngid:	0
Pid:	251089
PPid:	251087
[...]

# cat /proc/251089/stack
[<0>] wait_woken+0x54/0x60
[<0>] n_tty_read+0x588/0x640
[<0>] tty_read+0x134/0x250
[<0>] vfs_read+0x1fe/0x350
[<0>] ksys_read+0x6f/0xf0
[<0>] do_syscall_64+0x5d/0x90
[<0>] entry_SYSCALL_64_after_hwframe+0x6e/0xd8

The git program in the apply mode expects to receive 0 upon read() in
order conclude its processing. If tramp-pipe-stty-settings is set to
'-icanon min 1 time 0', that is never going to happen, since Magit
resorts to use process-send-eof to designate the end of input data,
which in turn sends VEOF if it is detected that pty is in use (and it
is, since this is what Tramp does behind the scene). The VEOF character
looses its special meaning in the non-canonical mode hence a deadlock
ensues.

Given above I would argue the canonical mode should be set as the
default one.

>
> We have already ssh multiplexing. If you like to avoid pty's at all, you
> might try to use direct async processes:
>
> (info "(tramp)Improving performance of asynchronous remote processes")
>
In my case that's the optimal solution. Thank you!

--
Kind regards,
at

bug#62093: [PATCH] Let processes read nothing from stdin in tramp

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Aleksander Trofimowicz <trof@n90.eu> writes:

> Hi Michael,

Hi Aleksander,

> Given above I would argue the canonical mode should be set as the
> default one.

Canonical mode was used in the past, and there were other use cases
which have shown problems. There won't be a one-fits-all solution.

That's why I have pushed the proposed change (new defcustom
tramp-pipe-stty-settings), which could help magit and other packages to
influence the settings. Will appear on GNU ELPA as Tramp 2.6.2 in a
couple of days.

>> We have already ssh multiplexing. If you like to avoid pty's at all, you
>> might try to use direct async processes:
>>
>> (info "(tramp)Improving performance of asynchronous remote processes")
>>
> In my case that's the optimal solution. Thank you!

Godd to know.

> Kind regards,
> at

Best regards, Michael.