On 01/19/2015 11:36 AM, Howard Chu wrote:
> Peter Hurley wrote:
>> Thanks, Howard.
>>
>> [ adding Ted too ]
>>
>> On 01/19/2015 07:46 AM, Howard Chu wrote:
>>> Peter Hurley wrote:
>>>> On 01/18/2015 05:45 PM, Howard Chu wrote:
>>>>> Peter Hurley wrote:
>>>>>> Commit 26df6d13406d1 ("tty: Add EXTPROC support for LINEMODE") added
>>>>>> the undocumented EXTPROC input processing mode, which ignores the ICANON
>>>>>> setting and forces pty slave input to be processed in non-canonical
>>>>>> mode.
>>>>>
>>>>> What's the motivation to remove this code, rather than improve it if it
>>>>> needs fixing? It has been removed from the Linux kernel at least once
>>>>> before already, and that was a mistake back then too.
>>>>
>>>> It is a significant maintenance burden, and I have concerns about the
>>>> level of support it's receiving. Here's some outstanding issues:
>>>>
>>>> 1. No man page documentation. At a minimum, tty_ioctl(4) and termios(3)
>>>> need the userspace visible definitions and behavior documented. Better
>>>> would be a LINEMODE (7) description of how this implementation works
>>>> wrt supporting RFC 1116.
>>>
>>> That can be added easily enough.
>>
>> Is this you signing up? :)
>
> Sure. I suppose the obvious place to add it is near the TIOCPKT doc in
> tty_ioctl(4). But, about your further comment:
Thanks.
>> Which brings up another point: only a pty master should be able to set
>> EXTPROC
>> mode. Right now, any tty can be set to EXTPROC and the pty slave can even
>> accidentally unset it. This argues for a new pty ioctl() to set EXTPROC in
>> termios. pty_set_termios() can silently merge the bit.
>
> Historically EXTPROC could also be set on regular ttys, for lines that were
> hooked up to special terminal interfaces. E.g. X.29 PADs that did their own
> input processing. I'm not sure how relevant this is now, but this is the
> reason EXTPROC is not a pty-specific ioctl.
How does the remote end find out when non-canon mode is selected?
And who does the EXTPROC setting, because if it's the program that opened the
tty, then it can just select raw mode instead.
>> A description of how the pty master uses EXTPROC to implement line mode would
>> be very helpful, especially to people working in the tty code (eg., me,
>> although
>> I don't need it now).
>
> The original patch submission carried this description
> http://lkml.iu.edu/hypermail/linux/kernel/1006.2/02428.html
>
>>>> 3. Does the local edit guarantee canon lines <= 4096 chars? What happens
>>>> if pty slave reader does this?
>>>>
>>>> char buffer[4096];
>>>> char *p = buffer;
>>>>
>>>> n = read(tty, buffer, sizeof(buffer));
>>>> if (n <= 0)
>>>> goto done;
>>>> while (*p++ != '\n')
>>>> ;
>>>
>>> This reader is broken, since the tty driver supports EOL and EOL2 and this
>>> code doesn't account for it.
>>
>> This reader set EOL2 to DISABLED_CHAR earlier, and left EOL unchanged.
>> I have seen userspace code that expects a line to be no longer than 4096
>> chars.
>>
>>> In practice your concern is misdirected - it's the job of whatever code is
>>> talking to the pty master to send valid data to the pty slave. There's no
>>> reason for the tty driver to second-guess the app here.
>>
>> What about a malicious client?
>
> This is still a broken reader. It's as bad as using strchr() without knowing
> if the string is NUL-terminated, if 4096 bytes are read the thing will go off
> into the weeds.
There's lots of broken userspace programs that do all kinds of stupid stuff.
Want to find out which ones they are? Just declare N_TTY_BUF_SIZE to be 8192
and start allowing that canon line size through.
>>>> 4. ioctl(TIOCSIG) can send _any_ signal to a different process without
>>>> permission checks. That's not good.
>>>
>>> It can only send to the pty slave. Permissions were already checked when
>>> the pty master was opened.
>>
>> Still not ok.
>>
>>> What further checks do you think are needed? You think it should be limited
>>> to tty-specific signals: INT, QUIT, CONT, TSTP, TTIN, TTOU, WINCH?
>>
>> My first choice is to do away with TIOCSIG ioctl completely; see ending
>> comments.
>> My second would be masked to only those already used: INT, QUIT, TSTP.
>
> Perhaps your way is better. This patch simply duplicated the BSD
> functionality, to maintain compatibility.
>
>>>> 5. This needs to work with readline(). Right now, I don't see how this
>>>> won't have worst-case behavior, constantly sending termios changes,
>>>> with scripted input where the reader switches back-and-forth between
>>>> canonical and non-canonical mode (like readline() does). Database
>>>> shells behave like this, but you can do a 20-line shell mockup with
>>>> just readline().
>>>
>>> readline() patched accordingly
>>> https://groups.google.com/forum/#!topic/gnu.bash.bug/o0UA55AhADs will
>>> cooperate. Sending one or two termios changes per input line is still far
>>> better than one character-at-a-time packets back and forth.
>>
>> Ok, I misunderstood: the only incompatibility here is that readline()
>> simply doesn't use line mode. That's fine from the kernel perspective.
>>
>> Although it does seem odd that the command shell doesn't support the
>> input mode :)
>>
>>>> 6. EXTPROC still does some input processing on the server. For example,
>>>> 7-bit mode (ISTRIP), tolower mode (IUCLC) and processing while
>>>> closing; if input processing is being done on the local/client side,
>>>> why the extra work here?
>>>
>>> That's defensive, on the assumption that something else might break if e.g.
>>> the tty expected only 7-bit input but 8-bit characters were sent to it.
>>
>> Ok, is that because RFC 1116 doesn't specify ISTRIP and IUCLC handling so
>> the server can't be sure the client did it? If so, that should be documented
>> so that refactors don't remove that handling.
Could you get back to me about this, as well?
>> Regular ptys dump input while closing so this should too.
>>
>>>> 7. This needs a reference userspace implementation which for the moment
>>>> could double as regression testing. A library with unit tests would
>>>> be ideal.
>>>
>>> telnet/telnetd can probably used as a starting point for this.
>>
>> Except telnet is unmaintained except by each distro. Your patches don't apply
>> to my distro telnet; they had been applied but are now ifdef'd out. I messed
>> around with rebuilding it but never got telnetd to actually set EXTPROC.
>>
>> My point is there is no straightforward way to test this, and that's a
>> problem.
>>
>> In fact, this is my main concern: no matter how useful this might be, it's
>> pointless if the complementary userspace puzzle piece is unsupported.
>> I know you have written patches but that's not the same as support; for
>> example
>> your OpenSSH fork is 4 years old, which is a non-starter because I'm sure
>> there's known exploits which have been fixed in those 4 years.
>>
>> Why doesn't Ubuntu/Debian telnet even compile for line mode support? Did the
>> package maintainer shut it off because it was causing bug reports?
>
> I don't know. Looking at the package history it appears they have never
> enabled it. https://launchpad.net/ubuntu/+source/netkit-telnet/+changelog
>
> I also don't know where the actual VCS repo is for this code.
There isn't one. The base code is from downloaded tarball.
I don't know what distro you use (or if you even do), but for Ubuntu/Debian
the base package information is here https://packages.debian.org/wheezy/telnetd
including maintainer email, etc.
Typically, to get changes merged requires making a patch(es) against the package
source and sending/uploading those.
Another alternative is to setup your own repository from that packaging;
sometimes I do that on Launchpad from the packaging source so that
the maintainer can easily see the difference. Ubuntu calls these PPAs.
>> What more needs to be done to get line mode working 100% in bash?
>
> TAB-completion was messy. Although editing and scrolling through command
> history can be done completely locally, TAB-completion required sending a
> partial line to the server and feeding the completion results back, and then
> possibly flushing the input buffer if the results weren't actually used.
>
> With a few years of hindsight, this can probably be cleaned up better now.
Ok.
>>>> I'd like to do away with the signalling part; just turn off EXTPROC
>>>> and send the appropriate signalling char from the pty master, like telnetd
>>>> does now. Same for EOF.
>>>
>>> That introduces additional mode changes, which you were just worrying about
>>> above, re: readline. It would make the traffic stream less efficient
>>> overall.
>>
>> Maybe you misunderstood. If local signalling is being performed by the
>> client,
>> the over-the-wire traffic is the same. The pty master would turn off EXTPROC
>> to
>> write the signalling char but would not reflect the termios to the client
>> (because
>> it contains no relevant changes).
>>
>> And the signalling char is isolated and unique: it's not as if it's being
>> stuffed
>> within an existing stream of i/o, because the client should have already
>> ceased
>> input if it's handling signals locally.
>>
> Makes sense. Don't know why the original telnet patch didn't do this.
Ok.
I might see if I can get telnet + a revised kernel interface to work together;
I'll
let you know if this works out.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
