Bug 215611 - Pseudo Terminals misbehave with 65 char lines
Summary: Pseudo Terminals misbehave with 65 char lines
Status: NEW
Alias: None
Product: Drivers
Classification: Unclassified
Component: Serial (show other bugs)
Hardware: All Linux
: P1 high
Assignee: Russell King
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-02-15 22:18 UTC by Daniel Gibson
Modified: 2022-03-29 23:47 UTC (History)
1 user (show)

See Also:
Kernel Version: 5.17-rc4
Tree: Mainline
Regression: Yes


Attachments
simple test program that demonstrates the issue (3.15 KB, text/x-csrc)
2022-02-15 22:18 UTC, Daniel Gibson
Details
Possible patch (981 bytes, patch)
2022-02-15 23:37 UTC, Linus Torvalds
Details | Diff
More extensive test program that checks this and similar bugs (17.92 KB, text/x-csrc)
2022-03-29 23:47 UTC, Daniel Gibson
Details

Description Daniel Gibson 2022-02-15 22:18:00 UTC
Created attachment 300470 [details]
simple test program that demonstrates the issue

If you feed a line with exactly 64 chars + terminating newline, and directly afterwards (without reading) another line into a pseudo terminal, the the first read() on the other side will return the 64 char line *without* terminating newline, and the next read() will return the missing terminating newline AND the complete next line (if it fits in the buffer).

So the second read returns a line with two newline chars (one at the beginning, one at the end) - as far as I can tell that shouldn't happen with line-buffered terminals (in icanon mode).
This actually breaks software, for example GDB in MI mode shuts down after receiving a line with 64chars + newline, because the FILE object it uses to wrap the PTY gets into a weird state then and returns EOF when trying to read the next line, which will make gdb quit.

This bug doesn't happen with kernel 5.4, but it does happen with 5.13, so I bisected and apparently this bug was introduced by this commit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3b830a9c34d5897be07176ce4e6f2d75e2c8cfd7
(which is why I put Mr Torvalds in CC).

So it has been in all stable releases since 5.12, I was still able to reproduce it with the latest mainline HEAD code on 2022-02-12; I don't think any TTY/PTY code has changed since then.

I'm kinda surprised it hasn't been reported before (as far as I can tell), lines with that length shouldn't be super rare?
Even though it seems like no one ran into this bug before (or, more likely, no one was able to trace the weird behavior they might have seen back to the Kernel's PTY code), I think this is a severe bug (with something this fundamental not behaving as expected), so I set Severity to High.

A simple test program to reproduce the issue is attached, it creates a pseudo terminal with posix_openpt(), writes some suitable lines into the master, and reads and prints the received lines from the other side (puppet/slave).

(Unfortunately, so far I haven't been able to figure out what exactly goes wrong.)

Cheers,
Daniel
Comment 1 Linus Torvalds 2022-02-15 22:27:24 UTC
Thanks, great report.

And yeah, that 64-byte boundary is special. I clearly screwed up on getting the boundary case right. Let me go look.
Comment 2 Linus Torvalds 2022-02-15 22:39:14 UTC
(In reply to Linus Torvalds from comment #1)
> 
> And yeah, that 64-byte boundary is special. 

In case somebody else looks at this concurrently, let me just point to where it's special: iterate_tty_read() uses a 64-byte kernel buffer for the data it reads. I haven't really looked at the details yet, but what I think happens is that the n_tty code ends up messing up the cookie logic when the terminating CR is rigth after that 64-byte chunk.
Comment 3 Linus Torvalds 2022-02-15 22:50:20 UTC
So canon_copy_from_read_buf() is where all the cookie logic ends up being, and it looks like the problem is that the function has found the terminating character in the read buffer, but it's just after the actual data that it copies.

So 'found' ends up being true in the function, which then causes that

        clear_bit(eol, ldata->read_flags);

which "eats" the terminating character. And then at the end, it will do a "return false" to say that it's all done, which makes n_tty_read() clear the cookie. That then makes iterate_tty_read() decide it's all done, which is why it returns the 64-byte line (without the terminating character).

And because it did that 'clear_bit(eol, ..)' then next time around the terminating character won't actually terminate the read, so you get it together with the next line.

So I'm pretty sure I've narrowed the bug down to that 'found' logic. But now I need to go page in the details of what it does that is wrong.  There were lots of subtle things here (like "what if there's no EOL at all") but it's been a year+...
Comment 4 Daniel Gibson 2022-02-15 22:58:03 UTC
I think ac8f3bf8832a405cc6e4dccb1d26d5cb2994d234 is relevant, somehow the +1 in
"n = min(*nr + 1, ..." sticks out
Comment 5 Linus Torvalds 2022-02-15 23:19:26 UTC
Ahh.. I'm starting to remember.

So the bug comes directly from the fact that we're searching for EOL not in the data that we return, but in the source data - and we intentionally search one more character than we are going to return:

        n = min(*nr + 1, canon_head - ldata->read_tail);

That "canon_head - ldata->read_tail" is how much data we have in the source buffer, and that "*nr" is the size of the destination. And that "+ 1" is exactly because we're going to look for EOL one _past_ the size of the returned buffer.

And we're doing that, because we actually *do* want to eat the EOL early for one very special case: the stupid __DISABLED_CHAR case. 

And this logic is all really really old, and was trying to handle the case where the user buffer was exactly the same size as a line with a EOL that should be ignored at the end, and we didn't have the cookie logic and the retry. With the new kernel buffer model and the continuation cookie, I don't think we need any of that complexity at all, and the fact that I carried it over from the old code was a mistake.

So I think I have a plan. Will test a patch and send it out if it looks successful.
Comment 6 Linus Torvalds 2022-02-15 23:25:07 UTC
(In reply to Daniel Gibson from comment #4)
> I think ac8f3bf8832a405cc6e4dccb1d26d5cb2994d234 is relevant, somehow the +1
> in
> "n = min(*nr + 1, ..." sticks out

Yes. I literally think that the fix is to just remove the "+1".

Now, the patch I'm about to boot into does a bit more than that, because once you remove the "+1", the later

        c = min(*nr, c);

is pointless (because 'c' can no longer be larger than '*nr'), but yes, that odd "scan past the end" is the cause of the bug.

And I think that the condition that commit ac8f3bf8832a ("n_tty: Fix poll() after buffer-limited eof push read") tried to fix back in 2015 is no longer relevant, because we don't have that eof_push thing.

But yeah, this area has been full of odd off-by-one issues for a long time.
Comment 7 Linus Torvalds 2022-02-15 23:37:31 UTC
Created attachment 300471 [details]
Possible patch

So this is literally that "just remove the +1, and then the now unnecessary min()" patch.

I think it fixes the problem.

I think that the issue that Peter Hurley tried to fix in ac8f3bf8832a ("n_tty: Fix poll() after buffer-limited eof push read") isn't relevant any more, but I actually do worry that the commit that that commit fixed - 40d5e0905a03 ("n_tty: Fix EOF push handling") might be relevant now.

That said, I think the "return 0" case can only happen when the EOL character is also __DISABLED_CHAR. I can't seem to find it in myself to care about such a broken setup.
Comment 8 Daniel Gibson 2022-02-16 00:02:30 UTC
Thank you very much for this spectacularly quick fix!

As far as I can tell it indeed fixes the issue, both in the simple test program (of course) and in Eclipse CDT's gdb frontend when debugging dhewm3, where I originally encountered the bug.
Comment 9 Linus Torvalds 2022-02-16 19:13:22 UTC
(In reply to Daniel Gibson from comment #8)
> As far as I can tell it indeed fixes the issue

Ok, I've committed that fix as 359303076163 ("tty: n_tty: do not look ahead for EOL character past the end of the buffer").

I'm not entirely happy about this: it re-introduces the odd "EOF at exactly the user buffer boundary" case, but I'm not 100% convinced that case isn't really valid to begin with. It's a fundamental ambiguity: what to do if the tty EOF marker was at the exact boundary of a user read().

What Peter Hurley's old patches from 2013 and 2015 tried to do was to basically not ever return a "real" EOF (is returning zero from the read() system call), by silently eating the EOF when it coincided with the end of the user space buffer.

And my "don't look ahead" thus basically removes that silent eating of the EOF character, and thus the next read() system call will see it at the beginning of the buffer, and return zero.

I think that behavior is equally valid to the silent eating, and that the situation is simply ambiguous. But I do want to point out that this is an actual semantic issue.

I know where to fix that if we turn out to care - handle it as a special case of "continuation read with no more room in the user space buffer" in n_tty_read(). But I didn't actually add that code.
Comment 10 Daniel Gibson 2022-03-29 23:47:56 UTC
Created attachment 300646 [details]
More extensive test program that checks this and similar bugs

I just uploaded an updated version of the test program that tests several edge cases of iCANON PTYs, including the original bug (64chars + EOL) and lines that have the same length as the read buffer (+/-1 character) - and all that both with regular newlines ('\n') and EOF characters (aka EOT - '\4').

With the current Linux Kernel the only bug it finds is the "silently eating the EOF when it coincided with the end of the user space buffer" edge case that Linus mentioned - instead of silently eating the EOF, the next read() returns 0.
read() returning 0 isn't even wrong, strictly speaking - FreeBSD and OSX do the same - but the old behavior is also defensible (Solaris also does it and it's probably best to stick to the behavior Linux has shown since 2013 or so).
Apart from being the old behavior, silently eating the EOF might be less confusing to readers that assume that read() returning 0 means EOF (which it generally does!) and that might then interpret it as "probably the other end has disconnected" and stop reading.

(With older kernels it also detects the 64char + NL line bug and the read_buffer_size == line_length bug)

I'll post a patch that restores the old behavior (without breaking the other edge cases) to the LKML soon.

I'm also planning to eventually port this standalone test program to kselftest so it can be added to tools/testing/selftests/

Note You need to log in before you can comment on or make changes to this bug.