Bug #116345 for libnet: TCP RST Issue using Net::Cmd / Net::FTP / Net::FTP::AutoReconnect

Wed Jul 20 15:21:38 2016 chris [...] neadwerx.com - Ticket created

Subject:	TCP RST Issue using Net::Cmd / Net::FTP / Net::FTP::AutoReconnect
Date:	Wed, 20 Jul 2016 15:21:07 -0400
To:	bug-libnet [...] rt.cpan.org
From:	Chris Autry <chris [...] neadwerx.com>

Greeting, we recently pulled the 3.09 update for Net::Cmd and began experiencing failures to transfer and receive files using Net::FTP and Net::FTP::AutoReconnect. After a little debugging, we found the issue to be caused in Net::Cmd: -our $VERSION = "3.08"; +our $VERSION = "3.09"; our @ISA = qw(Exporter); our @EXPORT = qw(CMD_INFO CMD_OK CMD_MORE CMD_REJECT CMD_ERROR CMD_PENDING); @@ -190,6 +190,8 @@ 1; } +sub timeout { 0 } + sub _syswrite_with_timeout { my $cmd = shift; my $line = shift; @@ -748,6 +750,12 @@ Removal of sub timeout { 0 } resolved the issue. This can be reproduced by: - Initiating an FTP connection with a remote host in passive mode (suggested setting the Debug => 1, Passive => 1 in Net::FTP constructor ). - Running a LIST / NLST / etc command (something that requires the opening of a second connection ), Net::FTP::ls() would work here. - Command will 'hang'. ( Command channel still active however ). Symptoms of the problem include the following: Error message (in Debug mode): Net::FTP: Net::Cmd::getline(): unexpected EOF on command channel: at /usr/share/perl5/vendor_perl/Net/FTP/AutoReconnect.pm line 328. Net::FTP::AutoReconnect Reconnecting to FTP server <redacted> tcpdump shows the SYN, SYN-ACK, followed by RST instead of ACK. netstat -tp | grep <remote peer> shows only command channel is active Server was ruled out by executing commands using telnet on the remote host. The following is our configuration and versions for OS, Perl, and supporting libs: - Linux 3.10.0-327.4.4.el7.x86_64 (CentOS 7) - Perl 5.16 - Net::Cmd version 3.09 - Net::FTP version 2.27 - Net::FTP::AutoReconnect version 0.3 Thank you *___________________________________Chris Autry* Nead Werx, Inc. | Senior Database Engineer 2323 Cumberland Parkway | Suite 201 | Atlanta, GA 30339 *T <redacted>* chris@neadwerx.com | http://www.neadwerx.com

Wed Jul 20 18:42:31 2016 steve.m.hay [...] googlemail.com - Correspondence added

Subject:	Re: [rt.cpan.org #116345] TCP RST Issue using Net::Cmd / Net::FTP / Net::FTP::AutoReconnect
Date:	Wed, 20 Jul 2016 23:42:19 +0100
To:	bug-libnet [...] rt.cpan.org
From:	Steve Hay <steve.m.hay [...] googlemail.com>

On 20 Jul 2016 20:21, "Chris Autry via RT" <bug-libnet@rt.cpan.org> wrote: Show quoted text

> > Wed Jul 20 15:21:38 2016: Request 116345 was acted upon. > Transaction: Ticket created by chris@neadwerx.com > Queue: libnet > Subject: TCP RST Issue using Net::Cmd / Net::FTP /

Net::FTP::AutoReconnect Show quoted text

> Broken in: (no value) > Severity: (no value) > Owner: Nobody > Requestors: chris@neadwerx.com > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=116345 > > > > Greeting, we recently pulled the 3.09 update for Net::Cmd and began > experiencing failures to transfer and receive files using Net::FTP and > Net::FTP::AutoReconnect. >

[...] Show quoted text

> > The following is our configuration and versions for OS, Perl, and > supporting libs: > > - Linux 3.10.0-327.4.4.el7.x86_64 (CentOS 7) > - Perl 5.16 > - Net::Cmd version 3.09 > - Net::FTP version 2.27 > - Net::FTP::AutoReconnect version 0.3 >

Thanks for the report. I'm curious as to why you've only pulled Net::Cmd 3.09, and still have an old Net::FTP (2.27)? Does it make any difference if you update all of libnet to 3.09?

Wed Jul 20 18:42:32 2016 The RT System itself - Status changed from 'new' to 'open'

Thu Jul 21 08:30:16 2016 gortan [...] cpan.org - Cc GORTAN added

Thu Jul 21 10:36:32 2016 chris [...] neadwerx.com - Correspondence added

Subject:	Re: [rt.cpan.org #116345] TCP RST Issue using Net::Cmd / Net::FTP / Net::FTP::AutoReconnect
Date:	Thu, 21 Jul 2016 10:35:52 -0400
To:	bug-libnet [...] rt.cpan.org
From:	Chris Autry <chris [...] neadwerx.com>

Apologies, I mis-typed the version number. We are running Net::FTP version 2.77 on the server in question. Net::FTP::AutoReconnect is as version 0.3 *___________________________________Chris Autry* Nead Werx, Inc. | Senior Database Engineer 2323 Cumberland Parkway | Suite 201 | Atlanta, GA 30339 *T 706.505.0505* chris@neadwerx.com | http://www.neadwerx.com On Wed, Jul 20, 2016 at 6:42 PM, steve.m.hay@googlemail.com via RT < bug-libnet@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=116345 > > > On 20 Jul 2016 20:21, "Chris Autry via RT" <bug-libnet@rt.cpan.org> wrote:

> > > > Wed Jul 20 15:21:38 2016: Request 116345 was acted upon. > > Transaction: Ticket created by chris@neadwerx.com > > Queue: libnet > > Subject: TCP RST Issue using Net::Cmd / Net::FTP /

> Net::FTP::AutoReconnect

> > Broken in: (no value) > > Severity: (no value) > > Owner: Nobody > > Requestors: chris@neadwerx.com > > Status: new > > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=116345 > > > > > > > Greeting, we recently pulled the 3.09 update for Net::Cmd and began > > experiencing failures to transfer and receive files using Net::FTP and > > Net::FTP::AutoReconnect. > >

> [...]

> > > > The following is our configuration and versions for OS, Perl, and > > supporting libs: > > > > - Linux 3.10.0-327.4.4.el7.x86_64 (CentOS 7) > > - Perl 5.16 > > - Net::Cmd version 3.09 > > - Net::FTP version 2.27 > > - Net::FTP::AutoReconnect version 0.3 > >

> > Thanks for the report. I'm curious as to why you've only pulled Net::Cmd > 3.09, and still have an old Net::FTP (2.27)? > > Does it make any difference if you update all of libnet to 3.09? > >

Thu Jul 21 10:47:27 2016 steve.m.hay [...] googlemail.com - Correspondence added

Subject:	Re: [rt.cpan.org #116345] TCP RST Issue using Net::Cmd / Net::FTP / Net::FTP::AutoReconnect
Date:	Thu, 21 Jul 2016 15:47:17 +0100
To:	bug-libnet [...] rt.cpan.org
From:	Steve Hay <steve.m.hay [...] googlemail.com>

On 21 Jul 2016 15:36, "Chris Autry via RT" <bug-libnet@rt.cpan.org> wrote: Show quoted text

> > Queue: libnet > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=116345 > > > Apologies, I mis-typed the version number. We are running Net::FTP

version Show quoted text

> 2.77 on the server in question. Net::FTP::AutoReconnect is as version 0.3 >

Net::FTP 2.77 is still an old version. All the libs in libnet have the same version number now, so it should be 3.09, the same as Net::Cmd.

Thu Jul 21 16:21:22 2016 chris [...] neadwerx.com - Correspondence added

Subject:	Re: [rt.cpan.org #116345] TCP RST Issue using Net::Cmd / Net::FTP / Net::FTP::AutoReconnect
Date:	Thu, 21 Jul 2016 16:20:46 -0400
To:	bug-libnet [...] rt.cpan.org
From:	Chris Autry <chris [...] neadwerx.com>

Sorry, I was mistaken. Our Net::FTP / Cmd packages were both at 3.09. We eventually downgraded to 2.29 for Net::Cmd and 2.77 for Net::FTP (These were pulled from a server which did not receive these package updates). Additionally, we are using Timeout => 30 as the only constructor option for Net::FTP. Apologies for the confusion. *___________________________________Chris Autry* Nead Werx, Inc. | Senior Database Engineer 2323 Cumberland Parkway | Suite 201 | Atlanta, GA 30339 *T 706.505.0505* chris@neadwerx.com | http://www.neadwerx.com On Thu, Jul 21, 2016 at 10:47 AM, steve.m.hay@googlemail.com via RT < bug-libnet@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=116345 > > > On 21 Jul 2016 15:36, "Chris Autry via RT" <bug-libnet@rt.cpan.org> wrote:

> > > > Queue: libnet > > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=116345 > > > > > Apologies, I mis-typed the version number. We are running Net::FTP

> version

> > 2.77 on the server in question. Net::FTP::AutoReconnect is as version 0.3 > >

> > Net::FTP 2.77 is still an old version. All the libs in libnet have the same > version number now, so it should be 3.09, the same as Net::Cmd. > >

Fri Jul 22 03:52:53 2016 shay [...] cpan.org - Correspondence added

On Thu Jul 21 16:21:22 2016, chris@neadwerx.com wrote: Show quoted text

> Sorry, I was mistaken. Our Net::FTP / Cmd packages were both at 3.09. > We > eventually downgraded to 2.29 for Net::Cmd and 2.77 for Net::FTP > (These > were pulled from a server which did not receive these package > updates). > Additionally, we are using Timeout => 30 as the only constructor > option for > Net::FTP. > > Apologies for the confusion. >

Are you able to supply a script that reproduces the failure? I've followed the steps you gave earlier to reproduce the problem using a random FTP server, but it works as expected: test.pl ------- use strict; use warnings; use Net::FTP; print "Using Net::FTP $Net::FTP::VERSION\n"; my $ftp = Net::FTP->new( Host => 'mirrors.uk2.net', Passive => 1, Timeout => 30) or die $@; $ftp->login('anonymous', undef); print join("\n", $ftp->ls()), "\n"; $ftp->quit(); print "OK\n"; Output: ------- C:\Dev\Temp>perl test.pl Using Net::FTP 3.09 debian hirensbootcd pub ubuntu ubuntu-releases OK

Fri Jul 22 09:11:54 2016 chris [...] neadwerx.com - Correspondence added

CC:	gortan [...] cpan.org
Subject:	Re: [rt.cpan.org #116345] TCP RST Issue using Net::Cmd / Net::FTP / Net::FTP::AutoReconnect
Date:	Fri, 22 Jul 2016 09:11:14 -0400
To:	bug-libnet [...] rt.cpan.org
From:	Chris Autry <chris [...] neadwerx.com>

Give this a shot. I tried it on the peers we are having issues with, as well as a public Speedtest server I found: #!/usr/bin/perl $|=1; use strict; use warnings; use Carp; use Net::FTP; use Data::Dumper(); my $cmd_version = $Net::FTP::VERSION; my $ftp_version = $Net::FTP::VERSION; print "Net::CMD Version: $cmd_version\n"; print "Net::FTP Version: $ftp_version\n"; my $ftp = Net::FTP->new( 'speedtest.tele2.net', Timeout => 30, Passive => 1, Debug => 1 ); unless( $ftp ) { croak 'Failed to create new FTP object'; } unless( $ftp->login( 'anonymous', 'anonymous' ) ) { croak 'Unable to connect'; } print "Connected.\n"; $ftp->binary; $ftp->cwd( '/' ); my @files = $ftp->ls(); print "Got files\n"; print Dumper( @files ); $ftp->quit; print "Done\n"; exit 0; Output: Net::CMD Version: 3.09 Net::FTP Version: 3.09 Net::FTP>>> Net::FTP(3.09) Net::FTP>>> Exporter(5.68) Net::FTP>>> Net::Cmd(3.09) Net::FTP>>> IO::Socket::IP(0.37) Net::FTP>>> IO::Socket(1.34) Net::FTP>>> IO::Handle(1.33) Net::FTP=GLOB(0x12e6a20)<<< 220 (vsFTPd 2.3.5) Net::FTP=GLOB(0x12e6a20)>>> USER anonymous Net::FTP=GLOB(0x12e6a20)<<< 331 Please specify the password. Net::FTP=GLOB(0x12e6a20)>>> PASS .... Net::FTP=GLOB(0x12e6a20)<<< 230 Login successful. Connected. Net::FTP=GLOB(0x12e6a20)>>> TYPE I Net::FTP=GLOB(0x12e6a20)<<< 200 Switching to Binary mode. Net::FTP=GLOB(0x12e6a20)>>> CWD / Net::FTP=GLOB(0x12e6a20)<<< 250 Directory successfully changed. Net::FTP=GLOB(0x12e6a20)>>> PASV Net::FTP=GLOB(0x12e6a20)<<< 227 Entering Passive Mode (90,130,70,73,115,229). Net::FTP=GLOB(0x12e6a20)>>> NLST <hangs> The speedtest's / directory should have a little over a dozen files ranging from 1KB to 1000GB in size. *___________________________________Chris Autry* Nead Werx, Inc. | Senior Database Engineer 2323 Cumberland Parkway | Suite 201 | Atlanta, GA 30339 *T 706.505.0505* chris@neadwerx.com | http://www.neadwerx.com On Fri, Jul 22, 2016 at 3:52 AM, Steve Hay via RT <bug-libnet@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=116345 > > > On Thu Jul 21 16:21:22 2016, chris@neadwerx.com wrote:

> > Sorry, I was mistaken. Our Net::FTP / Cmd packages were both at 3.09. > > We > > eventually downgraded to 2.29 for Net::Cmd and 2.77 for Net::FTP > > (These > > were pulled from a server which did not receive these package > > updates). > > Additionally, we are using Timeout => 30 as the only constructor > > option for > > Net::FTP. > > > > Apologies for the confusion. > >

> > Are you able to supply a script that reproduces the failure? I've followed > the steps you gave earlier to reproduce the problem using a random FTP > server, but it works as expected: > > test.pl > ------- > use strict; > use warnings; > use Net::FTP; > print "Using Net::FTP $Net::FTP::VERSION\n"; > my $ftp = Net::FTP->new( > Host => 'mirrors.uk2.net', > Passive => 1, > Timeout => 30) > or die $@; > $ftp->login('anonymous', undef); > print join("\n", $ftp->ls()), "\n"; > $ftp->quit(); > print "OK\n"; > > Output: > ------- > C:\Dev\Temp>perl test.pl > Using Net::FTP 3.09 > debian > hirensbootcd > pub > ubuntu > ubuntu-releases > OK > >

Fri Jul 22 13:17:56 2016 shay [...] cpan.org - Correspondence added

On Fri Jul 22 09:11:54 2016, chris@neadwerx.com wrote: Show quoted text

> Give this a shot. I tried it on the peers we are having issues with, as > well as a public Speedtest server I found: > > #!/usr/bin/perl > > $|=1; > > use strict; > use warnings; > > use Carp; > use Net::FTP; > use Data::Dumper(); > > my $cmd_version = $Net::FTP::VERSION; > my $ftp_version = $Net::FTP::VERSION; > > print "Net::CMD Version: $cmd_version\n"; > print "Net::FTP Version: $ftp_version\n"; > > my $ftp = Net::FTP->new( > 'speedtest.tele2.net', > Timeout => 30, > Passive => 1, > Debug => 1 > ); > > unless( $ftp ) > { > croak 'Failed to create new FTP object'; > } > > unless( $ftp->login( 'anonymous', 'anonymous' ) ) > { > croak 'Unable to connect'; > } > > print "Connected.\n"; > > $ftp->binary; > $ftp->cwd( '/' ); > > my @files = $ftp->ls(); > > print "Got files\n"; > print Dumper( @files ); > > $ftp->quit; > print "Done\n"; > exit 0; > > > Output: > Net::CMD Version: 3.09 > Net::FTP Version: 3.09 > Net::FTP>>> Net::FTP(3.09) > Net::FTP>>> Exporter(5.68) > Net::FTP>>> Net::Cmd(3.09) > Net::FTP>>> IO::Socket::IP(0.37) > Net::FTP>>> IO::Socket(1.34) > Net::FTP>>> IO::Handle(1.33) > Net::FTP=GLOB(0x12e6a20)<<< 220 (vsFTPd 2.3.5) > Net::FTP=GLOB(0x12e6a20)>>> USER anonymous > Net::FTP=GLOB(0x12e6a20)<<< 331 Please specify the password. > Net::FTP=GLOB(0x12e6a20)>>> PASS .... > Net::FTP=GLOB(0x12e6a20)<<< 230 Login successful. > Connected. > Net::FTP=GLOB(0x12e6a20)>>> TYPE I > Net::FTP=GLOB(0x12e6a20)<<< 200 Switching to Binary mode. > Net::FTP=GLOB(0x12e6a20)>>> CWD / > Net::FTP=GLOB(0x12e6a20)<<< 250 Directory successfully changed. > Net::FTP=GLOB(0x12e6a20)>>> PASV > Net::FTP=GLOB(0x12e6a20)<<< 227 Entering Passive Mode > (90,130,70,73,115,229). > Net::FTP=GLOB(0x12e6a20)>>> NLST > <hangs> > > The speedtest's / directory should have a little over a dozen files ranging > from 1KB to 1000GB in size. > >

That works fine for me (with a fix to import the Dumper function from Data::Dumper): C:\Dev\Temp>perl test2.pl Net::CMD Version: 3.09 Net::FTP Version: 3.09 Net::FTP>>> Net::FTP(3.09) Net::FTP>>> Exporter(5.71) Net::FTP>>> Net::Cmd(3.09) Net::FTP>>> IO::Socket::SSL(2.012) Net::FTP>>> IO::Socket::IP(0.29) Net::FTP>>> IO::Socket(1.38) Net::FTP>>> IO::Handle(1.35) Net::FTP=GLOB(0x2a2d9a8)<<< 220 (vsFTPd 2.3.5) Net::FTP=GLOB(0x2a2d9a8)>>> USER anonymous Net::FTP=GLOB(0x2a2d9a8)<<< 331 Please specify the password. Net::FTP=GLOB(0x2a2d9a8)>>> PASS .... Net::FTP=GLOB(0x2a2d9a8)<<< 230 Login successful. Connected. Net::FTP=GLOB(0x2a2d9a8)>>> TYPE I Net::FTP=GLOB(0x2a2d9a8)<<< 200 Switching to Binary mode. Net::FTP=GLOB(0x2a2d9a8)>>> CWD / Net::FTP=GLOB(0x2a2d9a8)<<< 250 Directory successfully changed. Net::FTP=GLOB(0x2a2d9a8)>>> PASV Net::FTP=GLOB(0x2a2d9a8)<<< 227 Entering Passive Mode (90,130,70,73,116,239). Net::FTP=GLOB(0x2a2d9a8)>>> NLST Net::FTP=GLOB(0x2a2d9a8)<<< 150 Here comes the directory listing. Net::FTP=GLOB(0x2a2d9a8)<<< 226 Directory send OK. Got files $VAR1 = '1000GB.zip'; $VAR2 = '100GB.zip'; $VAR3 = '100KB.zip'; $VAR4 = '100MB.zip'; $VAR5 = '10GB.zip'; $VAR6 = '10MB.zip'; $VAR7 = '1GB.zip'; $VAR8 = '1KB.zip'; $VAR9 = '1MB.zip'; $VAR10 = '200MB.zip'; $VAR11 = '20MB.zip'; $VAR12 = '2MB.zip'; $VAR13 = '3MB.zip'; $VAR14 = '500MB.zip'; $VAR15 = '50MB.zip'; $VAR16 = '512KB.zip'; $VAR17 = '5MB.zip'; $VAR18 = 'upload'; Net::FTP=GLOB(0x2a2d9a8)>>> QUIT Net::FTP=GLOB(0x2a2d9a8)<<< 221 Goodbye. Done

Sun Jul 24 14:55:53 2016 gortan [...] cpan.org - Correspondence added

On Fri Jul 22 09:11:54 2016, chris@neadwerx.com wrote: Show quoted text

> Give this a shot. I tried it on the peers we are having issues with, as > well as a public Speedtest server I found:

... Show quoted text

> <hangs>

We've seen this issue as well in $work after upgrading Net::FTP to 3.09. I was also just able to reproduce the issue on Arch Linux with perl 5.24.0 on x86_64 on a clean setup (e.g., ~/perl5 cleaned). A bit of bisecting showed that the timeouts started with this commit: 91e593b615334fa76ef0454c4e601b98b6663841 is the first bad commit commit 91e593b615334fa76ef0454c4e601b98b6663841 Author: Steve Hay <steve.m.hay@googlemail.com> Date: Thu Jul 7 08:39:04 2016 +0100 Provide (and document) a default Net::Cmd::timeout() Previuosly, subclasses were required to provide a timeout() function, but this was not documented anywhere! Fixes CPAN RT#110978.

Sun Jul 24 16:18:43 2016 gortan [...] cpan.org - Correspondence added

Out of curiosity, I rebased 91e593b615334fa76ef0454c4e601b98b6663841 onto a commit before dagolden's fix-syswrite - the issue remains, so it's not something introduced by this cleanup.

Wed Jul 27 04:02:27 2016 ppisar [...] redhat.com - Correspondence added

From:

ppisar [...] redhat.com

Dne St 20.čec.2016 15:21:38, chris@neadwerx.com napsal(a): Show quoted text

> Greeting, we recently pulled the 3.09 update for Net::Cmd and began > experiencing failures to transfer and receive files using Net::FTP and > Net::FTP::AutoReconnect. >

I can reproduce it with both tests (Fedora 24). It blocks in Net::Cmd::getline() on this line: my $select_ret = select($rout = $rin, undef, undef, $timeout); It's because if the Net::Cmd::timeout() subroutine is defined to return 0 (as introduced by the commit), the $timeout variable get undef regardless what Timeout was supplied to Net::FTP->new(). It's undef because the until loop starts with: my $timeout = $cmd->timeout || undef; and $cmd->timeout returns 0 as written in the Net::Cmd::timeout(). It should return the Timeout from Net::FTP->new() or a default one which Net::FTP hard-codes to 120.

Wed Jul 27 04:47:38 2016 ppisar [...] redhat.com - Correspondence added

From:

ppisar [...] redhat.com

Dne St 27.čec.2016 04:02:27, ppisar napsal(a): Show quoted text

> and $cmd->timeout returns 0 as written in the Net::Cmd::timeout(). It > should return the Timeout from Net::FTP->new() or a default one which > Net::FTP hard-codes to 120.

If Net::Cmd::timeout() does not exist, the $cmd->timout resolves to IO::Socket::IP::timeout that carry the Timeout value. It looks like a wrong precedence of classes in method resolution.

Wed Jul 27 05:01:08 2016 ppisar [...] redhat.com - Correspondence added

From:

ppisar [...] redhat.com

Dne St 27.čec.2016 04:47:38, ppisar napsal(a): Show quoted text

> It looks like a wrong precedence of classes in method resolution.

Net::FTP does: our @ISA = ('Exporter','Net::Cmd',$IOCLASS); because Net::Cmd::getline() should override $IOCLASS::getline(). For the same reason, the Net::Cmd::timeout() takes precedence over $IOCLASS::timeout().

Wed Jul 27 07:29:22 2016 ppisar [...] redhat.com - Correspondence added

From:

ppisar [...] redhat.com

Attached patch fixes the regression by defining timeout() method in each subclass as requested by Net::Cmd documentation. However, I'm not sure this the best way. Maybe the Net::Cmd::timeout() should be reverted and the documentation changed to require the base class to be IO::Socket::INET-like instead of IO::Handle.

Subject:

0001-Override-timeout-method-in-Net-FTP-and-other-subclas.patch

From 72d07cb0d6a2e32a6a18a1794a592a8f6bf71665 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= <ppisar@redhat.com> Date: Wed, 27 Jul 2016 12:57:23 +0200 Subject: [PATCH] Override timeout method in Net::FTP and other subclasses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After adding Net::Cmd::timeout() in commit 91e593b615334fa76ef0454c4e601b98b6663841, the method masked IO::Socket:IP::timeout() in Net::FTP objects causing infitive block in select() on a FTP connection. CPAN RT#116345 Signed-off-by: Petr PÃsaÅ <ppisar@redhat.com> --- lib/Net/FTP.pm | 5 +++++ lib/Net/NNTP.pm | 6 ++++++ lib/Net/POP3.pm | 5 +++++ lib/Net/SMTP.pm | 5 +++++ 4 files changed, 21 insertions(+) diff --git a/lib/Net/FTP.pm b/lib/Net/FTP.pm index 905d830..42c33f5 100644 --- a/lib/Net/FTP.pm +++ b/lib/Net/FTP.pm @@ -1210,6 +1210,11 @@ sub _data_cmd { ## +sub timeout { + $IOCLASS->can('timeout')->(@_); +} + + sub debug_text { $_[2] =~ /^(pass|resp|acct)/i ? "$1 ....\n" : $_[2]; } diff --git a/lib/Net/NNTP.pm b/lib/Net/NNTP.pm index 764d580..f1b5c17 100644 --- a/lib/Net/NNTP.pm +++ b/lib/Net/NNTP.pm @@ -419,6 +419,12 @@ sub slave { $nntp->_SLAVE; } + +sub timeout { + $ISA[-1]->can('timeout')->(@_); +} + + ## ## The following methods are not implemented by all servers ## diff --git a/lib/Net/POP3.pm b/lib/Net/POP3.pm index bb18aaf..7f1cf5a 100644 --- a/lib/Net/POP3.pm +++ b/lib/Net/POP3.pm @@ -387,6 +387,11 @@ sub quit { } +sub timeout { + $ISA[-1]->can('timeout')->(@_); +} + + sub DESTROY { my $me = shift; diff --git a/lib/Net/SMTP.pm b/lib/Net/SMTP.pm index 0dd966f..895c884 100644 --- a/lib/Net/SMTP.pm +++ b/lib/Net/SMTP.pm @@ -586,6 +586,11 @@ sub quit { } +sub timeout { + $ISA[-1]->can('timeout')->(@_); +} + + sub DESTROY { # ignore -- 2.5.5

Thu Jul 28 04:18:16 2016 shay [...] cpan.org - Correspondence added

On Wed Jul 27 07:29:22 2016, ppisar wrote: Show quoted text

> Attached patch fixes the regression by defining timeout() method in > each subclass as requested by Net::Cmd documentation. > > However, I'm not sure this the best way. Maybe the Net::Cmd::timeout() > should be reverted and the documentation changed to require the base > class to be IO::Socket::INET-like instead of IO::Handle.

Thanks for the analysis and patch. I can get my test.pl script above to fail with a Timeout if I remove IO::Socket::IP and IO::Socket::SSL from my perl installation, and I see why now. I'm tempted to revert the timeout() addition and update documentation as you say, but returning to your earlier https://rt.cpan.org/Ticket/Display.html?id=110978 (where all this started), that would require changing Net::SNPP::Server to inherit from IO::Socket::INET instead of IO::Handle in order to avoid reintroducing the #110978 bug. I haven't tested whether that really does work in practice, but I'm also concerned that there could be other users of Net::Cmd that would have to make similar changes. Providing a default timeout() in Net::Cmd itself seemed like a friendlier approach than imposing a requirement for such a change on all users, although (as this ticket shows) breakage can still occur if that default isn't suitable, which then still requires changes in the users of Net::Cmd to fix (namely, the addition of a suitable timeout() override, as per your patch)! So I'm torn which way to jump here. Any other opinions?

Thu Jul 28 08:35:41 2016 shay [...] cpan.org - Correspondence added

On Thu Jul 28 04:18:16 2016, SHAY wrote: Show quoted text

> On Wed Jul 27 07:29:22 2016, ppisar wrote:

> > Attached patch fixes the regression by defining timeout() method in > > each subclass as requested by Net::Cmd documentation. > > > > However, I'm not sure this the best way. Maybe the > > Net::Cmd::timeout() > > should be reverted and the documentation changed to require the base > > class to be IO::Socket::INET-like instead of IO::Handle.

> > Thanks for the analysis and patch. I can get my test.pl script above > to fail with a Timeout if I remove IO::Socket::IP and IO::Socket::SSL > from my perl installation, and I see why now. > > I'm tempted to revert the timeout() addition and update documentation > as you say, but returning to your earlier > https://rt.cpan.org/Ticket/Display.html?id=110978 (where all this > started), that would require changing Net::SNPP::Server to inherit > from IO::Socket::INET instead of IO::Handle in order to avoid > reintroducing the #110978 bug. I haven't tested whether that really > does work in practice, but I'm also concerned that there could be > other users of Net::Cmd that would have to make similar changes. > > Providing a default timeout() in Net::Cmd itself seemed like a > friendlier approach than imposing a requirement for such a change on > all users, although (as this ticket shows) breakage can still occur if > that default isn't suitable, which then still requires changes in the > users of Net::Cmd to fix (namely, the addition of a suitable timeout() > override, as per your patch)! > > So I'm torn which way to jump here. Any other opinions?

After searching around on grep.cpan.me it appears that almost all (public) users of Net::Cmd are also sub-classes of IO::Socket::INET anyway, including at least all of these: Net::APP, Net::Dict, Net::LMTP, Net::PH, Net::RGTP, Net::SNPP, NNML::Handle, Ponfish::News::MyNNTP, Roku::RCP, WAIT::Client::HTTP::Handle, WAIT::Handle and What::MTA. In fact, Net::SNPP::Server is the only one I've found that chooses IO::Handle instead. So I'm going to revert the Net::Cmd::timeout() change and document that Net::Cmd sub-classes are expected to provide the methods of IO::Socket::INET (either by doing so explicitly themselves, or by also deriving from IO::Socket::INET (or ::IP or ::INET6 or ::SSL)). That does mean that #110978 will be re-introduced, and Net::SNPP::Server will now have to change to fix it, but I think that's probably for the best since it will bring it into line with what most other users of Net::Cmd are doing already (including Net::SNPP in the same distro!).

Thu Jul 28 09:24:19 2016 shay [...] cpan.org - Correspondence added

On Thu Jul 28 08:35:41 2016, SHAY wrote: Show quoted text

> On Thu Jul 28 04:18:16 2016, SHAY wrote:

> > On Wed Jul 27 07:29:22 2016, ppisar wrote:

> > > Attached patch fixes the regression by defining timeout() method in > > > each subclass as requested by Net::Cmd documentation. > > > > > > However, I'm not sure this the best way. Maybe the > > > Net::Cmd::timeout() > > > should be reverted and the documentation changed to require the > > > base > > > class to be IO::Socket::INET-like instead of IO::Handle.

> > > > Thanks for the analysis and patch. I can get my test.pl script above > > to fail with a Timeout if I remove IO::Socket::IP and IO::Socket::SSL > > from my perl installation, and I see why now. > > > > I'm tempted to revert the timeout() addition and update documentation > > as you say, but returning to your earlier > > https://rt.cpan.org/Ticket/Display.html?id=110978 (where all this > > started), that would require changing Net::SNPP::Server to inherit > > from IO::Socket::INET instead of IO::Handle in order to avoid > > reintroducing the #110978 bug. I haven't tested whether that really > > does work in practice, but I'm also concerned that there could be > > other users of Net::Cmd that would have to make similar changes. > > > > Providing a default timeout() in Net::Cmd itself seemed like a > > friendlier approach than imposing a requirement for such a change on > > all users, although (as this ticket shows) breakage can still occur > > if > > that default isn't suitable, which then still requires changes in the > > users of Net::Cmd to fix (namely, the addition of a suitable > > timeout() > > override, as per your patch)! > > > > So I'm torn which way to jump here. Any other opinions?

> > After searching around on grep.cpan.me it appears that almost all > (public) users of Net::Cmd are also sub-classes of IO::Socket::INET > anyway, including at least all of these: Net::APP, Net::Dict, > Net::LMTP, Net::PH, Net::RGTP, Net::SNPP, NNML::Handle, > Ponfish::News::MyNNTP, Roku::RCP, WAIT::Client::HTTP::Handle, > WAIT::Handle and What::MTA. In fact, Net::SNPP::Server is the only one > I've found that chooses IO::Handle instead. > > So I'm going to revert the Net::Cmd::timeout() change and document > that Net::Cmd sub-classes are expected to provide the methods of > IO::Socket::INET (either by doing so explicitly themselves, or by also > deriving from IO::Socket::INET (or ::IP or ::INET6 or ::SSL)). > > That does mean that #110978 will be re-introduced, and > Net::SNPP::Server will now have to change to fix it, but I think > that's probably for the best since it will bring it into line with > what most other users of Net::Cmd are doing already (including > Net::SNPP in the same distro!).

Now done in commit 95330178b26b96232dddb188f82ad33da0d0b293, which will be in 3.10 shortly. Thanks to all for reporting/helping fix this.

Thu Jul 28 09:24:25 2016 shay [...] cpan.org - Status changed from 'open' to 'resolved'