Skip Menu |
 

This queue is for tickets about the Net-SSH-Perl CPAN distribution.

Report information
The Basics
Id: 48338
Status: resolved
Priority: 0/
Queue: Net-SSH-Perl

People
Owner: schwigon [...] cpan.org
Requestors: kevinm [...] abtech.org
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: Race condition in SSHv2
Date: Thu, 30 Jul 2009 21:02:03 -0400
To: bug-Net-SSH-Perl [...] rt.cpan.org
From: Kevin Miller <kevinm [...] abtech.org>
Download (untitled) / with headers
text/plain 1.7k
This was tested against Net-SSH-Perl-1.33, running under perl 5.8.8 on Linux-2.6c2.5-i686-64int. We have identified a race condition having to do with SSHv2 connections/channels created using the SSH2 'open2' method. The sequence of events that triggers the race condition is: 1. Client sends a message (using the open2 write handle) to the server that indicates it wishes to terminate the connection (application layer) 2. Server (application) acknowledges with a reply message, then closes the channel 3. Client (Net::SSH::Perl) processes the channel close message and marks the channel istate = CHAN_INPUT_CLOSED 4. Client (our application) parses the reply message and sends a final "\n" message to the server 5. SSH was opened using Net::SSH::Perl::SSH2::open2, so the "\n" is written via Net::SSH::Perl::Handle::SSH2 6. The Handle writes to the channel using send_data 7. send_data queues the data to the channel's input buffer 8. The channel's istate is 8 (CHAN_INPUT_CLOSED), so this buffer is never processed. However, we never exit the loop in Net::SSH::Perl::Channel::drain_outgoing, which has as the exit condition the input length being 0 It appears also that a similar race condition can occur if the order of events (from above) is: 1, 2, 4, 5, 6, 7, 3, 8. In this case, the channel is considered OPEN when the write occurs, but is subsequently set to CLOSED (in Net::SSH::Perl::Channel::rcvd_oclose) before the data can be processed for transmission. We have attempted to fix both race conditions by: * In Net::SSH::Perl::Handle::SSH2::WRITE, observe the istate of the channel and return undef if the istate is not CHAN_INPUT_OPEN * In Net::SSH::Perl::Channel::shutdown_read, flush the input buffer ($c->{input}->empty) Thanks, -Kevin
Excellent work - could you please provide a patch?
Here is a patch according to the instructions above. Can someone verify this?
--- Net/SSH/Perl/Channel.pm.orig 2009-11-24 12:31:59.000000000 -0800 +++ Net/SSH/Perl/Channel.pm 2009-11-24 12:32:44.000000000 -0800 @@ -266,6 +266,7 @@ sub shutdown_read { my $c = shift; return if $c->{type} == SSH_CHANNEL_LARVAL; + $c->{input}->empty; $c->{ssh}->debug("channel $c->{id}: close_read"); ## XXX: have to check for socket ($c->{socket}) and either --- Net/SSH/Perl/Handle/SSH2.pm.orig 2009-11-24 12:30:15.000000000 -0800 +++ Net/SSH/Perl/Handle/SSH2.pm 2009-11-24 12:31:41.000000000 -0800 @@ -43,6 +43,7 @@ my $len = length($data); while ($data) { my $chunk = substr($data, 0, CHUNK_SIZE, ''); + return undef if $h->{channel}{istate} != CHAN_INPUT_OPEN; $h->{channel}->send_data($chunk); } $len;
Download (untitled) / with headers
text/plain 472b
On Tue Nov 24 16:08:21 2009, dsrogers wrote: Show quoted text
> Here is a patch according to the instructions above. Can someone
verify this? I put the patch into a branch at https://github.com/renormalist/Net-SSH-Perl/tree/rt48338-race-condition-ssh2 but need opinions whether this patch is ok to include in a new version as I might not understand possible implications. Kind regards, Steffen -- Steffen Schwigon <ss5@renormalist.net> Dresden Perl Mongers <http://dresden-pm.org/>
Download (untitled) / with headers
text/plain 303b
On Tue Dec 04 13:27:15 2012, SCHWIGON wrote: Show quoted text
Now part of v1.36, just uploaded to CPAN. Steffen -- Steffen Schwigon <ss5@renormalist.net> Dresden Perl Mongers <http://dresden-pm.org/>


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.