classification
Title: Mutable Objects in SMTP send_message Signature
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Kenny Trytek, chason.chaffin, r.david.murray, serhiy.storchaka, steven.daprano
Priority: normal Keywords: patch

Created on 2018-01-24 22:44 by Kenny Trytek, last changed 2019-02-22 07:52 by inada.naoki. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 5334 closed chason.chaffin, 2018-01-26 03:40
Messages (10)
msg310641 - (view) Author: Kenny Trytek (Kenny Trytek) Date: 2018-01-24 22:44
Mutable arguments in signature of send_message:
https://github.com/python/cpython/blob/master/Lib/smtplib.py#L892-L893

More Information:
http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments
msg310658 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2018-01-25 07:00
Hi Kenny, and thanks, but I'm not sure what your point is. Are you claiming this is a bug in the code or the documentation?

For what it is worth... mutable defaults *are* a "gotcha", so the documentation isn't wrong. And mutable defaults are *not* illegal nor a bug and are occasionally useful. So I'm not sure why you are reporting this? Have you found a problem?

(I see that send_message gives rcpt_options a default of {} (empty dict) and then passes it to sendmail, which expects rcpt_options to be a list. I don't know if that matters.)
msg310660 - (view) Author: Chason Chaffin (chason.chaffin) * Date: 2018-01-25 07:25
Hi Steven,

I have nothing to do with the original submitter but this bug did catch my eye. Does using mutable variables in the SMTP library serve a purpose? Looking through the code, I wasn't able to spot anything obvious, and I did spot a place with a potential bug:

https://github.com/python/cpython/blob/master/Lib/smtplib.py#L960

Here mail_options is mutated in the case of a non-ASCII email address. Subsequent calls to send_message, even to a different SMTP server (if I'm not mistaken) would also send these headers along.
msg310677 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2018-01-25 14:28
Chason: that does look like a bug.

Mutable defaults are best to avoid, but if they are used read-only and not passed down further it isn't a problem.  send_message was modeled on sendmail, and so copied it's use of defaults (which date from quite some time back :), but obviously I missed that mutation of the value in the code review of the patch that added those lines :(
msg310679 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2018-01-25 15:18
On Thu, Jan 25, 2018 at 02:28:17PM +0000, R. David Murray wrote:

> obviously I missed that mutation of the value 
> in the code review of the patch that added those lines :(

The docstring for send_message does say 

    If the sender or any of the recipient addresses contain non-ASCII
    and the server advertises the SMTPUTF8 capability, the policy is
    cloned with utf8 set to True for the serialization, and SMTPUTF8
    and BODY=8BITMIME are asserted on the send.

which I don't really understand, but I thought that perhaps it was a 
typo for *inserted* on the send, in the sense of inserted into the mail 
options:

    mail_options += ['SMTPUTF8', 'BODY=8BITMIME']

So are we agreed this is a bug? What about the default for rcpt_options 
being a dict?
msg310713 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2018-01-25 22:05
On Thu, 25 Jan 2018 15:18:01 +0000, Steven D'Aprano <report@bugs.python.org> wrote:
> On Thu, Jan 25, 2018 at 02:28:17PM +0000, R. David Murray wrote:
> The docstring for send_message does say 
> 
>     If the sender or any of the recipient addresses contain non-ASCII
>     and the server advertises the SMTPUTF8 capability, the policy is
>     cloned with utf8 set to True for the serialization, and SMTPUTF8
>     and BODY=8BITMIME are asserted on the send.

"Asserted" means sent with the SMTP commands.  It could be reworded to
be clearer.

> which I don't really understand, but I thought that perhaps it was a 
> typo for *inserted* on the send, in the sense of inserted into the mail 
> options:
> 
>     mail_options += ['SMTPUTF8', 'BODY=8BITMIME']

Even if that had been true, it would still be a bug to do it to the
mutable argument :)

> So are we agreed this is a bug? What about the default for rcpt_options 
> being a dict?

I didn't look at that, but it probably is.
msg310727 - (view) Author: Chason Chaffin (chason.chaffin) * Date: 2018-01-26 02:27
I'm writing a PR for this right now. Is it worth it to write a test to make sure that mail_options isn't being mutated or is it enough just to update the code to use non-mutable defaults? Interesting side effect of looking into this is #32663 where I found the tests for the UTF8 code weren't being run.
msg310732 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2018-01-26 04:17
It's probably enough to fix it.  I fear that if we also change them in sendmail we'll break someone's code, but maybe we should do that anyway, for 3.7 only.
msg310734 - (view) Author: Chason Chaffin (chason.chaffin) * Date: 2018-01-26 04:43
> It's probably enough to fix it.  I fear that if we also change them in sendmail we'll break someone's code, but maybe we should do that anyway, for 3.7 only.

Should I make a separate issue for that, or attach it to this PR?
msg323779 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-08-20 08:26
See also issue34246.
History
Date User Action Args
2019-02-22 07:52:16inada.naokisetversions: + Python 3.8, - Python 3.6, Python 3.7
2019-02-22 07:51:58inada.naokisetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-08-20 08:26:51serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg323779
2018-01-26 04:43:17chason.chaffinsetmessages: + msg310734
2018-01-26 04:17:55r.david.murraysetmessages: + msg310732
versions: - Python 2.7, Python 3.4, Python 3.5, Python 3.8
2018-01-26 03:40:35chason.chaffinsetkeywords: + patch
stage: patch review
pull_requests: + pull_request5180
2018-01-26 02:27:01chason.chaffinsetmessages: + msg310727
2018-01-25 22:05:01r.david.murraysetmessages: + msg310713
2018-01-25 15:18:01steven.dapranosetmessages: + msg310679
2018-01-25 14:28:17r.david.murraysetnosy: + r.david.murray
messages: + msg310677
2018-01-25 07:25:23chason.chaffinsetnosy: + chason.chaffin
messages: + msg310660
2018-01-25 07:00:02steven.dapranosetnosy: + steven.daprano
messages: + msg310658
2018-01-24 22:44:57Kenny Trytekcreate