Bug #87922 for POE: Random exceptions in POE::Component::Server::TCP line 302

Sun Aug 18 11:55:07 2013 jhuckaby [...] gmail.com - Ticket created

Subject:	Random exceptions in POE::Component::Server::TCP line 302
Date:	Sun, 18 Aug 2013 08:54:50 -0700
To:	bug-POE [...] rt.cpan.org
From:	Joseph Huckaby <jhuckaby [...] gmail.com>

Hello POE Authors! I am using POE::Component::Server::TCP for a simple web server, and indirectly by way of POE::Component::Server::IRC (both in the same script). I am using the latest POE version (v1.354). At random times, with seemingly no web or IRC traffic on my server at all, I am getting a full crash with this error: Can't use an undefined value as a symbol reference at /usr/local/share/perl5/POE/Component/Server/TCP.pm line 302. I was able to capture a stack trace, in case it helps: main::__ANON__('Can\'t use an undefined value as a symbol reference at /usr/l...') called at /usr/local/share/perl5/POE/Kernel.pm line 1256 POE::Kernel::_rethrow_kr_exception('POE::Kernel=ARRAY(0x2495f48)') called at /usr/local/share/perl5/POE/Kernel.pm line 1244 POE::Kernel::run('POE::Kernel=ARRAY(0x2495f48)') called at /opt/simpleirc/bin/simpleircd.pl line 240 I believe the problem may be in this bit of code here, from POE/Component/Server/TCP.pm lines 302 - 306: unless (fileno($socket)) { # TODO - Error callback? Disconnected callback? # TODO - Should we do this before starting the child? return; } I believe it is possible that $socket can be undef when this code is reached. Perhaps with a carefully constructed (i.e. bad) socket request. That would certainly help explain my random crashes. My server is publicly accessible, and does receive "noise" from the internet. I have never seen this crash on my private server running locally on my Mac. Anyway, I believe $socket should be checked before calling fileno() on it. Perhaps something like: unless ($socket && ref($socket) && fileno($socket)) { I have not been able to capture a tcpdump of this crash occurring, as it is very rare (once every few hours), and I cannot reproduce it by hand. Here is my code, in case you are interested: https://github.com/jhuckaby/simpleirc Thanks for your time, and thank you for an awesome Perl object event system! I'm using Perl v5.10.1 (*) built for x86_64-linux-thread-multi, running on an Amazon EC2 instance with this kernel: Linux 3.4.43-43.43.amzn1.x86_64 #1 SMP Mon May 6 18:04:41 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux. POE is version 1.354. - Joe

Mon Aug 19 01:08:38 2013 RCAPUTO [...] cpan.org - Correspondence added

Thank you for the detailed bug report and patch. I've applied it along with a related documentation clarification: commit bcf5d4451637bab12a2c20ccbc028d042323389b Author: Joseph Huckaby <jhuckaby@gmail.com> Date: Mon Aug 19 01:04:45 2013 -0400 [rt.cpan.org 87922] Don't crash when ClientPreConnect returns undef. ClientPreConnect should be allowed to return an undefined value to abort the client connection. Instead, it caused PoCo::Server::TCP to crash. This change catches that condition. diff --git a/lib/POE/Component/Server/TCP.pm b/lib/POE/Component/Server/TCP.pm index a4f19fb..4390cbd 100644 --- a/lib/POE/Component/Server/TCP.pm +++ b/lib/POE/Component/Server/TCP.pm @@ -299,9 +299,11 @@ sub new { my $socket = $_[ARG0]; if ($client_pre_connect) { $socket = $client_pre_connect->(@_); - unless (fileno($socket)) { - # TODO - Error callback? Disconnected callback? - # TODO - Should we do this before starting the child? + unless (defined($socket) and ref($socket) and fileno($socket)) { + # TODO - The user ought to know what's going on + # here, since it's triggered by something their + # callback has done. Should we expose a callback + # anyway to avoid potential confusion? return; } } @@ -763,15 +765,21 @@ included in C<ClientConnected>'s parameters as @_[ARG0..$#_]. ... } -C<ClientPreConnect> is called before C<ClientConnected>, and it has -different parameters: $_[ARG0] contains a copy of the client socket -before it's given to POE::Wheel::ReadWrite for management. Most HEAP -members are set, except of course $_[HEAP]{client}, because the -POE::Wheel::ReadWrite has not yet been created yet. -C<ClientPreConnect> may enable SSL on the socket, using -POE::Component::SSLify. C<ClientPreConnect> must return a valid -socket to complete the connection; the client will be disconnected if -anything else is returned. +C<ClientPreConnect> is called before C<ClientConnected>, and its +purpose is to allow programs to reject connections or condition +sockets before they're given to POE::Wheel::ReadWrite for management. + +The C<ClientPreConnect> handler is called with the client socket in +$_[ARG0], and its return value is significant. It must return a +valid client socket if the connection is acceptable. It must return +undef to reject the connection. + +Most $_[HEAP] values are valid in the C<ClientPreConnect> handler. +Obviously, $_[HEAP]{client} is not because that wheel hasn't been +created yet. + +In the following example, the C<ClientPreConnect> handler returns the +client socket after it has been upgraded to an SSL connection. sub handle_client_pre_connect {

Mon Aug 19 01:08:39 2013 The RT System itself - Status changed from 'new' to 'open'

Mon Aug 19 01:08:40 2013 RCAPUTO [...] cpan.org - Status changed from 'open' to 'resolved'

Mon Aug 19 01:17:32 2013 jhuckaby [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #87922] Random exceptions in POE::Component::Server::TCP line 302
Date:	Sun, 18 Aug 2013 22:17:17 -0700
To:	bug-POE [...] rt.cpan.org
From:	Joseph Huckaby <jhuckaby [...] gmail.com>

Oh wow, thank you for the quick fix, Rocco! And thank you for the explanation too. It makes perfect sense (in retrospect, I should have mentioned I was using SSL and ClientPreConnect). Thanks again, and have a great day! - Joe On Aug 18, 2013, at 10:08 PM, "Rocco Caputo via RT" <bug-POE@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=87922 > > > Thank you for the detailed bug report and patch. I've applied it along with a related documentation clarification: > > commit bcf5d4451637bab12a2c20ccbc028d042323389b > Author: Joseph Huckaby <jhuckaby@gmail.com> > Date: Mon Aug 19 01:04:45 2013 -0400 > > [rt.cpan.org 87922] Don't crash when ClientPreConnect returns undef. > > ClientPreConnect should be allowed to return an undefined value to > abort the client connection. Instead, it caused PoCo::Server::TCP to > crash. This change catches that condition. > > diff --git a/lib/POE/Component/Server/TCP.pm b/lib/POE/Component/Server/TCP.pm > index a4f19fb..4390cbd 100644 > --- a/lib/POE/Component/Server/TCP.pm > +++ b/lib/POE/Component/Server/TCP.pm > @@ -299,9 +299,11 @@ sub new { > my $socket = $_[ARG0]; > if ($client_pre_connect) { > $socket = $client_pre_connect->(@_); > - unless (fileno($socket)) { > - # TODO - Error callback? Disconnected callback? > - # TODO - Should we do this before starting the child? > + unless (defined($socket) and ref($socket) and fileno($socket)) { > + # TODO - The user ought to know what's going on > + # here, since it's triggered by something their > + # callback has done. Should we expose a callback > + # anyway to avoid potential confusion? > return; > } > } > @@ -763,15 +765,21 @@ included in C<ClientConnected>'s parameters as @_[ARG0..$#_]. > ... > } > > -C<ClientPreConnect> is called before C<ClientConnected>, and it has > -different parameters: $_[ARG0] contains a copy of the client socket > -before it's given to POE::Wheel::ReadWrite for management. Most HEAP > -members are set, except of course $_[HEAP]{client}, because the > -POE::Wheel::ReadWrite has not yet been created yet. > -C<ClientPreConnect> may enable SSL on the socket, using > -POE::Component::SSLify. C<ClientPreConnect> must return a valid > -socket to complete the connection; the client will be disconnected if > -anything else is returned. > +C<ClientPreConnect> is called before C<ClientConnected>, and its > +purpose is to allow programs to reject connections or condition > +sockets before they're given to POE::Wheel::ReadWrite for management. > + > +The C<ClientPreConnect> handler is called with the client socket in > +$_[ARG0], and its return value is significant. It must return a > +valid client socket if the connection is acceptable. It must return > +undef to reject the connection. > + > +Most $_[HEAP] values are valid in the C<ClientPreConnect> handler. > +Obviously, $_[HEAP]{client} is not because that wheel hasn't been > +created yet. > + > +In the following example, the C<ClientPreConnect> handler returns the > +client socket after it has been upgraded to an SSL connection. > > sub handle_client_pre_connect { >