Opened 15 years ago

Closed 14 years ago

#1967 defect closed fixed (fixed)

twisted.conch.telnet dies in conjunction with PuTTY

Reported by: sam Owned by:
Priority: highest Milestone: Conch-0.9
Component: conch Keywords:
Cc: therve Branch:

Description (last modified by Glyph)

twisted.conch.telnet crashes when PuTTY is connected to it on Windows XP SP2, Python 2.5b1, twisted 2.4 (but twisted.conch.telnet at rev 17656). Minimal crash example:

from twisted.conch.telnet import Telnet
from twisted.internet.protocol import Factory
from twisted.internet import reactor

f = Factory()
f.protocol = Telnet

reactor.listenTCP(2323, f)

and then from a shell with plink, PuTTY's commandline tool, visible:

$ plink -telnet -P 2323 localhost

The problem appears to originate in lines 664-674 of twisted.conch.telnet. I can't figure out why those methods are calling self.protocol.*, because there's no reference to self.protocol in the rest of the Telnet class. There is, however, an identical definition a few lines down, in the TelnetTransport class, where using self.protocol makes sense.

Changing the definition of those four functions in Telnet to "pass" would fix this problem, but I don't know if this is an acceptable solution. They are, presently, unsuitable for conventional use, so I doubt that this would break code, but it may break the defined behaviour of these methods.

Change History (15)

comment:1 Changed 15 years ago by Jean-Paul Calderone

pass is basically right, I think, although return False would be more clear. I think those implementations are a hold-over from a prior version which used fewer classes.

comment:2 Changed 15 years ago by Glyph

Is there a test that should go with this? Or is this simply functionality that shouldn't exist at all? (If so, the path where it gets invoked should be tested to make sure it doesn't blow up...)

comment:3 Changed 15 years ago by Glyph

Description: modified (diff)

comment:4 Changed 15 years ago by Jean-Paul Calderone

Keywords: review added
Owner: z3p deleted
Priority: normalhighest

Ready for review in telnet-option-negotiation-1967

comment:5 Changed 15 years ago by tjs

Keywords: review removed

makes sense to me, tests pass, works in putty, I'd possibly consider passing the docstring about when to implement the disable methods to the error being raised but other than that its all good. btw you suck, I've now done something constructive for twisted :(

comment:6 Changed 15 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [17829]) Merge telnet-option-negotiation-1967

Author: exarkun Reviewer: tjs Fixes #1967

This replaces the implementations of Telnet.{enable,disable}{Local,Remote}, which were simply broken, with implementations which reject all option negotiations. It also adds test coverage for the behavior of these base implementations.

comment:7 Changed 15 years ago by Jean-Paul Calderone

(In [19153]) Revert r17829

Refs #1967 Refs #2099

This changeset introduced a regression in untested code which manifests when accessing a conch telnet server using the NetKit telnet client.

comment:8 Changed 15 years ago by Jean-Paul Calderone

Resolution: fixed
Status: closedreopened

comment:9 Changed 15 years ago by Jean-Paul Calderone

Milestone: Conch-0.8

comment:10 Changed 14 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: reopenednew

comment:11 Changed 14 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

See final comment on #2099.

Code for this ticket is now in teloptneg-1967

It's very similar to the previous effort (although I implemented it tdd from scratch before I looked at the diff for the previous branch, which I hadn't looked at since the merge was reverted). It does omit the changes to TelnetProtocol, though, which were not necessary to make the tests for the problematic behavior, enableLocal et al raising AttributeErrors, pass. One could argue that they are incorrect and should be fixed, but one should do so with a failing unit test. ;)

Please review.

comment:12 Changed 14 years ago by therve

Cc: therve added
Keywords: review removed
Owner: set to Jean-Paul Calderone
  • Both and should have their copyright updated
  • should get a test-case-name declaration
  • unused import internet in

I've looked at the problem with #2099: the changes to TelnetProtocol break the netkit client, so it seems wise not to put it again :). There are probably some stuff missing in TelnetTransport, but that doesn't matter for now.

I've tested the behavior with Putty, and it looked good, so once the few changes above made, please merge.

comment:13 Changed 14 years ago by Jean-Paul Calderone

Done in r21296, r21297, and r21298.

comment:14 Changed 14 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [21299]) Merge teloptneg-1967

Author: exarkun Reviewer: therve Fixes #1967 Refs #2099

Change the base telnet protocol implementation, twisted.conch.telnet.Telnet, so that, by default, it cleanly rejects all attempts to enable options, rather than raising AttributeErrors which cause the connection to be lost. Also change it to raise NotImplementedError, rather than an AttributeError, to indicate a programming error if one of its disable callbacks is somehow invoked.

This allows simple Telnet subclasses which do not customize option negotiation to work when used with clients which attempt option negotiation.

comment:15 Changed 11 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.