Bug #62106 for MailTools: Mail/Mailer/smtp error handling

Wed Oct 13 11:26:50 2010 tfm-cpanbugs [...] earth.li - Ticket created

Subject:	Mail/Mailer/smtp error handling
Date:	Wed, 13 Oct 2010 16:26:42 +0100
To:	bug-MailTools [...] rt.cpan.org
From:	Tristam Fenton-May <tfm-cpanbugs [...] earth.li>

I think that I have run into a bug/limitation in the error handling in Mail/Mailer/smtp.pm The problem is that if the SMTP mailer gives an error, or disconnects after the DATA command, but before accepting the mail the close function still returns true indicating that the mail was delivered correctly. The close function calls epilogue which in turn calls dataend on the socket, which will return false in the case of these kinds of errors, but the return value is ignored. I have attached a patch for smtp.pm which passes the error out of epilogue and then out of close, it also returns 0 in the case that the socket is already invalid when it tried to do this, I am not certain that this is correct, but assume that close() should return an error if there has been some previous error (e.g. TCP disconnect during the sending of the data?) I believe that smtps.pm has the same issue, and a similar fix could be applied, but I have not tested that. -- TFM

Wed Oct 13 11:35:44 2010 tfm-bitcard [...] earth.li - Correspondence added

From:

tfm-bitcard [...] earth.li

Appologies - I forgot to attach the patch to my bug report.

Subject:

MailTools-2.07_tfmErrorHandling.patch

--- MailTools-2.07-orig/lib/Mail/Mailer/smtp.pm 2010-10-13 16:12:30.250000000 +0100 +++ MailTools-2.07/lib/Mail/Mailer/smtp.pm 2010-10-13 16:13:54.781250000 +0100 @@ -57,11 +57,12 @@ { my $self = shift; my $sock = ${*$self}{sock}; - $sock->dataend; + my $ok = $sock->dataend; $sock->quit; delete ${*$self}{sock}; untie *$self; + $ok; } sub close(@) @@ -69,19 +70,19 @@ my $sock = ${*$self}{sock}; $sock && fileno $sock - or return 1; + or return 0; - $self->epilogue; + my $ok = $self->epilogue; # Epilogue should destroy the SMTP filehandle, # but just to be on the safe side. $sock && fileno $sock - or return 1; + or return $ok; close $sock or croak 'Cannot destroy socket filehandle'; - 1; + $ok; } package Mail::Mailer::smtp::pipe;

Wed Oct 13 11:35:45 2010 The RT System itself - Status changed from 'new' to 'open'

Wed Oct 13 18:57:39 2010 solutions [...] overmeer.net - Correspondence added

Subject:	Re: [rt.cpan.org #62106] Mail/Mailer/smtp error handling
Date:	Thu, 14 Oct 2010 00:57:28 +0200
To:	Tristam Fenton-May via RT <bug-MailTools [...] rt.cpan.org>
From:	Mark Overmeer <solutions [...] overmeer.net>

* Tristam Fenton-May via RT (bug-MailTools@rt.cpan.org) [101013 17:35]: Show quoted text

> Queue: MailTools > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=62106 >

See notes... +++ MailTools-2.07/lib/Mail/Mailer/smtp.pm 2010-10-13 16:13:54.781250000 @@ -57,11 +57,12 @@ { my $self = shift; my $sock = ${*$self}{sock}; - $sock->dataend; + my $ok = $sock->dataend; $sock->quit; delete ${*$self}{sock}; untie *$self; + $ok; } This may break existing code, but it sounds like a sound improvement. sub close(@) @@ -69,19 +70,19 @@ my $sock = ${*$self}{sock}; $sock && fileno $sock - or return 1; + or return 0; Why this change? When the socket is already closed, you can return success on each following close() on the same. No need to complain. (I would probably complain in a new module, because it will make users code better. However, this is impossible to improve for existing code) The other changes are ok. I have added the smtps changes. -- Regards, MarkOv ------------------------------------------------------------------------ Mark Overmeer MSc MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net

Thu Oct 14 06:31:07 2010 tfm-bitcard [...] earth.li - Correspondence added

From:

tfm-bitcard [...] earth.li

On Wed Oct 13 18:57:39 2010, solutions@overmeer.net wrote: Show quoted text

> * Tristam Fenton-May via RT (bug-MailTools@rt.cpan.org) [101013 > 17:35]:

> > Queue: MailTools > > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=62106 >

>

[snip] Show quoted text

> This may break existing code, but it sounds like a sound improvement. > > sub close(@) > @@ -69,19 +70,19 @@ > my $sock = ${*$self}{sock}; > > $sock && fileno $sock > - or return 1; > + or return 0; > > Why this change? When the socket is already closed, you can return > success on each following close() on the same. No need to complain. > (I would probably complain in a new module, because it will make users > code better. However, this is impossible to improve for existing code) > > The other changes are ok. I have added the smtps changes.

The reason for that change was that if a socket error occurs Net::Cmd may close the socket, we will then miss the error on close because the socket has already been closed due to an error. To keep the behaviour that muliple closes are safe, I think the best option might be to store the current status of the connection. In the call to exec it will be set to true, and then on every operation we will update the status if there is a failure. Then close can simply return this status. Whilst implementing this I noticed that no error is returned if any of the other setup calls return an error (from, to...) I have also modified the code to check those return values. I have however, made it only return an error if there are no valid "to" addresses, rather than failing if a single to fails. In addition, the new code will check for the socket being closed by the call to datasend in Mail::Mailer::smtp::pipe::PRINT to do this I had to modify the data structure used for the pipe "self" to contain a reference to the Mail::Mailer::smtp object instead of a reference to the socket directly. It may be better to give up and stop sending commands to the server if the status is set to false, but that would change the behaviour of the module quite a bit, so I have not made that change. Please let me know if you think this patch is OK.

Subject:

MailTools-2.07_tfmErrorHandling2.patch

--- MailTools-2.07-orig/lib/Mail/Mailer/smtp.pm 2010-10-13 16:12:30.250000000 +0100 +++ MailTools-2.07/lib/Mail/Mailer/smtp.pm 2010-10-14 11:10:03.796875000 +0100 @@ -31,12 +31,16 @@ } ${*$self}{sock} = $smtp; + ${*$self}{status} = 1; - $smtp->mail(mailaddress); - $smtp->mail($opt{From}) if $opt{From}; + ${*$self}{status} = 0 unless $smtp->mail(mailaddress); + if($opt{From}) { ${*$self}{status} = 0 unless $smtp->mail($opt{From});} - $smtp->to($_) for @$to; - $smtp->data; + my $validTos=0; + foreach (@$to) { ($validTos++) if $smtp->to($_); } + ${*$self}{status} = 0 unless $validTos>0; + + ${*$self}{status} = 0 unless $smtp->data; untie *$self if tied *$self; tie *$self, 'Mail::Mailer::smtp::pipe', $self; @@ -57,7 +61,7 @@ { my $self = shift; my $sock = ${*$self}{sock}; - $sock->dataend; + ${*$self}{status} = 0 unless $sock->dataend; $sock->quit; delete ${*$self}{sock}; @@ -69,19 +73,19 @@ my $sock = ${*$self}{sock}; $sock && fileno $sock - or return 1; + or return ${*$self}{status}; $self->epilogue; # Epilogue should destroy the SMTP filehandle, # but just to be on the safe side. $sock && fileno $sock - or return 1; + or return ${*$self}{status}; close $sock or croak 'Cannot destroy socket filehandle'; - 1; + return ${*$self}{status}; } package Mail::Mailer::smtp::pipe; @@ -92,13 +96,16 @@ sub TIEHANDLE { my ($class, $self) = @_; my $sock = ${*$self}{sock}; - bless \$sock, $class; + my $newObject = {smtp=>$self}; + bless $newObject, $class; } sub PRINT { my $self = shift; - my $sock = $$self; + my $smtp = $self->{smtp}; + my $sock = ${*$smtp}{sock}; $sock->datasend( @_ ); + ${*$smtp}{status} = 0 unless $sock && fileno $sock; } 1;

Sat Feb 25 08:49:54 2012 MARKOV [...] cpan.org - Correspondence added

suggestion not taken.

Sat Feb 25 08:49:55 2012 MARKOV [...] cpan.org - Status changed from 'open' to 'rejected'