[net] TelnetClient broken for binary transmissions + solution

• Key: NET-89
• Type: Bug
• Status: Reopened
• Priority: Major
• Assignee: Unassigned
• Reporter: Colin Surprenant

Environment

Operating System: All
Platform: All

Description

TelnetClient does not handle correctly binary transmissions in two places:

First in TelnetClient#connectAction() the telnet input and output streams are
wrapped in the NetASCII streams to handle net vs platform line separator
conversion which breaks the binary data. My quick solution was to simply remove
those two wrapping streams. A more general solution might be to provide access
to the unfilterer stream with methods like getUnfilteredInputStream and
getUnfilteredOutputStream or to dynamically stop the NetASCII stream from
'corrupting' the stream when a TelnetOption.BINARY option is negotiated.

Also, in TelnetInoutStream#__read() there is a bug in the __receiveState
handling for the _STATE_IAC state. When a second consecutive IAC (0x255) is
received to encode the single 0x255 character, read does not return 0x255 but
instead move on to reading the next char in the stream.

The current code reads:

case _STATE_IAC:
switch (ch)

{ case TelnetCommand.WILL: __receiveState = _STATE_WILL; continue; case TelnetCommand.WONT: __receiveState = _STATE_WONT; continue; case TelnetCommand.DO: __receiveState = _STATE_DO; continue; case TelnetCommand.DONT: __receiveState = _STATE_DONT; continue; /* TERMINAL-TYPE option (start)*/ case TelnetCommand.SB: __suboption_count = 0; __receiveState = _STATE_SB; continue; /* TERMINAL-TYPE option (end)*/ case TelnetCommand.IAC: __receiveState = _STATE_DATA; break; default: break; }

__receiveState = _STATE_DATA;
continue;
case _STATE_WILL:

but it should be:

case _STATE_IAC:
switch (ch)

{ case TelnetCommand.WILL: __receiveState = _STATE_WILL; continue; case TelnetCommand.WONT: __receiveState = _STATE_WONT; continue; case TelnetCommand.DO: __receiveState = _STATE_DO; continue; case TelnetCommand.DONT: __receiveState = _STATE_DONT; continue; /* TERMINAL-TYPE option (start)*/ case TelnetCommand.SB: __suboption_count = 0; __receiveState = _STATE_SB; continue; /* TERMINAL-TYPE option (end)*/ case TelnetCommand.IAC: __receiveState = _STATE_DATA; break; // exit to enclosing switch to return from read default: __receiveState = _STATE_DATA; continue; // move on the next char }

break; // exit and return from read
case _STATE_WILL:

I'll provide patches for this.
Colin.

Comments

[Colin Surprenant]

Forgot to mention that this is with commons-net-1.3.0. I never tested binary
transmissions with other versions.

binary transmission negotiation is enabled using:
telnet.addOptionHandler(new SimpleOptionHandler(TelnetOption.BINARY, true, true,
true, true));

My tests were done using a non commons-net telnet server.

[Rory Winston]

Thanks Colin. When you provide the patch(es), is there any chance you may be
able to provide a test case that can be runa against a Telnet server to show the
problem? That way, we can be sure it gets fixed (and stays fixed).

[Colin Surprenant]

I think a better testcase for TelnetInputStream would be to simply wrap it
around a ByteArrayInputStream which contains test patterns and verify what read
returns.

For the NetASCII problem I did not take the time to analyse what would be the
better approach. Regardless of the solution, we should build a "self contained"
test case and not rely on a telnet server to run the tests against.

[Edward Sargisson]

I'd like to provide some additional support for fixing this.

We have a situation where we have some hardware crypto modules connected to a
Lantronix EDS4100 serial-to-IP device. We need to use Telnet to connect to the
Lantronix box. However, the hardware crypto modules use \r as the line
seperator. When running our software on Linux this causes a failure because:
1. The Unix line separator means that FromNetASCIIInputStream is used.
2. The FromNetASCIIInputStream#__read method reads a \r then waits for the next
character.
3. Because our hardware crypto modules only send one line of response with a \r
terminator there is no next character.
4. The read then times out.

There are two basic issues here: One is that JRE properties are being used to
configure the library with no method for the developer to override this. The
second is that the code expects a character after every \r when there is no
guarantee this might happen.

In conclusion, I would like a way to either turn off line feed conversion or to
have access to the unconverted input stream.

[Martin Oberhuber]

For us, the unconditional wrapping of the inputStream in a FromNetASCIIInputStream in the connectAction() is also problematic. We have a Java vt100 terminal implementation, which can run on Windows or UNIX. For the vt100 Terminal, we need the CRLF line endings because LF-only must be interpreted as "line down only".

We could wrap the inputStream in another ToNetASCIIInputStream when on UNIX, but I don't fancy this idea too much since there is still a risk of losing data (LF-only MAY be different than CRLF, not only in binary but in Terminal apps as well, or am I mistaken?) - IMHO using the System.property for deciding on NetASCII decoding or not was not a good idea in the first place at all.

What about simply introducing a field e.g. TelnetClient#setNetASCIIConversion(boolean b) in order to switch on or off the auto-conversion? Should be called before connecting. I'd assume that anyone dealing with actual Terminals or binary transmission would want to switch the auto conversion off.

I also like switching off the auto-conversion when BINARY transmission is negotiated, but I'm a little bit worried about what should happen when the binary negotiation happens at a time where e.g. the CR is already consumed but the LF not yet...

[Martin Oberhuber]

PS there doesn't even seem to be a suitable workaround by subclassing TelnetClient and overriding protected connectAction (), because from the subclass we cannot call super.super.connectAction(); and doing super.connectAction() first will leave us with a TelnetInputStream whose Thread has already started and might have consumed some characters already, so we cannot replace the FromNetASCIIInputStream after the fact any more

And, giving access to protected input also isn't an option because we cannot have the TelnetInputStream and the client read from the same input at the same time

Not really having a workaround makes this a major issue indeed, and I'm not sure how this can be fixed without adding new API...

[Martin Oberhuber]

It looks like currently two separate issues are discussed here – the original issue with state handling (not sure if this is fixed yet), and the API problem of not being able to obtain an unconverted inputstream that doesn't undergo the FromNetASCIIInputStream conversion. Should a separate issue be created to track the latter one?

[Martin Oberhuber]

Can we schedule this for fixing in 1.5 and 2.1 by adding following API:

TelnetClient.java

/**
   * Enables or disables automatic conversion of input and output streams
   * to or from NetASCII standard format (CRLF).
   * By default, automatic conversion is ON. It should be disabled when direct
   * (binary) access to the data streams is needed. The setting can only
   * be made or changed before the TelnetClient is connected.
   * 
   * @param b <code>false</code> to disable automatic conversion of streams to NetASCII format.
   */
  public void setNetASCIIConversion(boolean b);

[Martin Oberhuber]

As an alternative, I believe we could do this – though I'm not so fond of this since it also requires a functional change: for this API to work, the conditional conversion from input to __input by applying the FromNetASCIIInputStream or not must be made after applying the ReaderThread, and not before as it's done now. May be a marginal change, but may also change functionality. Also note, that lazy creation of the FromNetASCIIInputStream / ToNetASCIIOutputStream in the getInputStream() / getOutputStream() methods would be preferred over unconditional creation of the Streams in the __connect method.

On the positive side, mixing the Streams (i.e. using either the binary or the NetASCII variant) may be possible when flush() is called when done with one variant. At this point, however, I'm not exactly sure how this could work out, so the Javadocs below discourage such usage.

Finally, another idea might be deprecating getInputStream() / getOutputStream() altogether and replace it with having the client do the ToNetASCIIInputStream() etc conversion on the client side after calling getBinaryOutputStream().

TelnetClient.java

/***
     * Returns the unconverted, binary telnet connection output stream
     * (without automatic NetASCII conversion to CRLF line endings).
     * Clients should use either the stream returned by this method, or
     * the stream returned from {@link #getOutputStream()}, but not mix
     * using the Streams.  You should not close the
     * stream when you finish with it.  Rather, you should call
     * {@link #disconnect  disconnect }.
     * <p>
     * This API is available in Commons Net 1.5 and 2.1, but not in version 2.0
     * <p>
     * @return The unconverted, binary telnet connection output stream.
     * @since 1.5
     * @since 2.1
     ***/
    public OutputStream getBinaryOutputStream()
    {
        return __binaryOutput;
    }

    /***
     * Returns the unconverted, binary telnet connection input stream
     * (without automatic NetASCII conversion from CRLF line endings).
     * Clients should use either the stream returned by this method, or
     * the stream returned from {@link #getInputStream()}, but not mix
     * using the Streams.  You should not close the
     * stream when you finish with it.  Rather, you should call
     * {@link #disconnect  disconnect }.
     * <p>
     * This API is available in Commons Net 1.5 and 2.1, but not in version 2.0
     * <p>
     * @return The unconverted, binary telnet connection input stream.
     * @since 1.5
     * @since 2.1
     ***/
    public InputStream getBinaryInputStream()
    {
        return __binaryInput;
    }

[Shlomy Reinstein]

I have a different case in which the ToNetASCIIInputStream causes a problem. I use text transmissions, not binary, however the telnet server does not seem to tell the difference between '\r' and '\n', and so the replacement of '\n' with "\r\n" causes every line I send to generate an extra newline, which prevents automation (e.g. in the login/password sequence, the password is always passed as empty due to the extra newline).

I suggest to add a method like the "setNetASCIIConversion(boolean)" above, and even have a dedicated one for the just the telnet's output - i.e. avoid only the wrapper ToNetASCIIOutputStream. Something like:

public void setNetASCIIConversionOnOutput(boolean b);
public void setNetASCIIConversionOnInput(boolean b);

The "set" functions can be replaced by "disable" if the default is to have them enabled. Another option, which seems better to me, is to replace these functions with new TelnetClient constructors that take an additional boolean parameter (or two), to ensure this is configured before the connection is made.

Thanks,
Shlomy