Inproper handling of NULL character and IAC character

[PRemek1]

I wanted to connect two Windows machines via serial ports (in Windows terms - I wanted to dial in from one machine to another using null modem thus establish a ppp session between the two). On the infrastructure the serial ports of both machines were exposed via vSPC.py (I connect the two TCP/IP ports together using socat utility). This didn't work. After a bit of investigation I was able to see that the vSCP.py is screwing up the transmitted data by two things. First thing is - not escaping the 0xFF character on outbound data (thus making the remote end interpret the FF as IAC sequence). Second thing - removing NULL characters from inbound data.

After I fixed both problems, everything started to work properly. Fix for the first thing was fixing the send_buffered method:

def send_buffered(self, s = ''): self.send_buffer += s if IAC in self.send_buffer: self.send_buffer = self.send_buffer.replace(IAC, IAC+IAC)

I am not sure why the author is defining its own method for sending data instead of using one from telnetlib while for receiving is using the original one (from the python's telnetlib library).

Fix for the second was to remove special handling of NULL in the process_rawq method

if self.sb == 0 and c == theNULL: continue

I am not sure why there is a special handling of NULL character. Having looked at the Telnet RFC, the NULL is not a special character.

Let me know what you think and if I shall put it into the source code.

Cheers, Prema

[pbhenson]

The first has been fixed in master; I'm not sure about the second? The upstream python telnetlib seems to eat NULL characters as well.

[dariusd0]

https://datatracker.ietf.org/doc/html/rfc856.html#section-5 suggests that eating NULL is definitely incorrect behavior when BINARY has been negotiated – it simply must be passed through. Without BINARY, eating NULL would seem acceptable/correct.

If upstream python telnetlib eats NULL while in BINARY mode, then that would be a defect in telnetlib too. (I have not tested this myself...)

Since BINARY is always supported and negotiated by both ESXi and vSPC.py, it may be appropriate (for simplicity's sake) to assume we are always in BINARY mode and should always pass through NULL, e.g. by removing those two lines of code. The alternative would be to conditionally eat the NULL based upon the current mode, but that seems unwarranted complexity unless there's a need to support endpoints which don't/won't BINARY.

Just my $0.02.

(Full disclosure: I am an engineer at VMware, and – among other things – I work on serial port emulation and vSPC support.)

[pbhenson]

Soooo.... As of python 3.11, the difference between the shipped telnetlib and the current vspc one is simply:

             while self.rawq:
                 c = self.rawq_getchar()
                 if not self.iacseq:
-                    if c == theNULL:
+                    if self.sb == 0 and c == theNULL:
                         continue
-                    if c == b"\021":
+                    if self.sb == 0 and c == "\021":
                         continue

The stock telnetlib always eats NULL, the vspc one only eats it if it's not in the middle of a subnegotiation. This seems to be a longstanding potential issue:

https://stackoverflow.com/questions/26136109/python-reading-until-null-character-from-telnet

with a workaround of overriding it to do what you wanted it to do. I assume if upstream python cared they'd have done something by now. Complicating things is it appears that telnetlib will be removed in 3.13, and currently has no maintainer:

https://peps.python.org/pep-0594/#deprecated-modules

The recommended replacements are telnetlib3:

https://pypi.org/project/telnetlib3/

or Exscript:

https://pypi.org/project/Exscript/

telnetlib3 isn't really a 1-to-1 replacement, it's a completely different API which is async. I've got no idea how much trouble it would be to make vspc use it. Exscript looks more like a tool to automate shell connections to remote systems and probably not appropriate for vspc.

Long term, this question can be addressed during a migration to another underlying telnet library, which will be a major version change expected to potentially break things for existing users. Short term, I think it works the way it is for the majority of users, and I dunno that I want to just change it at this point as a minor point release.

Reviewing the RFC you reference, I don't see any specific mention of null, so I assume you are referring to the line in section 5 "the receiver should interpret characters received from the transmitter which are not preceded with IAC as 8 bit binary data" which does I agree covers null along with any other character :). On the other hand, in section 6b, it says "The implementer of the binary transmission option should consider how (or whether) all characters are passed to a process receiving over a connection with binary transmission in effect" which kind of gives you carte blanche to do whatever you want ;), particularly as it's a SHOULD, not a MUST, in 5.

What other vspc implementations are available nowadays? The only other one I was familiar with was the commercial Avocent ACS v6000 which seems to have been discontinued. Looks like there's another python implementation:

https://opendev.org/x/vmware-vspc

Interestingly, it has its own async telnet implementation based on telnetlib. It also eats NULL except during subnegotiation, matching current behavior here.

Anyway, sorry, this is kind of a long response that basically says "I'm just gonna punt on this for now", but I didn't want to ignore the issue. Thanks for the interest and a pull request for migrating to telnetlib3 would certainly be reviewed :).

[dariusd0]

Yeah, punting on this seems reasonable.

FWIW, my interpretation of the intent of section 6 (supported by the examples throughout) is: "if the eventual receiver of the data being sent through BINARY can't handle NULL (or some other byte(s)), you're free to not pass NULL (or other byte(s)) along or to do whatever escaping/re-encoding is necessary to pass that data along", which is sort of an out, but really there seems no compelling reason we couldn't pass along the null... so... shrug.

I've been trying to track down some race conditions elsewhere in vSPC and ended up going down the same telnetlib3/Exscript rabbit-hole, and came to the same conclusion: telnetlib3 = perhaps a good portion of the way towards a rewrite, and Exscript = not at all suited for this task. Other than adjusting to the new "async" telnetlib3 implementation, other options would be either rolling the existing telnet package into vSPC or someone volunteering to maintain it in upstream python and convince them to un-deprecate it... unlikely.

I have also been investigating the OpenDev vmware-vspc project... and have a change in my local tree to undo the eating of null there too. :-)

I will let you know if I make progress with telnetlib3. At this stage I have no idea how likely that will be.

Thanks for your reply and for your efforts on this project!