204229 - Multiple 251 SMTP Responses Causes Send Failure

Status: RESOLVED FIXED

(MailNews Core :: Networking: SMTP, defect)

Description

[Jason Koeninger]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4; en-US; rv:1.0.0) Gecko/20020602
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3) Gecko/20030312

Our mail server uses 251 responses when forwarding mail to external servers. 
Mozilla is fine with sending to one address resulting in a 251 response that's
at the END of the list of recipients.  If, however, you send to multiple
addresses with 251 responses or send with a 251 response before the end of the
list, you get an error.

I've not looked a lot at the Mozilla source, but I did find code that seems to
explain the problem.  In nsSmtpProtocol.cpp, there's a state system used to walk
through sending a message with SMTP.  In this section of code in the
SendRecipientResponse method:

1162    if(m_responseCode != 250 && m_responseCode != 251)
1163     {
1164     nsresult rv = nsExplainErrorDetails(m_runningURL,
NS_ERROR_SENDING_RCPT_COMMAND, m_responseText.get());
1165     NS_ASSERTION(NS_SUCCEEDED(rv), "failed to explain SMTP error");
1166 
1167     m_urlErrorState = NS_ERROR_BUT_DONT_SHOW_ALERT;
1168     return(NS_ERROR_SENDING_RCPT_COMMAND);
1169     }
1170 
1171     if(m_addressesLeft > 0)
1172     {
1173         /* more senders to RCPT to 
1174          */
1175         m_nextState = SMTP_SEND_MAIL_RESPONSE; 
1176         return(0);
1177     }
1178 

251's are accepted as they should be.  But, in line 1175, the state is set back
to the mail from response state which would seem to go back to the
SendMailResopnse method with this piece of code:

1101   if(m_responseCode != 250 || CHECK_SIMULATED_ERROR(SIMULATED_SEND_ERROR_11))
1102     {
1103         nsresult rv = nsExplainErrorDetails(m_runningURL,
NS_ERROR_SENDING_FROM_COMMAND, m_responseText.get());
1104         NS_ASSERTION(NS_SUCCEEDED(rv), "failed to explain SMTP error");
1105 
1106         m_urlErrorState = NS_ERROR_BUT_DONT_SHOW_ALERT;
1107         return(NS_ERROR_SENDING_FROM_COMMAND);
1108     }

The 251 would then be rejected at this point as it's not a valid response for
the MAIL FROM command.

Two possible solutions:

1.  Don't use exact error numbers...just stick to the major classes like 2xy for
positive completion responses per RFC 821.

2.  Change m_ResponseCode back to 250 at line 1175.

Like I said, though, I don't know the Mozilla source so I'm pretty much just
guessing here that I'm looking at the right piece of code.

Reproducible: Always

Steps to Reproduce:
1.  Find a mail server that uses 251 responses.
2.  Create and send a message to 2 or more recipients that will cause 251 responses


Actual Results:  
User not local; will forward to <> error message displayed in error message dialog.

Expected Results:  
Mozilla should have continued sending the message.

Comment 1

[Matthias Versen [:Matti]]

Could you attach a SMTP log ?
http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html

Comment 2

[Christian Eyrich]

Attachment: proposed patch

Jason, your conclusion was perfectly right, thanks.

I've chosen solution 2 because I wanted to follow the RFC and don't allow other
2xx codes.

Comment 3

[Christian Eyrich]

Comment on attachment 122434 [details] [diff] [review]
proposed patch

A a= for 1.4b would be nice too. A dead bug is a dead bug and this fix is
without risk I think.

Comment 4

[Christian Eyrich]

-> to get me responsible

Comment 5

[David :Bienvenu]

Comment on attachment 122434 [details] [diff] [review]
proposed patch

r=bienvenu - I'll forward to Sspitzer for sr/approval

Comment 6

[David :Bienvenu]

is this a recent regression? If so, that would make it much more likely that
we'll take this fix for 1.4b

Comment 7

[Christian Eyrich]

No, seems to be there from beginning.

Comment 8

[(not reading, please use [e-mail] instead)]

grepping for 250 in nsSmtpProtocol:

if (m_responseCode != 250)

if (m_responseCode == 250 || m_responseCode == 251)

if(m_responseCode != 250 || CHECK_SIMULATED_ERROR(SIMULATED_SEND_ERROR_11))

if(m_responseCode != 250 && m_responseCode != 251)

if((m_responseCode != 354) && (m_responseCode != 250)) {

if(((m_responseCode != 354) && (m_responseCode != 250)) ||
CHECK_SIMULATED_ERROR(SIMULATED_SEND_ERROR_12)) {

note, 2 are ok (w.r.t. 251), would it be better to fix the other 4 places to
work with 251?  (are other problems lurking?)

Comment 9

[Jason Koeninger]

RFC 821 Section 4.3 only shows the 251 response being valid for the RCPT TO and
VRFY commands.  The only methods I see handling the RCPT TO result are
SendRecipientResponse (directly) and SendMailResponse (indirectly by stepping
back  for additional recipients).  If I had written the original code, I would
have split out the sending of the recipient command and called it in the
SendRecipientResponse method instead of stepping the state back to share the code.

It looks like VRFY already supports 251.

Comment 10

[Christian Eyrich]

Right, all other states where 251 (VRFY) is valid are already aware of it.

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 122434 [details] [diff] [review]
proposed patch

sr/a=sspitzer

Comment 12

[David :Bienvenu]

fix checked in.