Re: [bug-inetutils] Bug report for gnu inetutils telnetd

[Mats Erik Andersson]

Hello there,

Wednesday den 22 July 2015 klockan 14:36 skrev Chris Severance detta:
>    Working version: 1.9.2
>    Broken version: 1.9.3 (5/12/2015) and 1.9.4
>    Type: regression
>    Severity: telnetd is unusable for production and hard to use for test
>    Environment: Arch Linux x86_64 Linux host 4.1.2-2-ARCH #1 SMP PREEMPT
>    https://www.archlinux.org/packages/core/x86_64/inetutils/
>    https://wiki.archlinux.org/index.php/Telnet
> 
>    Problem:
> 
>    The first connection to the telnetd server usually connects. 90% of
>    additional connections to the telnetd server show the banner then
>    disconnect without a login prompt. Any connection that shows a login
>    prompt works fine.
> 
>    Here's how I do it in PuTTY for Windows:
>    Create and save a Telnet connection with the following setting:
>      Close on window exit (*)Always
> 
>    Create a link on the desktop:
>      putty.exe -load "Telnet Foo"
> 
>    Click fast!

Morally it can be argued that this failure is a good thing! 

>              /* Something to read from the pty... */
>    -         if (pty_read () <= 0)
>    +         if (pty_read () < 0)
>                break;
>              /* The first byte is now TIOCPKT data.  Peek at it.  */
>              c = pty_get_char (1);

The call to pty_read() returns 0 in case EWOULDBLOCK, EAGAIN or EIO
appeared during read. Here EIO is reported to appear when reading
from the master device, while the slave side is not yet active.
This is likely the reason you have been able to provoke a disconnect.

The main reason for me to implement the change, now under dispute,
was that the BSD implementation uses exactly 'if (read() < 0) break;',
and my analysis revealed no side effects of a change, rather the change
would avoid a no-op run. It now seems my predecessors of GNU Inetutils
have conceiled side-effects when reorganizing the original BSD code
to their own taste.

>    I don't know that this fix is correct but I note some things.
> 
>    This change drew my attention because a comparison change is non
>    trivial. Other code would need substantial changes for this to be a
>    valid change.

Could I get some detail for this claim to support a "valid change"?

>    The return value of readstream is minimally documented. -1 seems to be
>    an error. 0 is not obvious. 1 appears to be a flag value. 2+ seems to
>    be a length.

On BSD and GNU/Linux, readstream() deteriorates to read(). To my knowledge,
only Solaris, OpenSolaris and OpenIndiana would set HAVE_STREAMSPTY,
thus making readstream() into anything more involved than a simple read().
> 
>    I suspect 0 is a valid return value because I think pty_get_char is
>    expected to block when necessary.

No, pty_get_char() is only grasping into a string buffer, so it always
returns, but possibly with NUL as return character. Other mechanisms
fill this buffer as needs arise.

At the time of this writing I am testing a patch witch does what you
suggest and also adds code comments to clarify why the return value 0
seems so decisive.

Regards for now,
  M E Andersson