Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Margarita Manterola]

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

Hi,

The problem:
Large pastes (5k or more) into a readline enabled program fail when
running kernels larger than v2.6.31-rc5.  "Fail" means that some lines
are incomplete.  From v2.6.39-rc1 onwards, "some lines" become "almost
all lines after the first 4k".  This turns up at least in Fedora,
Debian, Ubuntu and Gentoo.  From our findings, it should happen in any
readline enabled program on a system running kernels later than the
mentioned ones.

The problematic commits in the kernel tree:
1 - 2009-07-27 (never shipped) -
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3a54297478e6578f96fd54bf4daa1751130aca86

After this commit, pastes start breaking.  For a 35k file, about 50%
of the times one or two lines are partially incomplete.

2 - 2009-07-29 (v2.6.31-rc5) -
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e043e42bdb66885b3ac10d27a01ccb9972e2b0a3

This commit reverts the previous one, but adds one extra call to
flush_to_ldisc.  Pastes still break, commenting out the function call
prevents breakage *up to 2.6.39-rc1*.

3 - 2011-03-22 (v2.6.39-rc1) -
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=f23eb2b2b28547fc70df82dd5049eb39bec5ba12

This commit changes many schedule/flush/cancel_delayed_work calls into
schedule/flush/cancel_work.  After this commit, the big breakage
starts: for the 35k example file, it starts breaking at aprox. 4k and
then every line is partially incomplete or directly not there.

Still after this commit, commenting out the tty_flush_to_ldisc(tty)
call added by e043e42bdb66885b3ac10d27a01ccb9972e2b0a3 prevents the
breakage.

4 - 2011-04-04 (v2.6.39-rc2) -
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a5660b41af6a28f8004e70eb261e1202ad55c5e3

This commit modifies the behaviour of how the ttys are polled.  After
this commit, commenting out the tty_flush_to_ldisc(tty) call doesn't
prevent breakage anymore.

But then re-adding the call to schedule_work(&tty->buf.work) that was
removed in this commit, prevents the breakage even up to 3.11-rc2.
I'm attaching a diff of the patch that we applied, just to show what
had to be done, this is not a proposed fix, because this does cause a
busy loop that is particularly noticeable in VMs.

We haven't yet found a good fix for this issue, but we believe that
pasting into readline enabled programs shouldn't cause characters to
get lost, and it should be possible to do that properly without the
busy loop.

***
This was originally reported as a bug in readline, but it was found
that going back to very old kernels prevented the issue, regardless of
the version of readline.

Original Report (2012-06-25):
http://lists.gnu.org/archive/html/bug-readline/2012-06/msg00006.html
Follow Up thread (2013-07-22):
http://lists.gnu.org/archive/html/bug-readline/2013-07/msg00006.html

I'm attaching here a very simple readline enabled program that helps
with performing tests.  Compile, run, then copy and paste a large
enough file into it, close and diff.

Looking at the code in readline, the issue is triggered by these lines
in rltty.c, while preparing the input:

tiop->c_lflag &= ~(ICANON | ECHO);
(...)
tiop->c_iflag &= ~(ICRNL | INLCR);

If those two lines are replaced by:

tiop->c_lflag &= ~(ECHO);
(...)
tiop->c_iflag &= ~(INLCR);

Then the pastes work fine: no lines are missing.  Of course, this
means that readline doesn't work properly, but this is just to note
that those are the terminal settings that cause the issue to pop-up.

Credit: this investigation was done together with Maximiliano Curia.

-- 
Regards,
Margarita Manterola

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: minirl.c --]
[-- Type: text/x-csrc; name="minirl.c", Size: 862 bytes --]

/* mini program to demonstrate problem with cut and paste,  Frank Lübeck */
/* Simplified by Margarita Manterola. */
/* Compile:
 *    gcc -Wall -o minirl -O2 minirl.c -lreadline
 *    gcc -Wall -o minirl -O2 minirl.c $CPPFLAGS $LDFLAGS -lreadline -lncurses
 * Run:
 *    ./minirl TESTOUT
 * and paste the content of any text file (say 10kB without long lines),
 * end program with Ctrl-d.
 *
 * Then
 *    diff original-file TESTOUT
 * shows that in some lines trailing characters get lost. Or lines completely
 * lost.
*/

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <readline/readline.h>

int main(int argc, char** argv) {

  char* s;
  FILE *out;

  if (argc == 1) return 1;
  out = fopen(argv[1], "w");
  while (1) {
    s = readline("> ");
    if (!s) break;
    fprintf(out,"%s\n",s);
    free(s);
  }
  fflush(out);
  return 0;
}

[-- Attachment #3: prevent_readline_paste_breakage.diff --]
[-- Type: application/octet-stream, Size: 1240 bytes --]

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 4bf0fc0..6d4b647 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1667,7 +1667,7 @@ static inline int input_available_p(struct tty_struct
*tty, int amt)
 {
        struct n_tty_data *ldata = tty->disc_data;

-       tty_flush_to_ldisc(tty);
+       // tty_flush_to_ldisc(tty);
        if (ldata->icanon && !L_EXTPROC(tty)) {
                if (ldata->canon_data)
                        return 1;
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 9121c1f..6c2da95 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -449,8 +449,10 @@ static void flush_to_ldisc(struct work_struct *work)
                                tty_buffer_free(port, head);
                                continue;
                        }
-                       if (!tty->receive_room)
+                       if (!tty->receive_room) {
+                                schedule_work(&buf->work);
                                break;
+                        }
                        if (count > tty->receive_room)
                                count = tty->receive_room;
                        char_buf = head->char_buf_ptr + head->read;

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Peter Hurley]

On 07/25/2013 07:29 AM, Margarita Manterola wrote:
> Hi,
>
> The problem:
> Large pastes (5k or more) into a readline enabled program fail when
> running kernels larger than v2.6.31-rc5.  "Fail" means that some lines
> are incomplete.  From v2.6.39-rc1 onwards, "some lines" become "almost
> all lines after the first 4k".  This turns up at least in Fedora,
> Debian, Ubuntu and Gentoo.  From our findings, it should happen in any
> readline enabled program on a system running kernels later than the
> mentioned ones.
>
> The problematic commits in the kernel tree:
> 1 - 2009-07-27 (never shipped) -
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3a54297478e6578f96fd54bf4daa1751130aca86
>
> After this commit, pastes start breaking.  For a 35k file, about 50%
> of the times one or two lines are partially incomplete.
>
> 2 - 2009-07-29 (v2.6.31-rc5) -
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e043e42bdb66885b3ac10d27a01ccb9972e2b0a3
>
> This commit reverts the previous one, but adds one extra call to
> flush_to_ldisc.  Pastes still break, commenting out the function call
> prevents breakage *up to 2.6.39-rc1*.
>
> 3 - 2011-03-22 (v2.6.39-rc1) -
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=f23eb2b2b28547fc70df82dd5049eb39bec5ba12
>
> This commit changes many schedule/flush/cancel_delayed_work calls into
> schedule/flush/cancel_work.  After this commit, the big breakage
> starts: for the 35k example file, it starts breaking at aprox. 4k and
> then every line is partially incomplete or directly not there.
>
> Still after this commit, commenting out the tty_flush_to_ldisc(tty)
> call added by e043e42bdb66885b3ac10d27a01ccb9972e2b0a3 prevents the
> breakage.
>
> 4 - 2011-04-04 (v2.6.39-rc2) -
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a5660b41af6a28f8004e70eb261e1202ad55c5e3
>
> This commit modifies the behaviour of how the ttys are polled.  After
> this commit, commenting out the tty_flush_to_ldisc(tty) call doesn't
> prevent breakage anymore.
>
> But then re-adding the call to schedule_work(&tty->buf.work) that was
> removed in this commit, prevents the breakage even up to 3.11-rc2.
> I'm attaching a diff of the patch that we applied, just to show what
> had to be done, this is not a proposed fix, because this does cause a
> busy loop that is particularly noticeable in VMs.
>
> We haven't yet found a good fix for this issue, but we believe that
> pasting into readline enabled programs shouldn't cause characters to
> get lost, and it should be possible to do that properly without the
> busy loop.
>
> ***
> This was originally reported as a bug in readline, but it was found
> that going back to very old kernels prevented the issue, regardless of
> the version of readline.
>
> Original Report (2012-06-25):
> http://lists.gnu.org/archive/html/bug-readline/2012-06/msg00006.html
> Follow Up thread (2013-07-22):
> http://lists.gnu.org/archive/html/bug-readline/2013-07/msg00006.html
>
> I'm attaching here a very simple readline enabled program that helps
> with performing tests.  Compile, run, then copy and paste a large
> enough file into it, close and diff.
>
> Looking at the code in readline, the issue is triggered by these lines
> in rltty.c, while preparing the input:
>
> tiop->c_lflag &= ~(ICANON | ECHO);
> (...)
> tiop->c_iflag &= ~(ICRNL | INLCR);
>
> If those two lines are replaced by:
>
> tiop->c_lflag &= ~(ECHO);
> (...)
> tiop->c_iflag &= ~(INLCR);
>
> Then the pastes work fine: no lines are missing.  Of course, this
> means that readline doesn't work properly, but this is just to note
> that those are the terminal settings that cause the issue to pop-up.
>
> Credit: this investigation was done together with Maximiliano Curia.
>

readline is fundamentally incompatible with an active writer.

readline() saves and restores the termios settings for each input
line it reads. However, tty i/o is asynchronous in the kernel.
This means that when readline() restores the original termios
settings, any new data received by the tty will be interpreted
with the current, original termios settings.

When a large paste happens, the tty/line discipline read buffer
quickly fills up (to 4k). When full, further input is forced to
wait. After readline() reads an input line, more space becomes
available in the read buffer. Unfortunately, this event roughly
coincides with restoring the original termios settings, and
thus increases the probability that more paste data will be
received with the wrong termios settings.

That's why the patches that involve scheduling the receive
buffer work seem to have some effect on the outcome.

As you've already noted, readline() termios settings are
substantially different than the default termios settings.

Below I've included a simple test jig that
  1) sets termios to the same settings as readline()
  2) uses the same read() method as readline()
  3) outputs what it reads to stdout
  4) restores the original termios

Note that the test jig differs from readline() in that
it reads _all_ the available input without changing
termios.

The test jig will identically duplicate a large paste. Feel
free to modify the test jig to change the termios settings
per-line.

Regards,
Peter Hurley


[ NB: In the test jig, I didn't bother with ensuring eof is
the first input of a new line.

I converted CR -> NL because that's what your test
program does, in conjunction with readline(). Pasted lines
are CR-terminated; readline() returns the input line less the
CR; your test program prints the line followed by NL.

]

--- >% ---
#include <stdio.h>
#include <termios.h>
#include <unistd.h>
#include <stdlib.h>

int main(int argc, char* argv[]) {

	int c, eof;
         ssize_t actual;
	struct termios termios, save;
	int err;

	err = tcgetattr(STDIN_FILENO, &termios);
	if (err < 0)
		exit(EXIT_FAILURE);

	save = termios;

	termios.c_lflag &= ~(ICANON | ECHO | ISIG);
	termios.c_iflag &= ~(IXON | IXOFF);
	if ((termios.c_cflag & CSIZE) == CS8)
		termios.c_iflag &= ~(ISTRIP | INPCK);
	termios.c_iflag &= ~(ICRNL | INLCR);
	termios.c_cc[VMIN] = 1;
	termios.c_cc[VTIME] = 0;
	termios.c_cc[VLNEXT] = _POSIX_VDISABLE;
	eof = termios.c_cc[VEOF];

	err = tcsetattr(STDIN_FILENO, TCSANOW, &termios);
	if (err < 0)
		exit(EXIT_FAILURE);

	while (1) {
		actual = read( fileno(stdin), &c, sizeof(unsigned char));
		if (actual <= 0)
			break;
		if (actual == sizeof(unsigned char)) {
			if (c == eof)
				break;
			if (c == '\r')
				c = '\n';
			fputc(c, stdout);
		}
		fflush(stdout);
	}

	err = tcsetattr(STDIN_FILENO, TCSAFLUSH, &save);
	if (err < 0)
		exit(EXIT_FAILURE);

	return 0;
}

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Maximiliano Curia]

Peter Hurley wrote:

> readline is fundamentally incompatible with an active writer.

This wasn't the case with older kernel versions. I don't see any POSIX 
reference that claims user input could be lost setting termios so I think 
this is a serious regression.

Also, consider the readline use cases. bash, for instance, uses readline to 
process the command lines entered, but needs to change the termios to a 
canonical mode for each entered command. I would expect that pasting a 
sequence of commands (of 4K, which is not even 'a lot') to work.

The same is true for psql, where users might paste several KB of queries, or 
almost every readline enabled "shell".

> readline() saves and restores the termios settings for each input
> line it reads. However, tty i/o is asynchronous in the kernel.
> This means that when readline() restores the original termios
> settings, any new data received by the tty will be interpreted
> with the current, original termios settings.

> When a large paste happens, the tty/line discipline read buffer
> quickly fills up (to 4k). When full, further input is forced to
> wait. After readline() reads an input line, more space becomes
> available in the read buffer. Unfortunately, this event roughly
> coincides with restoring the original termios settings, and
> thus increases the probability that more paste data will be
> received with the wrong termios settings.

> That's why the patches that involve scheduling the receive
> buffer work seem to have some effect on the outcome.

It's not totally clear to me why receiving characters with the wrong termios 
settings might lead to this characters being dropped when reading them with 
different settings.

I took a deep look into the code, trying to find where was the code that ended 
up dropping characters, but could not find it.
Could you maybe point me to it?

> As you've already noted, readline() termios settings are
> substantially different than the default termios settings.
> 
> Below I've included a simple test jig that
>   1) sets termios to the same settings as readline()
>   2) uses the same read() method as readline()
>   3) outputs what it reads to stdout
>   4) restores the original termios

I've updated your code the be closer to the readline behaviour. readline 
calls tcsetattr with TCSADRAIN, and not TCSAFLUSH which explictly claims to 
discard the input. I've also reordered the call to process lines, and 
initialized the int c.

--- >% ---
#include <stdio.h>
#include <termios.h>
#include <unistd.h>
#include <stdlib.h>

void init(int *eof, struct termios* save)
{
    int err;
    static struct termios termios;

    err = tcgetattr(STDIN_FILENO, &termios);
    if (err < 0)
        exit(EXIT_FAILURE);
    *save = termios;

    termios.c_lflag &= ~(ICANON | ECHO | ISIG);
    termios.c_iflag &= ~(IXON | IXOFF);
    if ((termios.c_cflag & CSIZE) == CS8)
        termios.c_iflag &= ~(ISTRIP | INPCK);
    termios.c_iflag &= ~(ICRNL | INLCR);
    termios.c_cc[VMIN] = 1;
    termios.c_cc[VTIME] = 0;
    termios.c_cc[VLNEXT] = _POSIX_VDISABLE;
    *eof = termios.c_cc[VEOF];

    err = tcsetattr(STDIN_FILENO, TCSADRAIN, &termios);
    if (err < 0)
        exit(EXIT_FAILURE);
}

void deinit(struct termios* termios)
{
    int err;
    err = tcsetattr(STDIN_FILENO, TCSADRAIN, termios);
    if (err < 0)
        exit(EXIT_FAILURE);
}

int main(int argc, char* argv[])
{
    int c=0, eof;
    ssize_t actual;
    struct termios save;

    while (1) {
        init(&eof, &save);
        while (1) {
            actual = read(fileno(stdin), &c, sizeof(unsigned char));
            if (actual <= 0)
                break;
            if (actual == sizeof(unsigned char)) {
                if (c == eof)
                    break;
                if (c == '\r') {
                    c = '\n';
                }
                fputc(c, stdout);
                fflush(stdout);
                if (c == '\n') break;
            }
        }
        deinit(&save);
        if (c == eof) break;
    }

    return 0;
}
--- >% ---

-- 
"Seek simplicity, and distrust it." -- Whitehead's Rule
Saludos /\/\ /\ >< `/

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Peter Hurley]

Please don't drop CCs.

[ +cc Margarita Manterola, Greg Kroah-Hartman, Jiri Slaby, linux-kernel ]

On 07/30/2013 08:41 AM, Maximiliano Curia wrote:
> Peter Hurley wrote:
>
>> readline is fundamentally incompatible with an active writer.
>
> This wasn't the case with older kernel versions. I don't see any POSIX
> reference that claims user input could be lost setting termios so I think
> this is a serious regression.

I'm only interested in this discussion on the basis of a kernel
regression, because this behavior is fully POSIX compliant.

 From POSIX 2008, Section 11. General Terminal Interface, Part 11.1.6.
Canonical Mode Input Processing:

      In canonical mode input processing, terminal input is processed in
      units of lines. A line is delimited by a <newline> character (NL),
      an end-of-file character (EOF), or an end-of-line (EOL) character.
      See Special Characters for more information on EOF and EOL. This
      means that a read request shall not return until an entire line has
      been typed or a signal has been received. Also, no matter how many
      bytes are requested in the read() call, at most one line shall be
      returned. It is not, however, necessary to read a whole line at once;
      any number of bytes, even one, may be requested in a read() without
      losing information.

      If {MAX_CANON} is defined for this terminal device, it shall be a
      limit on the number of bytes in a line. The behavior of the system
      when this limit is exceeded is implementation-defined.


In other words, while processing input in canonical mode, the system may
discard input in excess of MAX_CANON until a NL character is received.
Linux defines MAX_CANON as 255.


> Also, consider the readline use cases. bash, for instance, uses readline to
> process the command lines entered, but needs to change the termios to a
> canonical mode for each entered command. I would expect that pasting a
> sequence of commands (of 4K, which is not even 'a lot') to work.
>
> The same is true for psql, where users might paste several KB of queries, or
> almost every readline enabled "shell".
>
>> readline() saves and restores the termios settings for each input
>> line it reads. However, tty i/o is asynchronous in the kernel.
>> This means that when readline() restores the original termios
>> settings, any new data received by the tty will be interpreted
>> with the current, original termios settings.
>
>> When a large paste happens, the tty/line discipline read buffer
>> quickly fills up (to 4k). When full, further input is forced to
>> wait. After readline() reads an input line, more space becomes
>> available in the read buffer. Unfortunately, this event roughly
>> coincides with restoring the original termios settings, and
>> thus increases the probability that more paste data will be
>> received with the wrong termios settings.
>
>> That's why the patches that involve scheduling the receive
>> buffer work seem to have some effect on the outcome.
>
> It's not totally clear to me why receiving characters with the wrong termios
> settings might lead to this characters being dropped when reading them with
> different settings.

Many termios settings are applied when the input is received rather than when
the application reads them.

Here's an example sequence:

1. termios is set to non-canonical mode
2. the input buffer is filled with paste data (although unknown to the kernel
    there is more paste data which has not yet been received). The paste data
    contains CR as line delimiter.
3. the input buffer is read character-by-character until the first CR is found.
4. termios is set to canonical mode but ICRNL is not set.
5. the writer thread is woken because more space has become available in the
    input buffer.
6. more paste data is received in canonical mode but a NL cannot be found
    (because there are only CRs in the paste data).
7. the system discards this input to prevent already received data from being
    overrun and continues to do so until a NL is found (or termios is set back
    to non-canonical mode).


> I took a deep look into the code, trying to find where was the code that ended
> up dropping characters, but could not find it.
> Could you maybe point me to it?

n_tty_set_room() in drivers/tty/n_tty.c (3.10 mainline)

 From n_tty_set_room():

	/*
	 * If we are doing input canonicalization, and there are no
	 * pending newlines, let characters through without limit, so
	 * that erase characters will be handled.  Other excess
	 * characters will be beeped.
	 */
	if (left <= 0)
		left = ldata->icanon && !ldata->canon_data;
	old_left = tty->receive_room;
	tty->receive_room = left;


The code means that if the input buffer is full and ICANON is set and
a NL has not been received yet, still report that space for 1 char
is available in the input buffer.

When the character is actually received and interpreted, if there is
no space in the input buffer, the character is dropped. If ECHO is set,
a '\a' (bell) is written to the tty's output.


>> As you've already noted, readline() termios settings are
>> substantially different than the default termios settings.
>>
>> Below I've included a simple test jig that
>>    1) sets termios to the same settings as readline()
>>    2) uses the same read() method as readline()
>>    3) outputs what it reads to stdout
>>    4) restores the original termios
>
> I've updated your code the be closer to the readline behaviour.

That's why I wrote the test jig: to show the paste data was
received error-free with _exactly_ the same termios settings
that readline() uses.

Re-introducing setting and unsetting termios simply confirms
what I already diagnosed.

Regards,
Peter Hurley


> readline calls tcsetattr with TCSADRAIN, and not TCSAFLUSH which explictly
> claims to discard the input. I've also reordered the call to process lines,
> and initialized the int c.
>
> --- >% ---
> #include <stdio.h>
> #include <termios.h>
> #include <unistd.h>
> #include <stdlib.h>
>
> void init(int *eof, struct termios* save)
> {
>      int err;
>      static struct termios termios;
>
>      err = tcgetattr(STDIN_FILENO, &termios);
>      if (err < 0)
>          exit(EXIT_FAILURE);
>      *save = termios;
>
>      termios.c_lflag &= ~(ICANON | ECHO | ISIG);
>      termios.c_iflag &= ~(IXON | IXOFF);
>      if ((termios.c_cflag & CSIZE) == CS8)
>          termios.c_iflag &= ~(ISTRIP | INPCK);
>      termios.c_iflag &= ~(ICRNL | INLCR);
>      termios.c_cc[VMIN] = 1;
>      termios.c_cc[VTIME] = 0;
>      termios.c_cc[VLNEXT] = _POSIX_VDISABLE;
>      *eof = termios.c_cc[VEOF];
>
>      err = tcsetattr(STDIN_FILENO, TCSADRAIN, &termios);
>      if (err < 0)
>          exit(EXIT_FAILURE);
> }
>
> void deinit(struct termios* termios)
> {
>      int err;
>      err = tcsetattr(STDIN_FILENO, TCSADRAIN, termios);
>      if (err < 0)
>          exit(EXIT_FAILURE);
> }
>
> int main(int argc, char* argv[])
> {
>      int c=0, eof;
>      ssize_t actual;
>      struct termios save;
>
>      while (1) {
>          init(&eof, &save);
>          while (1) {
>              actual = read(fileno(stdin), &c, sizeof(unsigned char));
>              if (actual <= 0)
>                  break;
>              if (actual == sizeof(unsigned char)) {
>                  if (c == eof)
>                      break;
>                  if (c == '\r') {
>                      c = '\n';
>                  }
>                  fputc(c, stdout);
>                  fflush(stdout);
>                  if (c == '\n') break;
>              }
>          }
>          deinit(&save);
>          if (c == eof) break;
>      }
>
>      return 0;
> }
> --- >% ---
>

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Maximiliano Curia]

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

Hi,

> n_tty_set_room() in drivers/tty/n_tty.c (3.10 mainline)

> From n_tty_set_room():

> 	/*
> 	 * If we are doing input canonicalization, and there are no
> 	 * pending newlines, let characters through without limit, so
> 	 * that erase characters will be handled.  Other excess
> 	 * characters will be beeped.
> 	 */
> 	if (left <= 0)
> 		left = ldata->icanon && !ldata->canon_data;
> 	old_left = tty->receive_room;
> 	tty->receive_room = left;

I took a long look at this code and thought about how it could be made to work
for readline's case and also for the canonical readers.  I came up with this
simple patch:

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 4bf0fc0..2ba7f4e 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -149,7 +149,8 @@ static int set_room(struct tty_struct *tty)
         * characters will be beeped.
         */
        if (left <= 0)
-               left = ldata->icanon && !ldata->canon_data;
+               if (waitqueue_active(&tty->read_wait))
+                       left = ldata->icanon && !ldata->canon_data;
        old_left = tty->receive_room;
        tty->receive_room = left;

This is of course just an idea, but I tested this and it worked correctly for
the cases I was testing.

The effect of this patch is that when there is a canonical reader waiting for
input, it maintains the previous behavior, but when there's no reader (like
when readline is changing modes), it blocks and doesn't lose any characters.

Another approach would be to recalculate the size of canon_data when the mode
is changed, but this would probably be much more invasive, and awfully less
efficient since it would imply going through the buffer.

What do you think? Is the proposed solution, or something along those lines,
acceptable?

If there are other cases that need to be taken into account and that I
currently don't know about, please let me know.

-- 
"If you think your users are idiots, only idiots will use it."
-- Linus Torvalds
Saludos /\/\ /\ >< `/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Pavel Machek]

Hi!

> > n_tty_set_room() in drivers/tty/n_tty.c (3.10 mainline)
> 
> > From n_tty_set_room():
> 
> > 	/*
> > 	 * If we are doing input canonicalization, and there are no
> > 	 * pending newlines, let characters through without limit, so
> > 	 * that erase characters will be handled.  Other excess
> > 	 * characters will be beeped.
> > 	 */
> > 	if (left <= 0)
> > 		left = ldata->icanon && !ldata->canon_data;
> > 	old_left = tty->receive_room;
> > 	tty->receive_room = left;
> 
> I took a long look at this code and thought about how it could be made to work
> for readline's case and also for the canonical readers.  I came up with this
> simple patch:
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 4bf0fc0..2ba7f4e 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -149,7 +149,8 @@ static int set_room(struct tty_struct *tty)
>          * characters will be beeped.
>          */
>         if (left <= 0)
> -               left = ldata->icanon && !ldata->canon_data;
> +               if (waitqueue_active(&tty->read_wait))
> +                       left = ldata->icanon && !ldata->canon_data;
>         old_left = tty->receive_room;
>         tty->receive_room = left;
> 
> This is of course just an idea, but I tested this and it worked correctly for
> the cases I was testing.
> 
> The effect of this patch is that when there is a canonical reader waiting for
> input, it maintains the previous behavior, but when there's no reader (like
> when readline is changing modes), it blocks and doesn't lose any characters.
> 
> Another approach would be to recalculate the size of canon_data when the mode
> is changed, but this would probably be much more invasive, and awfully less
> efficient since it would imply going through the buffer.
> 
> What do you think? Is the proposed solution, or something along those lines,
> acceptable?
> 
> If there are other cases that need to be taken into account and that I
> currently don't know about, please let me know.

Was this applied? You may want to cc rjw... it is a regression, it is
not pretty, and it is something I blieve I hit but thought it was some
kind of "X weirdness".

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Margarita Manterola]

Hi,

On Sat, Aug 17, 2013 at 5:28 PM, Pavel Machek <pavel@ucw.cz> wrote:

>> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
>> index 4bf0fc0..2ba7f4e 100644
>> --- a/drivers/tty/n_tty.c
>> +++ b/drivers/tty/n_tty.c
>> @@ -149,7 +149,8 @@ static int set_room(struct tty_struct *tty)
>>          * characters will be beeped.
>>          */
>>         if (left <= 0)
>> -               left = ldata->icanon && !ldata->canon_data;
>> +               if (waitqueue_active(&tty->read_wait))
>> +                       left = ldata->icanon && !ldata->canon_data;
>>         old_left = tty->receive_room;
>>         tty->receive_room = left;

> Was this applied? You may want to cc rjw... it is a regression, it is
> not pretty, and it is something I blieve I hit but thought it was some
> kind of "X weirdness".

There were no replies to the previous mail asking for comments, and as
far as I can see this has not been applied. I don't know who rjw is,
could you be a bit more explicit, please?

-- 
Besos,
Marga

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Geert Uytterhoeven]

On Sun, Aug 18, 2013 at 12:57 AM, Margarita Manterola
<margamanterola@gmail.com> wrote:
> There were no replies to the previous mail asking for comments, and as
> far as I can see this has not been applied. I don't know who rjw is,
> could you be a bit more explicit, please?

"git grep rjw MAINTAINERS"

Rafael J. Wysocki <rjw@sisk.pl>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Arkadiusz Miskiewicz]

On Sunday 18 of August 2013, Margarita Manterola wrote:
> Hi,
> 
> On Sat, Aug 17, 2013 at 5:28 PM, Pavel Machek <pavel@ucw.cz> wrote:
> >> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> >> index 4bf0fc0..2ba7f4e 100644
> >> --- a/drivers/tty/n_tty.c
> >> +++ b/drivers/tty/n_tty.c
> >> @@ -149,7 +149,8 @@ static int set_room(struct tty_struct *tty)
> >> 
> >>          * characters will be beeped.
> >>          */
> >>         
> >>         if (left <= 0)
> >> 
> >> -               left = ldata->icanon && !ldata->canon_data;
> >> +               if (waitqueue_active(&tty->read_wait))
> >> +                       left = ldata->icanon && !ldata->canon_data;
> >> 
> >>         old_left = tty->receive_room;
> >>         tty->receive_room = left;
> > 
> > Was this applied? You may want to cc rjw... it is a regression, it is
> > not pretty, and it is something I blieve I hit but thought it was some
> > kind of "X weirdness".
> 
> There were no replies to the previous mail asking for comments, and as
> far as I can see this has not been applied. I don't know who rjw is,
> could you be a bit more explicit, please?

Hi.

Was there some kind of continuation of this thread or the thing died 
completly?

-- 
Arkadiusz Miśkiewicz, arekm / maven.pl

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Arkadiusz Miskiewicz]

On Tuesday 03 of September 2013, Arkadiusz Miskiewicz wrote:
> On Sunday 18 of August 2013, Margarita Manterola wrote:
> > Hi,
> > 
> > On Sat, Aug 17, 2013 at 5:28 PM, Pavel Machek <pavel@ucw.cz> wrote:
> > >> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> > >> index 4bf0fc0..2ba7f4e 100644
> > >> --- a/drivers/tty/n_tty.c
> > >> +++ b/drivers/tty/n_tty.c
> > >> @@ -149,7 +149,8 @@ static int set_room(struct tty_struct *tty)
> > >> 
> > >>          * characters will be beeped.
> > >>          */
> > >>         
> > >>         if (left <= 0)
> > >> 
> > >> -               left = ldata->icanon && !ldata->canon_data;
> > >> +               if (waitqueue_active(&tty->read_wait))
> > >> +                       left = ldata->icanon && !ldata->canon_data;
> > >> 
> > >>         old_left = tty->receive_room;
> > >>         tty->receive_room = left;
> > > 
> > > Was this applied? You may want to cc rjw... it is a regression, it is
> > > not pretty, and it is something I blieve I hit but thought it was some
> > > kind of "X weirdness".
> > 
> > There were no replies to the previous mail asking for comments, and as
> > far as I can see this has not been applied. I don't know who rjw is,
> > could you be a bit more explicit, please?
> 

Hi

Was just going over bug-readline and lkml archives and found no continuation 
of this. 

There was a patch proposed but didn't get applied. 
https://lkml.org/lkml/2013/8/17/31
Maybe it only needs proper patch submission?

Looking from bug-readline archives debugging it took huge amount of work and 
would be sad that all that ended up being wasted.

Whole thread if someone lost it https://lkml.org/lkml/2013/7/25/205

-- 
Arkadiusz Miśkiewicz, arekm / maven.pl

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Maximiliano Curia]

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

¡Hola Arkadiusz!

El 2013-10-24 a las 18:00 +0200, Arkadiusz Miskiewicz escribió:
> Was just going over bug-readline and lkml archives and found no continuation
> of this.

> There was a patch proposed but didn't get applied.
> https://lkml.org/lkml/2013/8/17/31
> Maybe it only needs proper patch submission?

The latest patch is: https://lkml.org/lkml/2013/9/3/539

And the latest reply is: https://lkml.org/lkml/2013/9/11/825

Where Peter Hurley suggests that the patch needs a complete and detailed
analysis, but sadly I'm still not sure how to provide it. If anybody is up for
the task, by all means, go ahead.

The patch seems to be applied in the ubuntu kernel without any known issues so
far.

> Looking from bug-readline archives debugging it took huge amount of work and
> would be sad that all that ended up being wasted.

True, but I somehow agree with Peter in that a known bug is better than
applying a fix with unknown consequences, even though I think that
this particular patch won't break anything.

Happy hacking,
-- 
"If you have too many special cases, you are doing it wrong." -- Craig Zarouni
Saludos /\/\ /\ >< `/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Peter Hurley]

On 10/29/2013 09:50 AM, Maximiliano Curia wrote:
> ¡Hola Arkadiusz!
>
> El 2013-10-24 a las 18:00 +0200, Arkadiusz Miskiewicz escribió:
>> Was just going over bug-readline and lkml archives and found no continuation
>> of this.
>
>> There was a patch proposed but didn't get applied.
>> https://lkml.org/lkml/2013/8/17/31
>> Maybe it only needs proper patch submission?
>
> The latest patch is: https://lkml.org/lkml/2013/9/3/539
>
> And the latest reply is: https://lkml.org/lkml/2013/9/11/825
>
> Where Peter Hurley suggests that the patch needs a complete and detailed
> analysis, but sadly I'm still not sure how to provide it. If anybody is up for
> the task, by all means, go ahead.

The analysis is on my TODO list. I'll include it with a proper patch, this or
next weekend.

Regards,
Peter Hurley

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Pavel Machek]

Hi!

On Wed 2013-10-30 07:21:13, Peter Hurley wrote:
> On 10/29/2013 09:50 AM, Maximiliano Curia wrote:
> >??Hola Arkadiusz!
> >
> >El 2013-10-24 a las 18:00 +0200, Arkadiusz Miskiewicz escribió:
> >>Was just going over bug-readline and lkml archives and found no continuation
> >>of this.
> >
> >>There was a patch proposed but didn't get applied.
> >>https://lkml.org/lkml/2013/8/17/31
> >>Maybe it only needs proper patch submission?
> >
> >The latest patch is: https://lkml.org/lkml/2013/9/3/539
> >
> >And the latest reply is: https://lkml.org/lkml/2013/9/11/825
> >
> >Where Peter Hurley suggests that the patch needs a complete and detailed
> >analysis, but sadly I'm still not sure how to provide it. If anybody is up for
> >the task, by all means, go ahead.
> 
> The analysis is on my TODO list. I'll include it with a proper patch, this or
> next weekend.

Any news?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Margarita Manterola]

HI,

On Sun, Nov 17, 2013 at 7:29 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Any news?

I'm also not capable of doing the analysis requested, but I want to
add a link for the the fact that Canonical has applied the patch to
the kernels shipped in Ubuntu, and up to now there have been no
reports of problems:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1208740

It was applied in Saucy on Sept. 11th. Ported to the previous Ubuntu
versions on Oct. 24th through the "proposed" mechanism (only reaches a
portion of the users).  Released in the official kernels on Nov. 8th.

Since a few days, then, everyone running an updated Ubuntu system,
regardless of the specific version, should have this patch.

-- 
Besos,
Marga

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Peter Hurley]

On 11/17/2013 01:29 PM, Pavel Machek wrote:
> Hi!
>
> On Wed 2013-10-30 07:21:13, Peter Hurley wrote:
>> On 10/29/2013 09:50 AM, Maximiliano Curia wrote:
>>> ??Hola Arkadiusz!
>>>
>>> El 2013-10-24 a las 18:00 +0200, Arkadiusz Miskiewicz escribió:
>>>> Was just going over bug-readline and lkml archives and found no continuation
>>>> of this.
>>>
>>>> There was a patch proposed but didn't get applied.
>>>> https://lkml.org/lkml/2013/8/17/31
>>>> Maybe it only needs proper patch submission?
>>>
>>> The latest patch is: https://lkml.org/lkml/2013/9/3/539
>>>
>>> And the latest reply is: https://lkml.org/lkml/2013/9/11/825
>>>
>>> Where Peter Hurley suggests that the patch needs a complete and detailed
>>> analysis, but sadly I'm still not sure how to provide it. If anybody is up for
>>> the task, by all means, go ahead.
>>
>> The analysis is on my TODO list. I'll include it with a proper patch, this or
>> next weekend.
>
> Any news?
> 									Pavel
>

A quick overview of the problem and proposed solution:

Larger than 4k pastes immediately fill the line discipline buffer
and trigger an error recovery path which discards input. The error
recovery path exists to avoid wedging userspace when the line
discipline buffer is full but no newline has been received. Without
the error recovery, a canonical (line-by-line) reader will _never_
receive more input when the line discipline buffer is full because,
since no more input will be accepted, no pending newline will
be received, which is a pre-condition for a canonical reader to
read input.

readline() can inadvertently trigger the error recovery path
with pastes larger than 4k because it changes the termios to non-canonical
mode to perform its read() and restores the canonical mode for the caller.
When canonical mode is restored, the error recovery path is triggered and
input is discarded until a newline is received.

The proposed patch below disables the error recovery path unless a blocking
reader/poll is waiting. IOW, if a blocking reader/poll is waiting and the
line discipline buffer is full, input will continue to be accepted but
discarded until a newline is received. If a blocking reader is not waiting,
the receive_buf() worker is aborted and no further input is processed.


From: Maximiliano Curia <maxy@gnuservers.com.ar>
Date: Tue, 3 Sep 2013 22:48:34 +0200
Subject: [PATCH] Only let characters through when there are active readers.

If there is an active reader, previous behavior is in place. When there is no
active reader, input is blocked until the next read call unblocks it.

This fixes a long standing issue with readline when pasting more than 4096
bytes.
---
  drivers/tty/n_tty.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 4bf0fc0..cdc3b19 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -147,9 +147,16 @@ static int set_room(struct tty_struct *tty)
  	 * pending newlines, let characters through without limit, so
  	 * that erase characters will be handled.  Other excess
  	 * characters will be beeped.
+	 * If there is no reader waiting for the input, block instead of
+	 * letting the characters through.
  	 */
  	if (left <= 0)
-		left = ldata->icanon && !ldata->canon_data;
+		if (waitqueue_active(&tty->read_wait)) {
+			left = ldata->icanon && !ldata->canon_data;
+		} else {
+			left = 0;
+		}
+
  	old_left = tty->receive_room;
  	tty->receive_room = left;

-- 

Analysis:

There are two concerns that require analysis:
1) Does the patch function as designed? and
2) Does the error recovery continue to function as designed? If not, are there
    suitable alternatives?

For the first concern; yes, the patch functions as designed (provided the
departing reader is not on the waitqueue when n_tty_set_room() is called;
currently a patch queued for 3.13 has re-ordered the exit in n_tty_read() so
that would need to be re-re-ordered).

The central concept of the proposed patch is to ensure that the error
recovery path is only evaluated when a read() is in-progress, and thus only when
the termios state corresponding with that read() has already been set.

Also, since a blocking reader will evaluate the buffer state before sleeping,
if the buffer is full, the worker will be started and the error recovery path
correctly triggered, which will in turn re-wake the reader when a newline is
received.

However, error recovery will only continue to function correctly for that
one i/o model, blocking reads.

Users of the signal-driven i/o model would become wedged when the line discipline
buffer is full because there is no thread on the read_wait queue and SIGIO will
no longer be sent.

The non-blocking i/o model also becomes susceptible to wedging, if, for example,
poll() is called _after_ the worker has discovered the buffer is full; poll()
won't see input available and the worker will not be restarted.

ioctl(FIONREAD) will also indicate no input is available; emacs uses this
in conjunction with both select() and SIGIO to determine if a read() is
even necessary.


Other possible solutions:

1) I experimented with forcing would-be readers to accept input from partially
completed lines; that is, to wakeup readers/send SIGIO if the line discipline
buffer is full and return read data as if newline had been received when it
had not. While I got that working, I couldn't really convince myself that
this wouldn't cause buffer overruns or access faults for readers not
expecting lines > 4096.

2) A variation of #1 would be to do the wakeup/send SIGIO but have the
reader discard input instead. Unfortunately, there may not be a newline
in the buffered input, which would require returning an error from
the read; I'm not sure how some apps might interpret that.

3) Simplify the whole process and let user apps get wedged; they can be
un-wedged with tcflush(TCIFLUSH/TCIOFLUSH).

4) Unwedge with a watchdog timer.

5) Something I haven't thought of (like fix console selection ioctls and
use those).

6) Fix this in userspace.


Regards,
Peter Hurley

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Peter Hurley]

On 11/21/2013 12:04 AM, Peter Hurley wrote:
> On 11/17/2013 01:29 PM, Pavel Machek wrote:
>> Hi!
>>
>> On Wed 2013-10-30 07:21:13, Peter Hurley wrote:
>>> On 10/29/2013 09:50 AM, Maximiliano Curia wrote:
>>>> ??Hola Arkadiusz!
>>>>
>>>> El 2013-10-24 a las 18:00 +0200, Arkadiusz Miskiewicz escribió:
>>>>> Was just going over bug-readline and lkml archives and found no continuation
>>>>> of this.
>>>>
>>>>> There was a patch proposed but didn't get applied.
>>>>> https://lkml.org/lkml/2013/8/17/31
>>>>> Maybe it only needs proper patch submission?
>>>>
>>>> The latest patch is: https://lkml.org/lkml/2013/9/3/539
>>>>
>>>> And the latest reply is: https://lkml.org/lkml/2013/9/11/825
>>>>
>>>> Where Peter Hurley suggests that the patch needs a complete and detailed
>>>> analysis, but sadly I'm still not sure how to provide it. If anybody is up for
>>>> the task, by all means, go ahead.
>>>
>>> The analysis is on my TODO list. I'll include it with a proper patch, this or
>>> next weekend.
>>
>> Any news?
>>                                     Pavel
>>
> Other possible solutions:

<...>
7) Rescan line discipline buffer when changing from non-canonical to canonical
mode. The real problem with this approach (besides the inefficiency) is that this
solution could break some (admittedly unknown) program that contrived to exchange
data in non-canonical mode but read in canonical mode (just not exceeding the
line discipline buffer limit).

8) Don't restart the input processing worker with the termios change, but rather
wait for the future reader (which is woken if waiting) to restart input processing
instead (if it needs to).

If signal-driven i/o is being used for this tty, then the input processing worker
is immediately restarted in the line buffer is full. This is necessary to ensure
a full line buffer does not wedge a userspace program using signal-driven i/o
exclusively (because there is no waiting reader).

Similarly, since non-blocking i/o will not have a waiting reader, poll() must
restart the input processing iff there is no input and the tty is in canonical
mode and the line buffer was full. These conditions can only exist immediately
after a termios change (since terminals always start in canonical mode, the
line buffer cannot become full and have no input available without first having
been changed to non-canonical mode and then reset back to canonical mode).

Test patch below

--- >% ---
Subject: [PATCH] n_tty: Fix buffer overruns with larger-than-4k pastes

readline() inadvertently triggers an error recovery path when
pastes larger than 4k overrun the line discipline buffer. The
error recovery path discards input when the line discipline buffer
is full and operating in canonical mode and no newline has been
received. Because readline() changes the termios to non-canonical
mode to read the line char-by-char, the line discipline buffer
can become full, and then when readline() restores termios back
to canonical mode for the caller, the now-full line discipline
buffer triggers the error recovery.

Instead of restarting the input processing worker from the
change in termios, let the waking reader restart the worker.
In this way, the termios set at the time of the actual read() will
apply if/when the worker restarts.

Because signal-driven i/o may not have an active reader to restart
the worker, the change in termios must restart the worker if
O_ASYNC flag was set.

Similarly, because a poll() may not be waiting at the time the
termios is changed, the worker may need to be restarted by the
poll() (if the specific conditions exist; namely, canonical mode
&& no input available && receive buffer was full).

Reported-by: Margarita Manterola <margamanterola@gmail.com>
Cc: Maximiliano Curia <maxy@gnuservers.com.ar>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
  drivers/tty/n_tty.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 0f74945..ddc0129 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1809,7 +1809,12 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old)
  		else
  			ldata->real_raw = 0;
  	}
-	n_tty_set_room(tty);
+
+	/* Only restart worker if tty is using signal-driven i/o.
+	 * Otherwise, let the reader restart the worker
+	 */
+	if (tty->fasync)
+		n_tty_set_room(tty);
  	/*
  	 * Fix tty hang when I_IXON(tty) is cleared, but the tty
  	 * been stopped by STOP_CHAR(tty) before it.
@@ -2393,6 +2398,8 @@ static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file,
  	poll_wait(file, &tty->write_wait, wait);
  	if (input_available_p(tty, TIME_CHAR(tty) ? 0 : MIN_CHAR(tty)))
  		mask |= POLLIN | POLLRDNORM;
+	else if (ldata->icanon)
+		n_tty_set_room(tty);
  	if (tty->packet && tty->link->ctrl_status)
  		mask |= POLLPRI | POLLIN | POLLRDNORM;
  	if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
-- 
1.8.1.2

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[One Thousand Gnomes]

> 7) Rescan line discipline buffer when changing from non-canonical to canonical
> mode. The real problem with this approach (besides the inefficiency) is that this
> solution could break some (admittedly unknown) program that contrived to exchange
> data in non-canonical mode but read in canonical mode (just not exceeding the
> line discipline buffer limit).

See bugzilla 55981, 55991 btw

Alan

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Peter Hurley]

On 11/23/2013 07:29 PM, One Thousand Gnomes wrote:
>> 7) Rescan line discipline buffer when changing from non-canonical to canonical
>> mode. The real problem with this approach (besides the inefficiency) is that this
>> solution could break some (admittedly unknown) program that contrived to exchange
>> data in non-canonical mode but read in canonical mode (just not exceeding the
>> line discipline buffer limit).
>
> See bugzilla 55981, 55991 btw

Thanks for the bug references, Alan.

The solution proposed in 55991 (to perform an EOF push when switching from
non-canon to canon) would further break paste to readline().

The caller to readline() may not actually perform any read() but may
simply loop, calling readline();  in this case, when readline()
switches back to non-canonical, it will eventually read the inserted '\0'.
That would be bad.

Regards,
Peter Hurley

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Peter Hurley]

On 11/24/2013 06:55 AM, Peter Hurley wrote:
> On 11/23/2013 07:29 PM, One Thousand Gnomes wrote:
>>> 7) Rescan line discipline buffer when changing from non-canonical to canonical
>>> mode. The real problem with this approach (besides the inefficiency) is that this
>>> solution could break some (admittedly unknown) program that contrived to exchange
>>> data in non-canonical mode but read in canonical mode (just not exceeding the
>>> line discipline buffer limit).
>>
>> See bugzilla 55981, 55991 btw
>
> Thanks for the bug references, Alan.
>
> The solution proposed in 55991 (to perform an EOF push when switching from
> non-canon to canon) would further break paste to readline().
>
> The caller to readline() may not actually perform any read() but may
> simply loop, calling readline();  in this case, when readline()
> switches back to non-canonical, it will eventually read the inserted '\0'.
> That would be bad.

Stas Sergeev (the reporter of kernel bug# 55991) had proposed a
solution allowing data in the read buffer to become immediately
available for read when switching to canonical mode.

With one minor change, the proposed solution appears to solve the
readline() paste overflow problem (at least, that's the result of
my testing on the test bench originally provided by Margarita
earlier in the thread).

This patch should apply cleanly to 3.13-rc1+ (or to 3.12-final+ with
'git am -C1 <patch_file_name>'.

Please test ASAP as I'd like to see this in 3.13. I'll backport it
to the stable kernels once this is in mainline.

Regards,
Peter Hurley

--- >% ---
Subject: [PATCH v2] n_tty: Fix buffer overruns with larger-than-4k pastes

readline() inadvertently triggers an error recovery path when
pastes larger than 4k overrun the line discipline buffer. The
error recovery path discards input when the line discipline buffer
is full and operating in canonical mode and no newline has been
received. Because readline() changes the termios to non-canonical
mode to read the line char-by-char, the line discipline buffer
can become full, and then when readline() restores termios back
to canonical mode for the caller, the now-full line discipline
buffer triggers the error recovery.

When changing termios from non-canon to canon mode and the read
buffer contains data, simulate an EOF push _without_ the
DISABLED_CHAR in the read buffer. canon_copy_to_read_buf()
correctly interprets this condition and will return data in the
read buffer as one line.

Importantly for the readline() problem, the termios can be
changed back to non-canonical mode without changes to the read
buffer occurring; ie., as if the previous termios change had not
happened (as long as no intervening read took place).

Patch based on original proposal and discussion here
https://bugzilla.kernel.org/show_bug.cgi?id=55991
by Stas Sergeev <stsp@users.sourceforge.net>

Reported-by: Margarita Manterola <margamanterola@gmail.com>
Cc: Maximiliano Curia <maxy@gnuservers.com.ar>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
Acked-by: Stas Sergeev <stsp@users.sourceforge.net>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 3919ced..2184d7b 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1778,7 +1778,13 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old)
 
 	if (!old || (old->c_lflag ^ tty->termios.c_lflag) & ICANON) {
 		bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE);
-		ldata->line_start = ldata->canon_head = ldata->read_tail;
+		if (!L_ICANON(tty) || !read_cnt(ldata))
+			ldata->line_start = ldata->canon_head = ldata->read_tail;
+		else {
+			set_bit((ldata->read_head - 1) & (N_TTY_BUF_SIZE - 1),
+				ldata->read_flags);
+			ldata->canon_head = ldata->read_head;
+		}
 		ldata->erasing = 0;
 		ldata->lnext = 0;
 	}
@@ -1993,6 +1999,12 @@ static int copy_from_read_buf(struct tty_struct *tty,
  *	it copies one line of input up to and including the line-delimiting
  *	character into the user-space buffer.
  *
+ *	NB: When termios is changed from non-canonical to canonical mode and
+ *	the read buffer contains data, n_tty_set_termios() simulates an EOF
+ *	push (as if C-d were input) _without_ the DISABLED_CHAR in the buffer.
+ *	This causes data already processed as input to be immediately available
+ *	as input although a newline has not been received.
+ *
  *	Called under the atomic_read_lock mutex
  *
  *	n_tty_read()/consumer path:
-- 
1.8.1.2

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Peter Hurley]

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

On 11/25/2013 08:16 PM, Peter Hurley wrote:
> On 11/24/2013 06:55 AM, Peter Hurley wrote:
>> On 11/23/2013 07:29 PM, One Thousand Gnomes wrote:
>>>> 7) Rescan line discipline buffer when changing from non-canonical to canonical
>>>> mode. The real problem with this approach (besides the inefficiency) is that this
>>>> solution could break some (admittedly unknown) program that contrived to exchange
>>>> data in non-canonical mode but read in canonical mode (just not exceeding the
>>>> line discipline buffer limit).
>>>
>>> See bugzilla 55981, 55991 btw
>>
>> Thanks for the bug references, Alan.
>>
>> The solution proposed in 55991 (to perform an EOF push when switching from
>> non-canon to canon) would further break paste to readline().
>>
>> The caller to readline() may not actually perform any read() but may
>> simply loop, calling readline();  in this case, when readline()
>> switches back to non-canonical, it will eventually read the inserted '\0'.
>> That would be bad.
>
> Stas Sergeev (the reporter of kernel bug# 55991) had proposed a
> solution allowing data in the read buffer to become immediately
> available for read when switching to canonical mode.
>
> With one minor change, the proposed solution appears to solve the
> readline() paste overflow problem (at least, that's the result of
> my testing on the test bench originally provided by Margarita
> earlier in the thread).
>
> This patch should apply cleanly to 3.13-rc1+ (or to 3.12-final+ with
> 'git am -C1 <patch_file_name>'.
>
> Please test ASAP as I'd like to see this in 3.13. I'll backport it
> to the stable kernels once this is in mainline.

Unfortunately, this patch breaks EOF push handling.

Normally, when an EOF is found not at the line start, the output
is made available to a canonical reader (without the EOF) -- this is
commonly referred to as EOF push. An EOF at the beginning of a line
forces the read to return 0 bytes read, which the caller interprets
as an end-of-file and discontinues reading.

Since this patch simulates an EOF push, an actual EOF push that
follows will appear to be an EOF at the beginning of a line and
cause read to return 0, thus indicating premature end-of-file.

I've attached a simulation testcase that shows the unexpected EOF.

I think this general approach is still the way forward with this bug
but I need to ponder how the simulated EOF push state can properly be
distinguished from the other eol conditions in canon_copy_from_read_buf()
so line_start is not reset to the read_tail.

Regards,
Peter Hurley

> --- >% ---
> Subject: [PATCH v2] n_tty: Fix buffer overruns with larger-than-4k pastes
>
> readline() inadvertently triggers an error recovery path when
> pastes larger than 4k overrun the line discipline buffer. The
> error recovery path discards input when the line discipline buffer
> is full and operating in canonical mode and no newline has been
> received. Because readline() changes the termios to non-canonical
> mode to read the line char-by-char, the line discipline buffer
> can become full, and then when readline() restores termios back
> to canonical mode for the caller, the now-full line discipline
> buffer triggers the error recovery.
>
> When changing termios from non-canon to canon mode and the read
> buffer contains data, simulate an EOF push _without_ the
> DISABLED_CHAR in the read buffer. canon_copy_to_read_buf()
> correctly interprets this condition and will return data in the
> read buffer as one line.
>
> Importantly for the readline() problem, the termios can be
> changed back to non-canonical mode without changes to the read
> buffer occurring; ie., as if the previous termios change had not
> happened (as long as no intervening read took place).
>
> Patch based on original proposal and discussion here
> https://bugzilla.kernel.org/show_bug.cgi?id=55991
> by Stas Sergeev <stsp@users.sourceforge.net>
>
> Reported-by: Margarita Manterola <margamanterola@gmail.com>
> Cc: Maximiliano Curia <maxy@gnuservers.com.ar>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
> Acked-by: Stas Sergeev <stsp@users.sourceforge.net>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>   drivers/tty/n_tty.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 3919ced..2184d7b 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -1778,7 +1778,13 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old)
>
>   	if (!old || (old->c_lflag ^ tty->termios.c_lflag) & ICANON) {
>   		bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE);
> -		ldata->line_start = ldata->canon_head = ldata->read_tail;
> +		if (!L_ICANON(tty) || !read_cnt(ldata))
> +			ldata->line_start = ldata->canon_head = ldata->read_tail;
> +		else {
> +			set_bit((ldata->read_head - 1) & (N_TTY_BUF_SIZE - 1),
> +				ldata->read_flags);
> +			ldata->canon_head = ldata->read_head;
> +		}
>   		ldata->erasing = 0;
>   		ldata->lnext = 0;
>   	}
> @@ -1993,6 +1999,12 @@ static int copy_from_read_buf(struct tty_struct *tty,
>    *	it copies one line of input up to and including the line-delimiting
>    *	character into the user-space buffer.
>    *
> + *	NB: When termios is changed from non-canonical to canonical mode and
> + *	the read buffer contains data, n_tty_set_termios() simulates an EOF
> + *	push (as if C-d were input) _without_ the DISABLED_CHAR in the buffer.
> + *	This causes data already processed as input to be immediately available
> + *	as input although a newline has not been received.
> + *
>    *	Called under the atomic_read_lock mutex
>    *
>    *	n_tty_read()/consumer path:
>


[-- Attachment #2: canon_switch2.c --]
[-- Type: text/x-csrc, Size: 3828 bytes --]

/*
 * canon_switch2.c
 *
 * Test read() after non-canon -> canon switch
 *  w/ EOL push immediately after
 *
 *    gcc -Wall -o canon_switch canon_switch.c
 *
 * Based on orginal code by Ilya Zykov <ilya@ilyx.ru>
 */

#include <stdio.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <termios.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
#include <stdarg.h>
#include <unistd.h>
#include <poll.h>
#include <sys/wait.h>

static int fd;

static void error_exit(char *f, ...) {
	va_list va;

	va_start(va, f);
	vprintf(f, va);
	if (errno)
		printf(": %s (code: %d)\n", strerror(errno), errno);
	else
		printf("\n");
	va_end(va);

	if (fd >= 0)
		close(fd);

	exit(EXIT_FAILURE);
}

static void fill_pattern(char *pattern, size_t len) {
	size_t i, n;
	const char test[] = "1234567890";

	n = strlen(test);
	for (i = 0; i <= len - n; i += n)
		strncpy(&pattern[i], test, n);
	strncpy(&pattern[i], test, len % n);
}

int main(int argc, char *argv[]) {
	int child_id;
	char pts_name[24];
	int ptn, unlock = 0;
	struct termios termios, save;

	setbuf(stdout, NULL);

	fd = open("/dev/ptmx", O_RDWR);
	if (fd < 0)
		error_exit("opening pty master");
	if (ioctl(fd, TIOCGPTN, &ptn) < 0)
		error_exit("getting pty #");
	if (ioctl(fd, TIOCSPTLCK, &unlock) < 0)
                error_exit("unlocking pty pair");
        snprintf(pts_name, sizeof(pts_name), "/dev/pts/%d", ptn);

	if (tcgetattr(fd, &termios) < 0)
		error_exit("tcgetattr");
	save = termios;
	termios.c_lflag &= ~(ICANON | ECHO | ISIG);
	termios.c_iflag &= ~(IXON | IXOFF | ICRNL | INLCR);
	if ((termios.c_cflag & CSIZE) == CS8)
		termios.c_cflag &= ~(ISTRIP | INPCK);
	termios.c_cc[VMIN] = 1;
	termios.c_cc[VTIME] = 0;
	termios.c_cc[VLNEXT] = _POSIX_VDISABLE;
	termios.c_oflag &= ~OPOST;
	if (tcsetattr(fd, TCSAFLUSH, &termios) < 0)
		error_exit("tcsetattr master");

	child_id = fork();
	switch (child_id) {
	case -1: error_exit("forking child");

	case 0:	{ /* child */
		printf("Child [%ld] on slave pty %s\n", (long)getpid(), pts_name);

		close(fd);	/* master pty no longer needed */

		fd = open(pts_name, O_RDWR);
		if (fd < 0)
			error_exit("opening pty slave");

		/* wait for master write in non-canon mode */
		{
			struct pollfd pollfds[1] = { { fd, POLLIN },
						   };
			poll(pollfds, 1, -1);
		}

		sleep(1);

		if (tcsetattr(fd, TCSANOW, &save))
			error_exit("tcsetattr restore");

		{
			char buf[8192];
			int n;

			n = read(fd, buf, sizeof(buf));
			if (n < 0)
				error_exit("slave reading");
			printf("read: %d\n", n);
			printf("%.*s", n, buf);

			n = read(fd, buf, sizeof(buf));
			if (n < 0)
				error_exit("slave reading");
			printf("read: %d\n", n);
			if (n == 0)
				error_exit("unexpected EOF");
			printf("%.*s", n, buf);

			n = read(fd, buf, sizeof(buf));
			if (n < 0)
				error_exit("slave reading");
			printf("read: %d\n", n);
			printf("%.*s", n, buf);
		}

		close(fd);
		}
		return 0;

	default: { /* parent */
		int n, id, status, c;

		char pattern[4096];
		char pattern2[] = "The quick brown fox jumped over the lazy dog.\r";

		/* Simulate an input pattern that was supposed to be
		 * all 4096 chars of pattern (which terminates in an EOF push)
		 * but was received as 4095 (because that's the maximum raw mode
		 * allows in the input buffer at once).
		 */
		fill_pattern(pattern, sizeof(pattern));
		pattern[4095] = termios.c_cc[VEOF];
		c = 4096;
		n = write(fd, pattern, c);
		if (n < 0)
			error_exit("master writing");
		printf("write: %d, wrote: %d\n", c, n);

		c = strlen(pattern2);
		n = write(fd, pattern2, c);
		if (n < 0)
			error_exit("master writing");
		printf("write: %d, wrote: %d\n", c, n);

		id = waitpid(child_id, &status, 0);
		if (id < 0 || id != child_id)
			error_exit("waiting for child");
		printf("[%ld] exited status: %d\n", (long) child_id, status);
		}
		return 0;
	}
}

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Stas Sergeev]

03.12.2013 04:18, Peter Hurley пишет:
> Unfortunately, this patch breaks EOF push handling.
>
> Normally, when an EOF is found not at the line start, the output
> is made available to a canonical reader (without the EOF) -- this is
> commonly referred to as EOF push. An EOF at the beginning of a line
> forces the read to return 0 bytes read, which the caller interprets
> as an end-of-file and discontinues reading.
>
> Since this patch simulates an EOF push, an actual EOF push that
> follows will appear to be an EOF at the beginning of a line and
> cause read to return 0, thus indicating premature end-of-file.
>
> I've attached a simulation testcase that shows the unexpected EOF.
>
> I think this general approach is still the way forward with this bug
> but I need to ponder how the simulated EOF push state can properly be
> distinguished from the other eol conditions in canon_copy_from_read_buf()
> so line_start is not reset to the read_tail.
Hi Peter, why do you think this is even a problem?
If you enable icanon and the first thing you did was
to send VEOF, then you need an EOF.
If you want to be backward-compatible, you'll likely need
to go that route, because currently it works exactly that
way, except that the read buffer is lost. Other than preserving
the read buffer, my patch was not supposed to change anything.
I already have a program (written, tested, went to customer,
used in production, oops sorry:) that switches to icanon and
sends VEOF to simulate EOF. If you change this, then the behaviour
will depend on whether the reader happened to read the data
while still in RAW mode or already in icanon mode, which will
create an unfixable race.

I think the only reliable and consistent fix would be to add ioctls
for EOF and EOL pushes. Then people will not even need to switch
back-n-forth like crazy. But as things are now, I think my patch
is conservative and safe. Do you think it can break something real,
other than the test-case? I think your test-case was made with the
particular patch in mind, but it is not compatible with the current
kernel, so it can't be "broken".

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Peter Hurley]

On 12/03/2013 04:01 AM, Stas Sergeev wrote:
> 03.12.2013 04:18, Peter Hurley пишет:
>> Unfortunately, this patch breaks EOF push handling.
>>
>> Normally, when an EOF is found not at the line start, the output
>> is made available to a canonical reader (without the EOF) -- this is
>> commonly referred to as EOF push. An EOF at the beginning of a line
>> forces the read to return 0 bytes read, which the caller interprets
>> as an end-of-file and discontinues reading.
>>
>> Since this patch simulates an EOF push, an actual EOF push that
>> follows will appear to be an EOF at the beginning of a line and
>> cause read to return 0, thus indicating premature end-of-file.
>>
>> I've attached a simulation testcase that shows the unexpected EOF.
>>
>> I think this general approach is still the way forward with this bug
>> but I need to ponder how the simulated EOF push state can properly be
>> distinguished from the other eol conditions in canon_copy_from_read_buf()
>> so line_start is not reset to the read_tail.
> Hi Peter, why do you think this is even a problem?
> If you enable icanon and the first thing you did was
> to send VEOF, then you need an EOF.
> If you want to be backward-compatible, you'll likely need
> to go that route, because currently it works exactly that
> way, except that the read buffer is lost. Other than preserving
> the read buffer, my patch was not supposed to change anything.
> I already have a program (written, tested, went to customer,
> used in production, oops sorry:) that switches to icanon and
> sends VEOF to simulate EOF. If you change this, then the behaviour
> will depend on whether the reader happened to read the data
> while still in RAW mode or already in icanon mode, which will
> create an unfixable race.
>
> I think the only reliable and consistent fix would be to add ioctls
> for EOF and EOL pushes. Then people will not even need to switch
> back-n-forth like crazy. But as things are now, I think my patch
> is conservative and safe. Do you think it can break something real,
> other than the test-case? I think your test-case was made with the
> particular patch in mind, but it is not compatible with the current
> kernel, so it can't be "broken".

Stas,

Any unit test is specifically designed to break the code under test.
This unit test does in fact break a possible input: note specifically
that the writer is not changing the termios so has no control over
the timing of when the input is received.

Also note that the test is a simulation; the patch will break any
input stream under the following conditions:
1. The writer writes an EOF-terminated buffer
2. All the input is received _except_ the EOF; this is strictly
    timing-related and not controllable.
3. The reader changes the termios from non-canon -> canon.

At that point the damage is done; the read_flags will indicate
2 EOFs and the 2nd EOF will be interpreted as end-of-file because
it will appear to begin on a new line.

That said, this problem is definitely solvable; I'm just looking
for the best way to solve it.

Consider the total brute-force approach; a shadow read_flags that
distinguishes a real EOF receive from the fake EOF push initiated
by the patch. That would work, but I'm looking for a solution more
space-efficient and simpler than a duplicate 256-byte buffer :)

Regards,
Peter Hurley

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Stas Sergeev]

03.12.2013 21:00, Peter Hurley пишет:
> Any unit test is specifically designed to break the code under test.
> This unit test does in fact break a possible input: note specifically
> that the writer is not changing the termios so has no control over
> the timing of when the input is received.
>
> Also note that the test is a simulation; the patch will break any
> input stream under the following conditions:
> 1. The writer writes an EOF-terminated buffer
> 2. All the input is received _except_ the EOF; this is strictly
>    timing-related and not controllable.
> 3. The reader changes the termios from non-canon -> canon.
>
> At that point the damage is done; the read_flags will indicate
> 2 EOFs and the 2nd EOF will be interpreted as end-of-file because
> it will appear to begin on a new line.
How is this different from the unpatched kernel?
In the unpatched kernel, if you happen on reader side
to enable icanon while n_tty received all but VEOF (is this possible at 
all?),
then the buffer will be flushed, and the remaining VEOF
will get you a nice EOF.
So, in the unpatched kernel you get EOF because the buffer
gets wiped.
On patched kernel you get EOF because of 2 consequitive marks
in read_flags.
This is intentional, for backward compatibility.
What is the problem with that, why do you call it a breakage?
Or am I misreading the scenario you describe?

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Peter Hurley]

On 12/03/2013 02:18 PM, Stas Sergeev wrote:
> 03.12.2013 21:00, Peter Hurley пишет:
>> Any unit test is specifically designed to break the code under test.
>> This unit test does in fact break a possible input: note specifically
>> that the writer is not changing the termios so has no control over
>> the timing of when the input is received.
>>
>> Also note that the test is a simulation; the patch will break any
>> input stream under the following conditions:
>> 1. The writer writes an EOF-terminated buffer
>> 2. All the input is received _except_ the EOF; this is strictly
>>    timing-related and not controllable.
>> 3. The reader changes the termios from non-canon -> canon.
>>
>> At that point the damage is done; the read_flags will indicate
>> 2 EOFs and the 2nd EOF will be interpreted as end-of-file because
>> it will appear to begin on a new line.
> How is this different from the unpatched kernel?
> In the unpatched kernel, if you happen on reader side
> to enable icanon while n_tty received all but VEOF (is this possible at all?),
> then the buffer will be flushed, and the remaining VEOF
> will get you a nice EOF.
> So, in the unpatched kernel you get EOF because the buffer
> gets wiped.

???

Testcase output from 3.12 w/o patch:

write: 4096, wrote: 4096
write: 46, wrote: 46
Child [4444] on slave pty /dev/pts/1
read: 4095
123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789
012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678
901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567
890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345read: 
46
The quick brown fox jumped over the lazy dog.

This output is correct (child had to be terminated
because there is no 3rd read data.)



Testcase output from 3.12 w/patch:

write: 4096, wrote: 4096
write: 46, wrote: 46
Child [9453] on slave pty /dev/pts/2
read: 4095
123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789
012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678
901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567
890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456
789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345read: 
0
unexpected EOF
[9453] exited status: 256


> On patched kernel you get EOF because of 2 consequitive marks
> in read_flags.
> This is intentional, for backward compatibility.
> What is the problem with that, why do you call it a breakage?
> Or am I misreading the scenario you describe?

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Stas Sergeev]

04.12.2013 03:53, Peter Hurley пишет:
> On 12/03/2013 02:18 PM, Stas Sergeev wrote:
>> 03.12.2013 21:00, Peter Hurley пишет:
>>> Any unit test is specifically designed to break the code under test.
>>> This unit test does in fact break a possible input: note specifically
>>> that the writer is not changing the termios so has no control over
>>> the timing of when the input is received.
>>>
>>> Also note that the test is a simulation; the patch will break any
>>> input stream under the following conditions:
>>> 1. The writer writes an EOF-terminated buffer
>>> 2. All the input is received _except_ the EOF; this is strictly
>>>    timing-related and not controllable.
>>> 3. The reader changes the termios from non-canon -> canon.
>>>
>>> At that point the damage is done; the read_flags will indicate
>>> 2 EOFs and the 2nd EOF will be interpreted as end-of-file because
>>> it will appear to begin on a new line.
>> How is this different from the unpatched kernel?
>> In the unpatched kernel, if you happen on reader side
>> to enable icanon while n_tty received all but VEOF (is this possible 
>> at all?),
>> then the buffer will be flushed, and the remaining VEOF
>> will get you a nice EOF.
>> So, in the unpatched kernel you get EOF because the buffer
>> gets wiped.
>
> ???
>
> Testcase output from 3.12 w/o patch:
OK, sorry, after a year of rot of my patch in bugzilla, I've
completely forgot the pre-conditions, which is that the
buffer is not discarded, just not pushed.

> Consider the total brute-force approach; a shadow read_flags that
> distinguishes a real EOF receive from the fake EOF push initiated
> by the patch.
What is to do with them?
Remove all "fake" EOFs on receiving the real one?
But that will depend on whether the reader happened to
read everything before the real one arrived.
I think that changing termios on reader side while another
process is writing, is full of surprises. For example, even in
your test-case (without the patch) the writer could expect
that the reader would receive the VEOF char in raw mode.
But the reader switced icanon and the VEOF char is not being read.
And I am sure there are other oddities to suspect, so why
the unexpected EOF is any worse?

> That would work, but I'm looking for a solution more
> space-efficient and simpler than a duplicate 256-byte buffer 
I think "fake" EOFs do not need the entire bitmap.
It is only important to remember the position where icanon was
enabled the last time I suppose, even if it was switched many
times.

[PATCH v3] n_tty: Fix buffer overruns with larger-than-4k pastes

[Peter Hurley]

readline() inadvertently triggers an error recovery path when
pastes larger than 4k overrun the line discipline buffer. The
error recovery path discards input when the line discipline buffer
is full and operating in canonical mode and no newline has been
received. Because readline() changes the termios to non-canonical
mode to read the line char-by-char, the line discipline buffer
can become full, and then when readline() restores termios back
to canonical mode for the caller, the now-full line discipline
buffer triggers the error recovery.

When changing termios from non-canon to canon mode and the read
buffer contains data, simulate an EOF push _without_ the
DISABLED_CHAR in the read buffer. canon_copy_to_read_buf()
correctly interprets this condition and will return data in the
read buffer as one line.

Importantly for the readline() problem, the termios can be
changed back to non-canonical mode without changes to the read
buffer occurring; ie., as if the previous termios change had not
happened (as long as no intervening read took place).

Patch based on original proposal and discussion here
https://bugzilla.kernel.org/show_bug.cgi?id=55991
by Stas Sergeev <stsp@users.sourceforge.net>

Reported-by: Margarita Manterola <margamanterola@gmail.com>
Cc: Maximiliano Curia <maxy@gnuservers.com.ar>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
Acked-by: Stas Sergeev <stsp@users.sourceforge.net>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---

v3 - Fix false-positive end-of-file indication when an actual
     EOF push coincidentally happens to be the next input
     char (but not @ line start)

 drivers/tty/n_tty.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 0f74945..e9e3e52 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -104,6 +104,7 @@ struct n_tty_data {
 
 	/* must hold exclusive termios_rwsem to reset these */
 	unsigned char lnext:1, erasing:1, raw:1, real_raw:1, icanon:1;
+	unsigned char push:1;
 
 	/* shared by producer and consumer */
 	char read_buf[N_TTY_BUF_SIZE];
@@ -1755,7 +1756,15 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old)
 
 	if (!old || (old->c_lflag ^ tty->termios.c_lflag) & ICANON) {
 		bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE);
-		ldata->line_start = ldata->canon_head = ldata->read_tail;
+		if (!L_ICANON(tty) || !read_cnt(ldata)) {
+			ldata->line_start = ldata->canon_head = ldata->read_tail;
+			ldata->push = 0;
+		} else {
+			set_bit((ldata->read_head - 1) & (N_TTY_BUF_SIZE - 1),
+				ldata->read_flags);
+			ldata->canon_head = ldata->read_head;
+			ldata->push = 1;
+		}
 		ldata->erasing = 0;
 		ldata->lnext = 0;
 	}
@@ -1958,6 +1967,12 @@ static int copy_from_read_buf(struct tty_struct *tty,
  *	it copies one line of input up to and including the line-delimiting
  *	character into the user-space buffer.
  *
+ *	NB: When termios is changed from non-canonical to canonical mode and
+ *	the read buffer contains data, n_tty_set_termios() simulates an EOF
+ *	push (as if C-d were input) _without_ the DISABLED_CHAR in the buffer.
+ *	This causes data already processed as input to be immediately available
+ *	as input although a newline has not been received.
+ *
  *	Called under the atomic_read_lock mutex
  *
  *	n_tty_read()/consumer path:
@@ -2007,6 +2022,7 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 	if (found && read_buf(ldata, eol) == __DISABLED_CHAR) {
 		n--;
 		eof_push = !n && ldata->read_tail != ldata->line_start;
+		ldata->push = 0;
 	}
 
 	n_tty_trace("%s: eol:%zu found:%d n:%zu c:%zu size:%zu more:%zu\n",
@@ -2031,7 +2047,10 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 	ldata->read_tail += c;
 
 	if (found) {
-		ldata->line_start = ldata->read_tail;
+		if (!ldata->push)
+			ldata->line_start = ldata->read_tail;
+		else
+			ldata->push = 0;
 		tty_audit_push(tty);
 	}
 	return eof_push ? -EAGAIN : 0;
-- 
1.8.1.2

Re: [PATCH v3] n_tty: Fix buffer overruns with larger-than-4k pastes

[Peter Hurley]

On 12/09/2013 11:26 AM, Stas Sergeev wrote:
> 09.12.2013 18:50, Peter Hurley пишет:
>>   	if (found && read_buf(ldata, eol) == __DISABLED_CHAR) {
>>   		n--;
>>   		eof_push = !n && ldata->read_tail != ldata->line_start;
>> +		ldata->push = 0;
>>   	}
> Will this work if the last (and only) char written in raw
> mode appear to be \0 (__DISABLED_CHAR)?

That would have triggered an EOF in older kernels so not a
regression.

>
>> -		ldata->line_start = ldata->read_tail;
>> +		if (!ldata->push)
>> +			ldata->line_start = ldata->read_tail;
>> +		else
>> +			ldata->push = 0;
> Will this work if more that one "fake" EOF is accumulated
> in bitmap because of multiple icanon switches?

Not possible. The read_flags and push indicator are reset
with icanon -> !icanon switches.

> Also, I am a bit surprised with the presence of the code
> like this:
> ---
>
> |	if  (n>  4096)
> 		n+=  4096;
> |
>
> ---
> Am I the only one thinking it is unclear what it does?
> Doesn't it deserve the comment at least?
>
> Or this:
> ---
>
> |eof_push= !n&&  ldata->read_tail!=  ldata->line_start;|
>
> ---
> If eof_push means that the EOL mark was found not at the
> line start, then it is completely confusing why !n is here
> (one have to read a lot of context to find out why).|||
> When I created the patch, the code was much more easy
> to follow than now.

The previous kernels did byte-by-byte copy with lock/unlock
for every byte, into the input buffer and back out of the input
buffer. Simple, but inefficient.

This version has roughly 4n-2 fewer bus locks. The necessary
unsigned arithmetic and additional condition checks make the
code somewhat more complex.

But please feel free to submit patches that fix or clarify
anything you find.

Regards,
Peter Hurley

Re: [PATCH v3] n_tty: Fix buffer overruns with larger-than-4k pastes

[Stas Sergeev]

09.12.2013 21:10, Peter Hurley пишет:
> On 12/09/2013 11:26 AM, Stas Sergeev wrote:
>> 09.12.2013 18:50, Peter Hurley пишет:
>>>       if (found && read_buf(ldata, eol) == __DISABLED_CHAR) {
>>>           n--;
>>>           eof_push = !n && ldata->read_tail != ldata->line_start;
>>> +        ldata->push = 0;
>>>       }
>> Will this work if the last (and only) char written in raw
>> mode appear to be \0 (__DISABLED_CHAR)?
>
> That would have triggered an EOF in older kernels so not a
> regression.
I mean the case when icanon is enabled _after_ the
\0 was written. In an unpatched kernel it will not result
in an EOL mark, so I don't expect it to trigger EOF.

Re: [PATCH v3] n_tty: Fix buffer overruns with larger-than-4k pastes

[Peter Hurley]

On 12/10/2013 01:15 AM, Stas Sergeev wrote:
> 09.12.2013 21:10, Peter Hurley пишет:
>> On 12/09/2013 11:26 AM, Stas Sergeev wrote:
>>> 09.12.2013 18:50, Peter Hurley пишет:
>>>>       if (found && read_buf(ldata, eol) == __DISABLED_CHAR) {
>>>>           n--;
>>>>           eof_push = !n && ldata->read_tail != ldata->line_start;
>>>> +        ldata->push = 0;
>>>>       }
>>> Will this work if the last (and only) char written in raw
>>> mode appear to be \0 (__DISABLED_CHAR)?
>>
>> That would have triggered an EOF in older kernels so not a
>> regression.
> I mean the case when icanon is enabled _after_ the
> \0 was written. In an unpatched kernel it will not result
> in an EOL mark, so I don't expect it to trigger EOF.

Right you are, Stas.
I found some other problems as well, so v4 coming.

Regards,
Peter Hurley

[PATCH v4] n_tty: Fix buffer overruns with larger-than-4k pastes

[Peter Hurley]

readline() inadvertently triggers an error recovery path when
pastes larger than 4k overrun the line discipline buffer. The
error recovery path discards input when the line discipline buffer
is full and operating in canonical mode and no newline has been
received. Because readline() changes the termios to non-canonical
mode to read the line char-by-char, the line discipline buffer
can become full, and then when readline() restores termios back
to canonical mode for the caller, the now-full line discipline
buffer triggers the error recovery.

When changing termios from non-canon to canon mode and the read
buffer contains data, simulate an EOF push _without_ the
DISABLED_CHAR in the read buffer.

Importantly for the readline() problem, the termios can be
changed back to non-canonical mode without changes to the read
buffer occurring; ie., as if the previous termios change had not
happened (as long as no intervening read took place).

Preserve existing userspace behavior which allows '\0's already
received in non-canon mode to be read as '\0's in canon mode
(rather than trigger add'l EOF pushes or an actual EOF).

Patch based on original proposal and discussion here
https://bugzilla.kernel.org/show_bug.cgi?id=55991
by Stas Sergeev <stsp@users.sourceforge.net>

Reported-by: Margarita Manterola <margamanterola@gmail.com>
Cc: Maximiliano Curia <maxy@gnuservers.com.ar>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
Acked-by: Stas Sergeev <stsp@users.sourceforge.net>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---

v3 - Fix false-positive end-of-file indication when an actual
     EOF push coincidentally happens to be the next input
     char (but not @ line start)

v4 - Always reset line_start to read_tail when changing icanon
   - Reset 'fake' EOF push status when flushing buffers
   - Allow '\0' written in non-canon to be read in canon
     (preserves existing userspace-visible behavior)

 drivers/tty/n_tty.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 669a507..1a25552 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -105,6 +105,7 @@ struct n_tty_data {
 
 	/* must hold exclusive termios_rwsem to reset these */
 	unsigned char lnext:1, erasing:1, raw:1, real_raw:1, icanon:1;
+	unsigned char push:1;
 
 	/* shared by producer and consumer */
 	char read_buf[N_TTY_BUF_SIZE];
@@ -341,6 +342,7 @@ static void reset_buffer_flags(struct n_tty_data *ldata)
 
 	ldata->erasing = 0;
 	bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE);
+	ldata->push = 0;
 }
 
 static void n_tty_packet_mode_flush(struct tty_struct *tty)
@@ -1758,7 +1760,16 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old)
 
 	if (!old || (old->c_lflag ^ tty->termios.c_lflag) & ICANON) {
 		bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE);
-		ldata->line_start = ldata->canon_head = ldata->read_tail;
+		ldata->line_start = ldata->read_tail;
+		if (!L_ICANON(tty) || !read_cnt(ldata)) {
+			ldata->canon_head = ldata->read_tail;
+			ldata->push = 0;
+		} else {
+			set_bit((ldata->read_head - 1) & (N_TTY_BUF_SIZE - 1),
+				ldata->read_flags);
+			ldata->canon_head = ldata->read_head;
+			ldata->push = 1;
+		}
 		ldata->erasing = 0;
 		ldata->lnext = 0;
 	}
@@ -1964,6 +1975,12 @@ static int copy_from_read_buf(struct tty_struct *tty,
  *	it copies one line of input up to and including the line-delimiting
  *	character into the user-space buffer.
  *
+ *	NB: When termios is changed from non-canonical to canonical mode and
+ *	the read buffer contains data, n_tty_set_termios() simulates an EOF
+ *	push (as if C-d were input) _without_ the DISABLED_CHAR in the buffer.
+ *	This causes data already processed as input to be immediately available
+ *	as input although a newline has not been received.
+ *
  *	Called under the atomic_read_lock mutex
  *
  *	n_tty_read()/consumer path:
@@ -2010,7 +2027,7 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 	n += found;
 	c = n;
 
-	if (found && read_buf(ldata, eol) == __DISABLED_CHAR) {
+	if (found && !ldata->push && read_buf(ldata, eol) == __DISABLED_CHAR) {
 		n--;
 		eof_push = !n && ldata->read_tail != ldata->line_start;
 	}
@@ -2037,7 +2054,10 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
 	ldata->read_tail += c;
 
 	if (found) {
-		ldata->line_start = ldata->read_tail;
+		if (!ldata->push)
+			ldata->line_start = ldata->read_tail;
+		else
+			ldata->push = 0;
 		tty_audit_push(tty);
 	}
 	return eof_push ? -EAGAIN : 0;
-- 
1.8.1.2

Re: [PATCH v4] n_tty: Fix buffer overruns with larger-than-4k pastes

[Greg Kroah-Hartman]

On Tue, Dec 10, 2013 at 05:12:02PM -0500, Peter Hurley wrote:
> readline() inadvertently triggers an error recovery path when
> pastes larger than 4k overrun the line discipline buffer. The
> error recovery path discards input when the line discipline buffer
> is full and operating in canonical mode and no newline has been
> received. Because readline() changes the termios to non-canonical
> mode to read the line char-by-char, the line discipline buffer
> can become full, and then when readline() restores termios back
> to canonical mode for the caller, the now-full line discipline
> buffer triggers the error recovery.
> 
> When changing termios from non-canon to canon mode and the read
> buffer contains data, simulate an EOF push _without_ the
> DISABLED_CHAR in the read buffer.
> 
> Importantly for the readline() problem, the termios can be
> changed back to non-canonical mode without changes to the read
> buffer occurring; ie., as if the previous termios change had not
> happened (as long as no intervening read took place).
> 
> Preserve existing userspace behavior which allows '\0's already
> received in non-canon mode to be read as '\0's in canon mode
> (rather than trigger add'l EOF pushes or an actual EOF).
> 
> Patch based on original proposal and discussion here
> https://bugzilla.kernel.org/show_bug.cgi?id=55991
> by Stas Sergeev <stsp@users.sourceforge.net>
> 
> Reported-by: Margarita Manterola <margamanterola@gmail.com>
> Cc: Maximiliano Curia <maxy@gnuservers.com.ar>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
> Acked-by: Stas Sergeev <stsp@users.sourceforge.net>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---

Is this a 3.13-final thing, or can it wait for 3.14-rc1?

thanks,

greg k-h

Re: [PATCH v4] n_tty: Fix buffer overruns with larger-than-4k pastes

[Peter Hurley]

On 12/16/2013 07:57 PM, Greg Kroah-Hartman wrote:
> On Tue, Dec 10, 2013 at 05:12:02PM -0500, Peter Hurley wrote:
>> readline() inadvertently triggers an error recovery path when
>> pastes larger than 4k overrun the line discipline buffer. The
>> error recovery path discards input when the line discipline buffer
>> is full and operating in canonical mode and no newline has been
>> received. Because readline() changes the termios to non-canonical
>> mode to read the line char-by-char, the line discipline buffer
>> can become full, and then when readline() restores termios back
>> to canonical mode for the caller, the now-full line discipline
>> buffer triggers the error recovery.
>>
>> When changing termios from non-canon to canon mode and the read
>> buffer contains data, simulate an EOF push _without_ the
>> DISABLED_CHAR in the read buffer.
>>
>> Importantly for the readline() problem, the termios can be
>> changed back to non-canonical mode without changes to the read
>> buffer occurring; ie., as if the previous termios change had not
>> happened (as long as no intervening read took place).
>>
>> Preserve existing userspace behavior which allows '\0's already
>> received in non-canon mode to be read as '\0's in canon mode
>> (rather than trigger add'l EOF pushes or an actual EOF).
>>
>> Patch based on original proposal and discussion here
>> https://bugzilla.kernel.org/show_bug.cgi?id=55991
>> by Stas Sergeev <stsp@users.sourceforge.net>
>>
>> Reported-by: Margarita Manterola <margamanterola@gmail.com>
>> Cc: Maximiliano Curia <maxy@gnuservers.com.ar>
>> Cc: Pavel Machek <pavel@ucw.cz>
>> Cc: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
>> Acked-by: Stas Sergeev <stsp@users.sourceforge.net>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>
> Is this a 3.13-final thing, or can it wait for 3.14-rc1?

Definitely not 3.13 at this point -- it should go to -next.

Regards,
Peter Hurley

Re: [PATCH v4] n_tty: Fix buffer overruns with larger-than-4k pastes

[Henrique de Moraes Holschuh]

On Mon, 16 Dec 2013, Peter Hurley wrote:
> >Is this a 3.13-final thing, or can it wait for 3.14-rc1?
> 
> Definitely not 3.13 at this point -- it should go to -next.

Please earmark it for stable, if possible. This fixes a rather annoying long
time bug, after all...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH v4] n_tty: Fix buffer overruns with larger-than-4k pastes

[Peter Hurley]

On 12/18/2013 06:48 AM, Henrique de Moraes Holschuh wrote:
> On Mon, 16 Dec 2013, Peter Hurley wrote:
>>> Is this a 3.13-final thing, or can it wait for 3.14-rc1?
>>
>> Definitely not 3.13 at this point -- it should go to -next.
>
> Please earmark it for stable, if possible. This fixes a rather annoying long
> time bug, after all...

There is a (unlikely) possibility that this will cause a regression
in userspace (because this causes a successful read() to return when
it would not have in prior kernels).

I'd like to see this survive into 3.14-rc5+ before this goes to 3.12
& 3.13 -stable.

This will need multiple, different backports for pre-3.12 kernels,
which may not work correctly because pre-3.12 didn't lock out the input
processing worker during termios changes.

Regards,
Peter Hurley

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Pavel Machek]

Hi!

> >>How is this different from the unpatched kernel?
> >>In the unpatched kernel, if you happen on reader side
> >>to enable icanon while n_tty received all but VEOF (is this
> >>possible at all?),
> >>then the buffer will be flushed, and the remaining VEOF
> >>will get you a nice EOF.
> >>So, in the unpatched kernel you get EOF because the buffer
> >>gets wiped.
> >
> >???
> >
> >Testcase output from 3.12 w/o patch:
> OK, sorry, after a year of rot of my patch in bugzilla, I've
> completely forgot the pre-conditions, which is that the
> buffer is not discarded, just not pushed.
> 
> >Consider the total brute-force approach; a shadow read_flags that
> >distinguishes a real EOF receive from the fake EOF push initiated
> >by the patch.

Was this solved somehow?

Given that it is recent regression, maybe right solution is to do the brute-force patch
now, and worry about effectivity later?

										Pavel

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Stas Sergeev]

28.01.2014 16:03, Pavel Machek пишет:
> Hi!
>
>>>> How is this different from the unpatched kernel?
>>>> In the unpatched kernel, if you happen on reader side
>>>> to enable icanon while n_tty received all but VEOF (is this
>>>> possible at all?),
>>>> then the buffer will be flushed, and the remaining VEOF
>>>> will get you a nice EOF.
>>>> So, in the unpatched kernel you get EOF because the buffer
>>>> gets wiped.
>>> ???
>>>
>>> Testcase output from 3.12 w/o patch:
>> OK, sorry, after a year of rot of my patch in bugzilla, I've
>> completely forgot the pre-conditions, which is that the
>> buffer is not discarded, just not pushed.
>>
>>> Consider the total brute-force approach; a shadow read_flags that
>>> distinguishes a real EOF receive from the fake EOF push initiated
>>> by the patch.
> Was this solved somehow?
Wasn't this already applied?
I think you've missed the part of the discussion thread:
https://groups.google.com/forum/#!msg/linux.kernel/05c-vQUDww4/umXJsD_uiskJ

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Peter Hurley]

On 01/28/2014 07:17 AM, Stas Sergeev wrote:
> 28.01.2014 16:03, Pavel Machek пишет:
>> Hi!
>>
>>>>> How is this different from the unpatched kernel?
>>>>> In the unpatched kernel, if you happen on reader side
>>>>> to enable icanon while n_tty received all but VEOF (is this
>>>>> possible at all?),
>>>>> then the buffer will be flushed, and the remaining VEOF
>>>>> will get you a nice EOF.
>>>>> So, in the unpatched kernel you get EOF because the buffer
>>>>> gets wiped.
>>>> ???
>>>>
>>>> Testcase output from 3.12 w/o patch:
>>> OK, sorry, after a year of rot of my patch in bugzilla, I've
>>> completely forgot the pre-conditions, which is that the
>>> buffer is not discarded, just not pushed.
>>>
>>>> Consider the total brute-force approach; a shadow read_flags that
>>>> distinguishes a real EOF receive from the fake EOF push initiated
>>>> by the patch.
>> Was this solved somehow?
> Wasn't this already applied?

Yes, this was applied and is on the mainline master branch,
so will appear in 3.14-rc1.

Regards,
Peter Hurley

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Peter Hurley]

On 08/08/2013 01:58 PM, Maximiliano Curia wrote:
> Hi,
>
>> n_tty_set_room() in drivers/tty/n_tty.c (3.10 mainline)
>
>>  From n_tty_set_room():
>
>> 	/*
>> 	 * If we are doing input canonicalization, and there are no
>> 	 * pending newlines, let characters through without limit, so
>> 	 * that erase characters will be handled.  Other excess
>> 	 * characters will be beeped.
>> 	 */
>> 	if (left <= 0)
>> 		left = ldata->icanon && !ldata->canon_data;
>> 	old_left = tty->receive_room;
>> 	tty->receive_room = left;
>
> I took a long look at this code and thought about how it could be made to work
> for readline's case and also for the canonical readers.  I came up with this
> simple patch:
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 4bf0fc0..2ba7f4e 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -149,7 +149,8 @@ static int set_room(struct tty_struct *tty)
>           * characters will be beeped.
>           */
>          if (left <= 0)
> -               left = ldata->icanon && !ldata->canon_data;
> +               if (waitqueue_active(&tty->read_wait))
> +                       left = ldata->icanon && !ldata->canon_data;
>          old_left = tty->receive_room;
>          tty->receive_room = left;
>
> This is of course just an idea, but I tested this and it worked correctly for
> the cases I was testing.
>
> The effect of this patch is that when there is a canonical reader waiting for
> input, it maintains the previous behavior, but when there's no reader (like
> when readline is changing modes), it blocks and doesn't lose any characters.

Apologies for taking so long to reply.

My primary concern is canonical readers not become stuck with a full
read buffer, even with bogus input data (IOW, that an error condition will
not prevent a reader from making forward progress). I believe that won't
happen with this change, but what I really need in this case is a detailed
analysis from you of why that won't happen. That analysis should be in
the patch changelog. (Feel free to send me private mail if you need help
preparing a patch.)

And the patch above has a bug that allows a negative 'left' to be
assigned to tty->receive_room which will be interpreted as a very large
positive value.

This approach still has several drawbacks.

1) Since additional state is reset when the termios is changed by
readline(), the canonical line buffer state will be bogus.
This renders the termios change by readline() pointless; the
caller will not be able to retrieve expected input properly.

2) Since the input data is interpreted with the current termios when
data is received, any embedded control characters will not be
interpreted properly; again, the caller will not be able to retrieve
expected input properly.

> Another approach would be to recalculate the size of canon_data when the mode
> is changed, but this would probably be much more invasive, and awfully less
> efficient since it would imply going through the buffer.

This approach is not possible prior to linux-next since the input worker
thread and the reader thread are not locked out of access to the read buffer
while changing the termios.

And while rescanning the read buffer is possible in linux-next (eg, to
compute the read_flags bitmap indicating the position of NLs), this doesn't
address embedded control characters not being reinterpreted. And completely
reinterpreting the read buffer makes interpreting when receiving pointless.

> What do you think? Is the proposed solution, or something along those lines,
> acceptable?

I'm wondering if this problem might be best addressed on the paste side
instead of the read side. Although this wouldn't be a magic bullet, it
would be easier to control when more paste data is added.

Regards,
Peter Hurley

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Maximiliano Curia]

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

¡Hola Peter!

El 2013-08-19 a las 08:25 -0400, Peter Hurley escribió:
> My primary concern is canonical readers not become stuck with a full
> read buffer, even with bogus input data (IOW, that an error condition will
> not prevent a reader from making forward progress). I believe that won't
> happen with this change, but what I really need in this case is a detailed
> analysis from you of why that won't happen. That analysis should be in
> the patch changelog. (Feel free to send me private mail if you need help
> preparing a patch.)

I'm not sure what level of analysis you are looking for. The driver will block
when there are no readers but as soon as there is a read call it unblocks.
I've added this information to the patch description that I'm including below.

> And the patch above has a bug that allows a negative 'left' to be
> assigned to tty->receive_room which will be interpreted as a very large
> positive value.

Ok, fixed with an else clause. It could also use an extra &&, but it looks a
bit confusing.

> This approach still has several drawbacks.

> 1) Since additional state is reset when the termios is changed by
> readline(), the canonical line buffer state will be bogus.
> This renders the termios change by readline() pointless; the
> caller will not be able to retrieve expected input properly.

> 2) Since the input data is interpreted with the current termios when
> data is received, any embedded control characters will not be
> interpreted properly; again, the caller will not be able to retrieve
> expected input properly.

Indeed this is correct, however this is not an issue of this patch but of the
current interaction between the kernel and readline. In order to fix this, the
reading buffer should always be in raw and only when responding to a read call
for canonical mode should it be interpreted. This is a very big change, and
I'm not sure if anybody will be interested in implementing it.

> >What do you think? Is the proposed solution, or something along those lines,
> >acceptable?

> I'm wondering if this problem might be best addressed on the paste side
> instead of the read side. Although this wouldn't be a magic bullet, it
> would be easier to control when more paste data is added.

I don't see how this could work, could you elaborate?

This is the patch proposal, for comments:

From 81afd3b666cbf94bb9923ebf87fb2017a7cd645e Mon Sep 17 00:00:00 2001
From: Maximiliano Curia <maxy@gnuservers.com.ar>
Date: Tue, 3 Sep 2013 22:48:34 +0200
Subject: [PATCH] Only let characters through when there are active readers.

If there is an active reader, previous behavior is in place. When there is no
active reader, input is blocked until the next read call unblocks it.

This fixes a long standing issue with readline when pasting more than 4096
bytes.
---
 drivers/tty/n_tty.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 4bf0fc0..cdc3b19 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -147,9 +147,16 @@ static int set_room(struct tty_struct *tty)
 	 * pending newlines, let characters through without limit, so
 	 * that erase characters will be handled.  Other excess
 	 * characters will be beeped.
+	 * If there is no reader waiting for the input, block instead of
+	 * letting the characters through.
 	 */
 	if (left <= 0)
-		left = ldata->icanon && !ldata->canon_data;
+		if (waitqueue_active(&tty->read_wait)) {
+			left = ldata->icanon && !ldata->canon_data;
+		} else {
+			left = 0;
+		}
+
 	old_left = tty->receive_room;
 	tty->receive_room = left;
 
-- 
1.8.4.rc3


-- 
"Always code as if the person who ends up maintaining your code is a violent
psychopath who knows where you live."
-- John Woods
Saludos /\/\ /\ >< `/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

[Peter Hurley]

On 09/03/2013 05:12 PM, Maximiliano Curia wrote:
> ¡Hola Peter!
>
> El 2013-08-19 a las 08:25 -0400, Peter Hurley escribió:
>> My primary concern is canonical readers not become stuck with a full
>> read buffer, even with bogus input data (IOW, that an error condition will
>> not prevent a reader from making forward progress). I believe that won't
>> happen with this change, but what I really need in this case is a detailed
>> analysis from you of why that won't happen. That analysis should be in
>> the patch changelog. (Feel free to send me private mail if you need help
>> preparing a patch.)
>
> I'm not sure what level of analysis you are looking for.

For example, why will CPU 0 (the reader) not hang forever under the following
circumstances?

CPU 0                                | CPU 1
                                      |
n_tty_read()                         | n_tty_receive_buf2()
   .                                  |   receive_room
   .                                  |     .
   .                                  |     is waitqueue_active()? no
   add_wait_queue()                   |       .
     .                                |       .
     is input_available_p()?          |       .
       no - schedule_timeout()  <-- reader sleeps

                                      |       .
                                      |       return 0
                                      | exit i/o loop in flush_to_ldisc() work function
                                      |


A complete and detailed analysis would go a long way to getting this patch
accepted. If you show that your patch won't hang readers _and_ fixes the
larger-than-4k-paste bug, then the readline() deficiencies will probably be
overlooked.

> The driver will block
> when there are no readers but as soon as there is a read call it unblocks.
> I've added this information to the patch description that I'm including below.
>
>> And the patch above has a bug that allows a negative 'left' to be
>> assigned to tty->receive_room which will be interpreted as a very large
>> positive value.
>
> Ok, fixed with an else clause. It could also use an extra &&, but it looks a
> bit confusing.
>
>> This approach still has several drawbacks.
>
>> 1) Since additional state is reset when the termios is changed by
>> readline(), the canonical line buffer state will be bogus.
>> This renders the termios change by readline() pointless; the
>> caller will not be able to retrieve expected input properly.
>
>> 2) Since the input data is interpreted with the current termios when
>> data is received, any embedded control characters will not be
>> interpreted properly; again, the caller will not be able to retrieve
>> expected input properly.
>
> Indeed this is correct, however this is not an issue of this patch but of the
> current interaction between the kernel and readline.

My point here was this: the whole point of readline() flipping termios settings
back and forth is to allow readline() to use one setting while the caller
uses a different setting.

If you don't care that the readline() caller doesn't receives its input
properly when pasted, why is the solution to this problem patching the
linux kernel instead of ripping out the termios-flipping that readline()
does?

> In order to fix this, the
> reading buffer should always be in raw and only when responding to a read call
> for canonical mode should it be interpreted. This is a very big change, and
> I'm not sure if anybody will be interested in implementing it.

So the receiver only echoes input once it's been read???  Because if implemented
as you suggest, until the input has been read you wouldn't know what termios
settings to apply for the echoed chars (or even if to echo).

>>> What do you think? Is the proposed solution, or something along those lines,
>>> acceptable?
>
>> I'm wondering if this problem might be best addressed on the paste side
>> instead of the read side. Although this wouldn't be a magic bullet, it
>> would be easier to control when more paste data is added.
>
> I don't see how this could work, could you elaborate?

Well, the vt driver already supports a PASTE_SELECTION ioctl
which bypasses the flip buffers. Something similar might be suitable
for line-oriented pasting, where only one line is written at a time.
I haven't given too much thought to how/what would perform the wake
up of the paster; right now, the vt driver implements unthrottle to
continue pasting.

> This is the patch proposal, for comments:
>
>  From 81afd3b666cbf94bb9923ebf87fb2017a7cd645e Mon Sep 17 00:00:00 2001
> From: Maximiliano Curia <maxy@gnuservers.com.ar>
> Date: Tue, 3 Sep 2013 22:48:34 +0200
> Subject: [PATCH] Only let characters through when there are active readers.
>
> If there is an active reader, previous behavior is in place. When there is no
> active reader, input is blocked until the next read call unblocks it.
>
> This fixes a long standing issue with readline when pasting more than 4096
> bytes.
> ---
>   drivers/tty/n_tty.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 4bf0fc0..cdc3b19 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -147,9 +147,16 @@ static int set_room(struct tty_struct *tty)
>   	 * pending newlines, let characters through without limit, so
>   	 * that erase characters will be handled.  Other excess
>   	 * characters will be beeped.
> +	 * If there is no reader waiting for the input, block instead of
> +	 * letting the characters through.
>   	 */
>   	if (left <= 0)
> -		left = ldata->icanon && !ldata->canon_data;
> +		if (waitqueue_active(&tty->read_wait)) {
> +			left = ldata->icanon && !ldata->canon_data;
> +		} else {
> +			left = 0;
> +		}

Style nitpick. Single-line if-else shouldn't have braces.

> +
>   	old_left = tty->receive_room;
>   	tty->receive_room = left;
>
>