From: James Gray <james@...>
Date: 2008-04-29T22:52:34+09:00
Subject: Re: Repeatable bug in Net::Telnet EOL translation


--Apple-Mail-1--758287156
Content-Type: text/plain;
	charset=US-ASCII;
	format=flowed;
	delsp=yes
Content-Transfer-Encoding: 7bit

On Apr 29, 2008, at 7:01 AM, Brian Candler wrote:

> I have found a bug in Net::Telnet - it only occurs infrequently, and
> therefore is difficult to track down when it bites you.

Thanks for reporting this.  I'm helping out with the maintenance of  
net/telnet these days, so I've spent some time this morning trying to  
understand your issue.  I think I may have a fix.

> The problem is this. Incoming telnet streams use \r\n as the line  
> separator. In normal operation (binmode=false) Net::Telnet converts  
> this to \n, which means for example you can match end-of-line using  
> $ in a regexp.
>
> However, if the \r appears at the end of one TCP segment, and the \n  
> at the start of the next segment, and the kernel passes them in two  
> separate read() calls, then this translation doesn't work. Therefore  
> sometimes you get \r\n when you were expecting \n.

I think I understand what you are saying here.  I guess we would need  
to add a check for the buffer in waitfor() ending in a \r and make  
sure we read enough content beyond that to determine the correct  
handling.  It looks like we only need one more character to cover  
"Binmode" and "Telnetmode" conversions.

I've created a patch against trunk that I believe adds this check and  
attached it to this message.  Could you please confirm that this patch  
resolves the regex matching issue you are seeing?

If anyone sees any problems with this fix, please feel free to point  
them out.

> It appears that there are some two and three-byte IAC sequences  
> which could also suffer from this.

I believe the code does handle this.  This chunk of waitfor() seems to  
keep partial sequences around for the next pass:

             elsif pt = c.rindex(/#{IAC} 
[^#{IAC}#{AO}#{AYT}#{DM}#{IP}#{NOP}]?\z/no)
               buf = preprocess(c[0 ... pt])
               rest = c[pt .. -1]

I added the new check to this section of the code.

James Edward Gray II


--Apple-Mail-1--758287156
Content-Disposition: attachment;
	filename=check_for_trailing_return_in_telnet.diff
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="check_for_trailing_return_in_telnet.diff"
Content-Transfer-Encoding: 7bit

Index: lib/net/telnet.rb
===================================================================
--- lib/net/telnet.rb	(revision 16234)
+++ lib/net/telnet.rb	(working copy)
@@ -562,7 +562,8 @@
                Integer(c.rindex(/#{IAC}#{SB}/no))
               buf = preprocess(c[0 ... c.rindex(/#{IAC}#{SB}/no)])
               rest = c[c.rindex(/#{IAC}#{SB}/no) .. -1]
-            elsif pt = c.rindex(/#{IAC}[^#{IAC}#{AO}#{AYT}#{DM}#{IP}#{NOP}]?\z/no)
+            elsif pt = c.rindex(/#{IAC}[^#{IAC}#{AO}#{AYT}#{DM}#{IP}#{NOP}]?\z/no) ||
+                       c.rindex(/\r\z/no)
               buf = preprocess(c[0 ... pt])
               rest = c[pt .. -1]
             else

--Apple-Mail-1--758287156
Content-Type: text/plain;
	charset=US-ASCII;
	format=flowed
Content-Transfer-Encoding: 7bit



--Apple-Mail-1--758287156--