Message-Id: <20220329235810.452513-1-daniel@gibson.sh>
Date:   Wed, 30 Mar 2022 01:58:09 +0200
From:   Daniel Gibson <daniel@...son.sh>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>, linux-kernel@...r.kernel.org
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Peter Hurley <peter@...leysoftware.com>,
        Daniel Gibson <daniel@...son.sh>
Subject: [PATCH 0/1]  tty: n_tty: Restore EOF push handling behavior

Since Kernel 5.12 there was a bug in Pseudo-Terminals (it has been fixed
recently) in ICANON mode where the terminating newline was missing in a
read() if the line had exactly 64 chars PLUS newline - and that newline
was then returned at the beginning of the next line that was read().

https://bugzilla.kernel.org/show_bug.cgi?id=215611 has more information
on this bug, but I'll summarize things relevant to this patch here:

This bug was introduced by 3b830a9c34d5 ("tty: convert tty_ldisc_ops
'read()' function to take a kernel pointer") but it turned out that a
slight variation of it existed even before (if the newline was just
after the buffer size passed to read()).
That bug had been introduced by ac8f3bf8832a ("n_tty: Fix poll() after
buffer-limited eof push read") which fixed an edge-case very similar to
the one it broke - if there was a "EOF" pseudo-EOL character (that's
a __DISABLED_CHAR and thus not supposed to be returned to the reader)
directly after a read-buffer-sized line, this EOF character was supposed
to be silently discarded - and while doing this the case of a regular
EOL character (that's supposed to be returned to the reader, like '\n')
directly following the read-buffer was broken.
As this original bug was less likely to be triggered than the newer
64 char case (I guess usually the reader passes a buffer that's big
enough to hold the whole line) it has never been found.
This EOF/pseudo-EOL character handling is also called "EOF pushing", and
the character that's used for it is EOT (ASCII 0x4). Like a regular EOL
character it terminates a line that's written to a pseudo-terminal, but
unlike a regular EOL character it's not returned to the reader.

Anyway, the fix for the bug with 64 chars + newline (359303076163 
"tty: n_tty: do not look ahead for EOL character past the end of the buffer")
broke the edge case of discarding EOT after the read buffer size, which
causes the next read() to return 0, because it's detected as an empty
line with a newline-character that's not returned to the reader.

read() returning 0 in this case actually *is* valid behavior (FreeBSD
and OSX do it), and has the advantage that the reader can interpret it
as "the line I just received is over" - otherwise the reader can't know,
could as well have been an even longer line and the previous read() just
returned the first part of it.

On the other hand, read() returning 0 means EOF and the reader could
misinterpret it as "there's no more data, probably the other side
disconnected" and stop reading or even close the pseudo-terminal.
So it might indeed be a good idea to avoid this by silently discarding
EOF (EOT) characters and returning the next line, like Linux did before.
Solaris (or at least OpenIndiana 2021.10) does the same.

While both options are defensible (and are used by other Unix-likes),
IMHO it's best to stick to the "old" Linux behavior since 40d5e0905a03
("n_tty: Fix EOF push handling") of 2013, i.e. silently discard the EOF.

The following patch, which is based on a patch Linus gave me for
testing, restores that behavior.

I wrote a pretty extensive test for several edge-cases of PTYs in ICANON
mode (including the bugs mentioned before) which you can download at:
https://bugzilla.kernel.org/show_bug.cgi?id=215611#c10
I plan to port that standalone-test to kselftest eventually, but so far
haven't gotten around to doing that, so I'd prefer to get this patch
merged now and the test later once it's ported.


Daniel Gibson (1):
  tty: n_tty: Restore EOF push handling behavior

 drivers/tty/n_tty.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

-- 
2.25.1

Powered by blists - more mailing lists