| Date: | Sun, 02 Oct 2005 02:06:10 -0400 |
| From: | Tom Metro <tmetro [...] cpan.org> |
| To: | bug-libnet [...] rt.cpan.org |
| Subject: | Net::Cmd returns obsolete error codes after connection failure |
While using Net::SMTP in a proxy server, I observed that under certain
circumstances Net::SMTP was returning the error code from the previous
command when the current command failed.
A specific scenario in which this might occur is when the server
Net::SMTP was talking to drops the connection during the DATA command
state, resulting in the dataend() method failing. At this point if you
call code() or message() methods, they will return the values set by the
data() method, which is normally 354. In a proxy server context, when
the client machine see 354 in response to "." (dataend) (instead of the
expected 250), it considers it a protocol error, and in the case of
sendmail, also considers it a permanent failure and bounces the message.
I tracked the problem down to the internals of Net::Cmd and confirmed
the problem still exists in the current version (2.26).
I've created a test server and client (attached) to demonstrate this
problem. First I start the server:
% perl crash_server.pl 1
Passing anything on the command line activates the "crash mode," which
causes the server to intentionally crash (exit) during the dataend state.
Then the client:
% perl -MCarp=verbose client.pl
CMD: banner
Net::SMTP>>> Net::SMTP(2.26)
Net::SMTP>>> Net::Cmd(2.26)
Net::SMTP>>> Exporter(5.58)
Net::SMTP>>> IO::Socket::INET(1.27)
Net::SMTP>>> IO::Socket(1.28)
Net::SMTP>>> IO::Handle(1.24)
Net::SMTP=GLOB(0x1b6dda4)<<< 220 [A] crash-server
CMD: helo example.com
Net::SMTP=GLOB(0x1b6dda4)>>> EHLO example.com
Net::SMTP=GLOB(0x1b6dda4)<<< 500 Syntax error: unrecognized command
Net::SMTP=GLOB(0x1b6dda4)>>> HELO example.com
Net::SMTP=GLOB(0x1b6dda4)<<< 250 [B] helo example.com
CMD: mail from <user@example.com>
Net::SMTP=GLOB(0x1b6dda4)>>> MAIL FROM:<user@example.com>
Net::SMTP=GLOB(0x1b6dda4)<<< 250 [C] mail from <user@example.com>
CMD: rcpt to <user@example.com>
Net::SMTP=GLOB(0x1b6dda4)>>> RCPT TO:<user@example.com>
Net::SMTP=GLOB(0x1b6dda4)<<< 250 [D] rcpt to <user@example.com>
CMD: data_init
Net::SMTP=GLOB(0x1b6dda4)>>> DATA
Net::SMTP=GLOB(0x1b6dda4)<<< 354 [E] data_init
CMD: data_part
Net::SMTP=GLOB(0x1b6dda4)>>> data data data data data data data data
data data
CMD: data_end
Net::SMTP=GLOB(0x1b6dda4)>>> .
Net::SMTP: Unexpected EOF on command channel at
C:/bin/dev/Perl/5.8.4/lib/Net/Cmd.pm line 273
Net::Cmd::getline('Net::SMTP=GLOB(0x1b6dda4)') called at
C:/bin/dev/Perl/5.8.4/lib/Net/Cmd.pm line 332
Net::Cmd::response('Net::SMTP=GLOB(0x1b6dda4)') called at
C:/bin/dev/Perl/5.8.4/lib/Net/Cmd.pm line 530
Net::Cmd::dataend('Net::SMTP=GLOB(0x1b6dda4)') called at
client.pl line 42
Client returned: 354 [E] data_init
What's important to note is that you see:
Net::SMTP=GLOB(0x1b6dda4)<<< 354 [E] data_init
in response to the DATA command, and then the same values returned from
code() and message() after the dataend() method fails:
Client returned: 354 [E] data_init
Reviewing Net::Cmd, it appears to have some problems with how the object
properties accessed by code() and message() are initialized. Currently
they're initialized near the end of command():
sub command
{
[...]
${*$cmd}{'net_cmd_resp'} = []; # the response
${*$cmd}{'net_cmd_code'} = "000"; # Made this one up :-)
}
$cmd;
}
which has two problems: 1. things can happen to cause command() to exit
before it gets to that point, and 2. this seems like it should really be
in response(), which can be called independent of command().
Looking at response() we see:
sub response
{
my $cmd = shift;
my($code,$more) = (undef) x 2;
${*$cmd}{'net_cmd_resp'} ||= [];
[...]
which suggests that response() can be called iteratively to build up a
multi-line response, yet it has internal logic that contradicts that.
The internal logic says that it won't return until all of the lines in a
multi-line response are received. My best guess is that this construct
was unintentionally copied from getline(), which has a similar
conditional initialization of its buffer, but there it makes sense.
response() also makes no attempt to initialize ${*$cmd}{'net_cmd_code'},
yet has failure modes in which it will return prior to setting it.
More generally, internal error handling is not done consistently. At the
top of command() we see:
unless (defined fileno($cmd))
{
$cmd->set_status("599", "Connection closed");
return $cmd;
}
which seems like a good approach, though this is the only place in the
code where a pseudo (internally generated) result code is used. Many
other places in which the code tests for a closed file handle or IO
errors it simply returns without communicating that the failure was
internal, and leaves the code and message properties in an undetermined
state.
The same situation is true for trapped timeouts, as mentioned in bug #9394.
(On a side note, I'd argue that "599" - a permanent failure - is not a
good choice for this purpose. Especially when used as part of a proxy
server such that the code gets passed on to a client. I propose 421,
which signifies a temporary connection failure.)
It may be argued (though not documented as such) that the return values
from code() or message() methods should not be used when a method fails,
but that logic doesn't hold, because the cause of a method returning
false more typically is due to the remote server returning an error
code. So the normal reaction to a failed method (in calling code) is
going to be examining the result code.
With some clean up in the initialization code, it could be made so that
Net::Cmd consistently returns "000" for internal errors, which would
have the advantage of distinguishing itself from remote server errors
(given that "000" is out of range for protocol errors), but that then
precludes being able to pass on the code in a proxy scenario, and it
requires that the calling code introduce a special case to handle it.
It seems the best solution is to embrace the idea of pseudo result
codes, and update the code to consistently set the code and message
properties when internal IO errors are trapped.
I've created a patch to do just that, and I'll post it as a follow up to
this message after it has undergone some testing.
-Tom
use strict;
use Net::SMTP;
warn "CMD: banner\n";
my $smtpc = Net::SMTP->new('localhost',
Port => 9025,
Hello => undef,
Debug => 99
) or do {
die q/connection failed/;
};
my $hostname = 'example.com';
warn "CMD: helo $hostname\n";
$smtpc->hello($hostname) or do {
client_error($smtpc);
};
my $address = "<user\@$hostname>";
warn "CMD: mail from $address\n";
$smtpc->mail($address) or do {
client_error($smtpc);
};
warn "CMD: rcpt to $address\n";
$smtpc->recipient($address) or do {
client_error($smtpc);
};
warn "CMD: data_init\n";
$smtpc->data() or do {
client_error($smtpc);
};
my $chunk = 'data ' x 10 . "\n";
warn "CMD: data_part\n";
$smtpc->datasend([$chunk]) or do {
client_error($smtpc);
};
warn "CMD: data_end\n";
$smtpc->dataend() or do {
client_error($smtpc);
};
warn "CMD: quit\n";
$smtpc->quit() or do {
client_error($smtpc);
};
sub client_error {
my $client = shift;
die "Client returned: ",$client->code()," ",$client->message(),"\n";
}
use strict;
use Net::Server::Mail::SMTP;
use IO::Socket::INET;
# pass 1 (or anything true) on the command line to invoke
# "crash mode"
my $crash = shift;
my $server = new IO::Socket::INET Listen => 1, LocalPort => 9025;
warn "Listening on port 9025\n";
warn "Crash mode enabled\n" if $crash;
my $conn;
while ($conn = $server->accept) {
warn "Connected to: ",$conn->peerhost(),"\n";
my $smtpd = new Net::Server::Mail::SMTP socket => $conn;
$smtpd->set_callback('banner' => \&banner);
$smtpd->set_callback('HELO' => \&helo);
$smtpd->set_callback('MAIL' => \&mail);
$smtpd->set_callback('RCPT' => \&rcpt);
$smtpd->set_callback('DATA-INIT' => \&data_init);
$smtpd->set_callback('DATA-PART' => \&data_part);
$smtpd->set_callback('DATA' => \&data_end);
$smtpd->set_callback('QUIT' => \&quit);
warn "Entering SMTP processing loop...\n";
$smtpd->process;
# cleanup
$smtpd = undef;
$conn = undef;
}
exit(0);
## subs
sub banner {
my $session = shift;
warn "CMD: banner\n";
return(0, 220, '[A] crash-server');
}
sub helo {
my $session = shift;
my $hostname = shift;
warn "CMD: helo $hostname\n";
return (1,250, "[B] helo $hostname");
}
sub mail {
my $session = shift;
my $address = shift;
warn "CMD: mail from $address\n";
return (1, 250, "[C] mail from $address");
}
sub rcpt {
my $session = shift;
my $address = shift;
warn "CMD: rcpt to $address\n";
return (1, 250, "[D] rcpt to $address");
}
sub data_init {
my $session = shift;
warn "CMD: data_init\n";
return (1, 354, '[E] data_init');
}
sub data_part {
my $session = shift;
my $chunk = shift; # ref to buffer
warn "CMD: data_part\n";
if ($crash) {
warn "** crashing **\n";
exit(0);
}
return (1);
}
sub data_end {
my $session = shift;
warn "CMD: data_end\n";
return (1, 250, '[F] data_end');
}
sub quit {
my $session = shift;
warn "CMD: quit\n";
return (1, 221, '[G] quit');
}