Bug 215611

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.

(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.

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+...

I think ac8f3bf8832a405cc6e4dccb1d26d5cb2994d234 is relevant, somehow the +1 in
"n = min(*nr + 1, ..." sticks out

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.

(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.

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.

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.

(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.

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/