Date:	Wed, 11 Sep 2013 21:36:24 -0400
From:	Peter Hurley <peter@...leysoftware.com>
To:	Maximiliano Curia <maxy@...servers.com.ar>
CC:	Margarita Manterola <margamanterola@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.cz>,
	Linux kernel <linux-kernel@...r.kernel.org>
Subject: Re: Large pastes into readline enabled programs causes breakage from
 v2.6.31 onwards

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@...servers.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;
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists