smtplib gets out of sync if server returns a 421 status #49963

msapiro · 2009-04-06T20:30:23Z

BPO	5713
Nosy	@warsaw, @terryjreedy, @jcea, @msapiro, @bitdancer, @kushaldas
Files	smtplib.patch: Possible smtplib patch issue5713.patch smtp421.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2013-03-21.02:16:44.927>
created_at = <Date 2009-04-06.20:30:23.080>
labels = ['type-bug', 'library', 'expert-email']
title = 'smtplib gets out of sync if server returns a 421 status'
updated_at = <Date 2013-03-21.04:34:15.238>
user = 'https://github.com/msapiro'

bugs.python.org fields:

activity = <Date 2013-03-21.04:34:15.238>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2013-03-21.02:16:44.927>
closer = 'r.david.murray'
components = ['Library (Lib)', 'email']
creation = <Date 2009-04-06.20:30:23.080>
creator = 'msapiro'
dependencies = []
files = ['15183', '29507', '29518']
hgrepos = []
issue_num = 5713
keywords = ['patch']
message_count = 14.0
messages = ['85666', '94240', '94366', '112752', '112814', '184775', '184786', '184794', '184802', '184821', '184823', '184828', '184832', '184841']
nosy_count = 7.0
nosy_names = ['barry', 'terry.reedy', 'jcea', 'msapiro', 'r.david.murray', 'python-dev', 'kushal.das']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue5713'
versions = ['Python 3.2', 'Python 3.3', 'Python 3.4']

msapiro · 2009-04-06T20:30:21Z

RFC821 upon which smtplib was originally based does not define a 421
status code and implies the server should only disconnect in response to
a QUIT command.

Subsequent extensions in RFC2821 (and now RFC5321) define situations
under which the server may return a 421 status and disconnect. This
leads to the following problem.

An smtplib.SMTP() instance is created and its sendmail() method is
called with a list of recipients which contains several invalid, local
addresses. sendmail() processes the recipient list, calling the rcpt()
method for each. Some of these may be accepted with a 250 or 251 status
and some may be rejected with a 550 or other status. The rejects are
kept in a dictionary to be eventually returned as the sendmail() result.

However, with the Postfix server at least, after 20 rejects, the server
sends a 421 Too many errors reply and disconnects, but sendmail
continues to process and this results in raising
SMTPServerDisconnected("Connection unexpectedly closed") and the
response dictionary containing the invalid addresses and their responses
is lost.

The caller may see the exception as retryable and may retry the send
after some delay, but since the caller has received no information about
the invalid addresses, it sends the same recipient list and the scenario
repeats.

jcea · 2009-10-19T15:16:25Z

So, if I understand the issue and the RFC correctly, when receiving a
421, we should close the connection and, if the 421 was received in the
"rcpt" phase, give back a dictionary with all pending directions as
"421" code, so the sending code knows they are 4xx codes and should
retry again later.

What return code would be give back if we get 421 in other situations?.
Maybe a regular "connection close"?.

msapiro · 2009-10-22T18:03:23Z

I'm not completely sure about this, but here's my thoughts. In the
scenarios I've seen, the 421 reply/disconnect only occurs in response to
a RCPT which has an invalid address and follows several prior refused
RCPTs. In this case, I think the proper action is to close the
connection and raise SMTPRecipientsRefused and return a dictionary with
the actual responses for the refused RCPTS prior to the 421 and the 421
response only for the RCPT that produced it.

If the 421 comes at another time, I think the current process does the
right thing. It will raise the appropriate exception if it gets the
chance. It just needs to be sure that if the response was 421 that
instead of doing self.rset() it does self.close().

I have attached a patch against the 2.6.1 smtplib.py which I think does
the right thing. I haven't tested this at all, but I think it should
work. The documentation may need to be updated to emphasize that even
though all recipients aren't listed in the dictionary returned with the
SMTPRecipientsRefused exception, no one got the mail.

terryjreedy · 2010-08-04T04:19:06Z

Smptlib is documented as RFC821/1869 based. So this is a request for a new feature. I presume more would be required to bring it up to 2821/5321. Assuming so, what is the rationale for adding just one new feature. This would mean tacking something like "and RFC 2821 for 421 status" on the end of "For details of SMTP and ESMTP operation, consult RFC 821 (Simple Mail Transfer Protocol) and RFC 1869 (SMTP Service Extensions)."

Can a test be written for this? There seem to be some coded status tests already.

I

warsaw · 2010-08-04T14:43:18Z

The latest relevant RFC is 5321:

http://www.faqs.org/rfcs/rfc5321.html

smtplib should be reviewed for compliance with this updated spec.

kushaldas · 2013-03-20T18:48:20Z

Here is a patch along with test case update which can be cleanly applied into default.

bitdancer · 2013-03-20T19:51:56Z

I think this should be considered a bug fix. While it is true that we are handling something that is a "more advanced" feature of the smpt protocl than what we are (currently) formally supporting in the module, the reality is that if one uses smtplib to communicate with a foreign mail server, you currently get unhandleable errors even if the library sticks to the currently supported RFC level. I think that the fact that smtplib does the wrong then when it receives an unexpected error code followed by a connection close is a bug in smtplib regardless of what RFC level we are talking about.

In fact, this is one instance of a larger problem. I have also seen a server that returns a 451 error code at the end of a data command *and then disconnects*. I think that is out of spec even with the newer RFCs, but it is something that happens in the real world and so we should handle it.

The general problem in smtplib is that it does an rset on error, and if the other end has closed the connection, the rset raises a ServerDisconnectedError, masking the error code that the server actually reported. This also should be fixed.

Clearly for 421 we should be doing a close and not an rset, and I think we may as well go ahead and do that even if we haven't upgraded the library to fully comply with the newer RFC yet, especially since we've got a patch here. It is not a publicly visible API change, so I don't think the "new feature" rule needs to be applied strictly. Correctly handling the case of rset finding the socket already closed would also have fixed this bug, and we should make that fix as well, since it seems that servers may unexpectedly close the connection after any error.

terryjreedy · 2013-03-20T20:50:46Z

Given that David is usually more conservative about calling things bugs than I am, knows more about this subject than me, and has explained that the change will make smptlib act more sanely in the face of external errors, I am reversing myself and agreeing with him.

The patch seems to tighten the test in a way relevant to the code change.

I see that there are several open smptlib issues.
Should we open one for 'update to rfc 5321'?
Does the absence of 'obsoleted by' (or similar) mean that it is still the latest?

bitdancer · 2013-03-20T21:30:22Z

Sure, a new issue for making sure smtplib conforms to 5321 would be good (we already did it for smptd; hopefully we didn't miss anything).

For the moment 5321 is the standard. There's a new one coming, where utf-8 will be allowed, but we're not quite ready to tackle that yet (soon, though, hopefully).

bitdancer · 2013-03-21T01:08:27Z

I had to rewrite the tests for 3.2, and I added tests to test all three cases where 421 is returned. I'm in the process of merging this, but I'll upload the patch so that Kushal can see the test structure for the followon patch.

python-dev · 2013-03-21T01:15:12Z

New changeset bc538bc8e65d by R David Murray in branch '3.2':
bpo-5713: Handle 421 error codes during sendmail by closing the socket.
http://hg.python.org/cpython/rev/bc538bc8e65d

New changeset cf5c40025af2 by R David Murray in branch '3.3':
Merge: bpo-5713: Handle 421 error codes during sendmail by closing the socket.
http://hg.python.org/cpython/rev/cf5c40025af2

New changeset ce4154268c47 by R David Murray in branch 'default':
Merge: bpo-5713: Handle 421 error codes during sendmail by closing the socket.
http://hg.python.org/cpython/rev/ce4154268c47

python-dev · 2013-03-21T01:56:34Z

New changeset fbf54209de75 by R David Murray in branch '3.2':
bpo-5713: fix timing issue in smtplib tests.
http://hg.python.org/cpython/rev/fbf54209de75

New changeset 0de74602692f by R David Murray in branch '3.3':
Merge: bpo-5713: fix timing issue in smtplib tests.
http://hg.python.org/cpython/rev/0de74602692f

New changeset 6dc948e4ea8f by R David Murray in branch 'default':
Merge: bpo-5713: fix timing issue in smtplib tests.
http://hg.python.org/cpython/rev/6dc948e4ea8f

bitdancer · 2013-03-21T02:16:45Z

Thanks Mark and Kushal.

python-dev · 2013-03-21T04:34:15Z

New changeset d068fcbe5009 by R David Murray in branch '3.3':
bpo-5713: One more test_smtplib timing fix.
http://hg.python.org/cpython/rev/d068fcbe5009

New changeset fcef6a33de17 by R David Murray in branch 'default':
Merge: bpo-5713: One more test_smtplib timing fix.
http://hg.python.org/cpython/rev/fcef6a33de17

msapiro mannequin added type-bug An unexpected behavior, bug, or error stdlib Python modules in the Lib dir labels Apr 6, 2009

warsaw self-assigned this Feb 21, 2010

terryjreedy added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Aug 4, 2010

bitdancer added the topic-email label Mar 20, 2013

terryjreedy added type-bug An unexpected behavior, bug, or error and removed type-feature A feature request or enhancement labels Mar 20, 2013

bitdancer closed this as completed Mar 21, 2013

bitdancer unassigned warsaw Mar 21, 2013

ezio-melotti transferred this issue from another repository Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

smtplib gets out of sync if server returns a 421 status #49963

smtplib gets out of sync if server returns a 421 status #49963

msapiro mannequin commented Apr 6, 2009

msapiro mannequin commented Apr 6, 2009

jcea commented Oct 19, 2009

msapiro mannequin commented Oct 22, 2009

terryjreedy commented Aug 4, 2010

warsaw commented Aug 4, 2010

kushaldas commented Mar 20, 2013

bitdancer commented Mar 20, 2013

terryjreedy commented Mar 20, 2013

bitdancer commented Mar 20, 2013

bitdancer commented Mar 21, 2013

python-dev mannequin commented Mar 21, 2013

python-dev mannequin commented Mar 21, 2013

bitdancer commented Mar 21, 2013

python-dev mannequin commented Mar 21, 2013

smtplib gets out of sync if server returns a 421 status #49963

smtplib gets out of sync if server returns a 421 status #49963

Comments

msapiro mannequin commented Apr 6, 2009

msapiro mannequin commented Apr 6, 2009

jcea commented Oct 19, 2009

msapiro mannequin commented Oct 22, 2009

terryjreedy commented Aug 4, 2010

warsaw commented Aug 4, 2010

kushaldas commented Mar 20, 2013

bitdancer commented Mar 20, 2013

terryjreedy commented Mar 20, 2013

bitdancer commented Mar 20, 2013

bitdancer commented Mar 21, 2013

python-dev mannequin commented Mar 21, 2013

python-dev mannequin commented Mar 21, 2013

bitdancer commented Mar 21, 2013

python-dev mannequin commented Mar 21, 2013