Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutable Objects in SMTP send_message Signature #76838

Closed
KennyTrytek mannequin opened this issue Jan 24, 2018 · 10 comments
Closed

Mutable Objects in SMTP send_message Signature #76838

KennyTrytek mannequin opened this issue Jan 24, 2018 · 10 comments
Labels
3.8 only security fixes stdlib Python modules in the Lib dir

Comments

@KennyTrytek
Copy link
Mannequin

KennyTrytek mannequin commented Jan 24, 2018

BPO 32657
Nosy @stevendaprano, @bitdancer, @serhiy-storchaka, @chason
PRs
  • bpo-32657 Removing mutable entry from send_message #5334
  • 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 2019-02-22.07:51:58.696>
    created_at = <Date 2018-01-24.22:44:57.593>
    labels = ['3.8', 'library']
    title = 'Mutable Objects in SMTP send_message Signature'
    updated_at = <Date 2019-02-22.07:52:16.162>
    user = 'https://bugs.python.org/KennyTrytek'

    bugs.python.org fields:

    activity = <Date 2019-02-22.07:52:16.162>
    actor = 'methane'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-02-22.07:51:58.696>
    closer = 'methane'
    components = ['Library (Lib)']
    creation = <Date 2018-01-24.22:44:57.593>
    creator = 'Kenny Trytek'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32657
    keywords = ['patch']
    message_count = 10.0
    messages = ['310641', '310658', '310660', '310677', '310679', '310713', '310727', '310732', '310734', '323779']
    nosy_count = 5.0
    nosy_names = ['steven.daprano', 'r.david.murray', 'serhiy.storchaka', 'Kenny Trytek', 'chason.chaffin']
    pr_nums = ['5334']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue32657'
    versions = ['Python 3.8']

    @KennyTrytek
    Copy link
    Mannequin Author

    KennyTrytek mannequin commented Jan 24, 2018

    @KennyTrytek KennyTrytek mannequin added 3.8 only security fixes 3.7 (EOL) end of life stdlib Python modules in the Lib dir labels Jan 24, 2018
    @stevendaprano
    Copy link
    Member

    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.)

    @chason
    Copy link
    Mannequin

    chason mannequin commented Jan 25, 2018

    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.

    @bitdancer
    Copy link
    Member

    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 :(

    @stevendaprano
    Copy link
    Member

    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?

    @bitdancer
    Copy link
    Member

    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.

    @chason
    Copy link
    Mannequin

    chason mannequin commented Jan 26, 2018

    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 bpo-32663 where I found the tests for the UTF8 code weren't being run.

    @bitdancer
    Copy link
    Member

    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.

    @bitdancer bitdancer removed the 3.8 only security fixes label Jan 26, 2018
    @chason
    Copy link
    Mannequin

    chason mannequin commented Jan 26, 2018

    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?

    @serhiy-storchaka
    Copy link
    Member

    See also bpo-34246.

    @methane methane closed this as completed Feb 22, 2019
    @methane methane added 3.8 only security fixes and removed 3.7 (EOL) end of life labels Feb 22, 2019
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants