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

SMTPServer of smptd does not support binding to an IPv6 address #58963

Closed
vsergeev mannequin opened this issue May 8, 2012 · 17 comments
Closed

SMTPServer of smptd does not support binding to an IPv6 address #58963

vsergeev mannequin opened this issue May 8, 2012 · 17 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@vsergeev
Copy link
Mannequin

vsergeev mannequin commented May 8, 2012

BPO 14758
Nosy @jcea, @giampaolo, @bitdancer, @hynek, @zvyn
Files
  • smtpd.patch
  • smtpd_060914.patch
  • smtpd_061014.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 2014-06-11.20:36:52.484>
    created_at = <Date 2012-05-08.21:15:49.594>
    labels = ['type-feature', 'library']
    title = 'SMTPServer of smptd does not support binding to an IPv6 address'
    updated_at = <Date 2014-06-11.20:36:52.482>
    user = 'https://bugs.python.org/vsergeev'

    bugs.python.org fields:

    activity = <Date 2014-06-11.20:36:52.482>
    actor = 'r.david.murray'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-06-11.20:36:52.484>
    closer = 'r.david.murray'
    components = ['Library (Lib)']
    creation = <Date 2012-05-08.21:15:49.594>
    creator = 'vsergeev'
    dependencies = []
    files = ['34289', '35542', '35556']
    hgrepos = []
    issue_num = 14758
    keywords = ['patch']
    message_count = 17.0
    messages = ['160226', '160233', '160235', '160285', '160290', '212761', '212764', '215953', '220091', '220128', '220172', '220291', '220292', '220300', '220302', '220305', '220308']
    nosy_count = 8.0
    nosy_names = ['jcea', 'giampaolo.rodola', 'r.david.murray', 'jesstess', 'python-dev', 'hynek', 'vsergeev', 'zvyn']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue14758'
    versions = ['Python 3.5']

    @vsergeev
    Copy link
    Mannequin Author

    vsergeev mannequin commented May 8, 2012

    The SMTPServer class of the smtpd module creates a server socket with the IPv4 socket.AF_INET address family hardcoded, and this prevents it from later binding to an IPv6 local address.

    This occurs on line 282 of smtpd.py for the Python 2.7 branch:
    http://hg.python.org/cpython/file/5319a4bf72e7/Lib/smtpd.py#l282

    And on line 435 of smtpd for the Python 3.2 branch ( Lib/smtpd.py:435 ):
    http://hg.python.org/cpython/file/d937b527b76e/Lib/smtpd.py#l435

    One IPv4/IPv6 agnostic solution is to look up provided local address with getaddrinfo(), and use one of the result's address family, socket type and address tuple for create_socket() and bind() at those lines:

    ...
    try:
    gai_results = socket.getaddrinfo(localaddr[0], localaddr[1])
    self.create_socket(gai_results[0][0], gai_results[0][1])
    # try to re-use a server port if possible
    self.set_reuse_addr()
    self.bind(gai_results[0][4])
    self.listen(5)
    ...

    @vsergeev vsergeev mannequin added type-bug An unexpected behavior, bug, or error stdlib Python modules in the Lib dir labels May 8, 2012
    @pitrou pitrou added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels May 8, 2012
    @giampaolo
    Copy link
    Contributor

    giampaolo commented May 8, 2012

    Agreed. The only problem I see is that unit tests rely on a mock socket object and should be rewritten by using an actual socket.

    @bitdancer
    Copy link
    Member

    bitdancer commented May 8, 2012

    I don't think it is necessary to rewrite the existing tests, just add some that test the socket functionality.

    @giampaolo
    Copy link
    Contributor

    giampaolo commented May 9, 2012

    Unfortunately unit tests overwrite the original smtpd.socket module object with test.mock_socket [1] and the latter one doesn't expose socket.getaddrinfo().

    [1] http://hg.python.org/cpython/file/d937b527b76e/Lib/test/test_smtpd.py#l54

    @bitdancer
    Copy link
    Member

    bitdancer commented May 9, 2012

    Well, that should be fixed anyway (a cleanup added that restores the original value). Then a new TestCase can test the socket stuff.

    @zvyn
    Copy link
    Mannequin

    zvyn mannequin commented Mar 5, 2014

    I was going to work on bpo-3461 where IPv6-tests are missing for smtplib and stumbled over this bug. I would be willing to work on this, since it's quiet clear what needs to be done to me: implement what (vsergeev) suggested and write tests (which includes fixing design flaws in current ones).

    It may be a good idea to teach mouckup_socket some IPv6, since it's needed for test_smtpd and test_smtplib, but IMHO that can be done as a extra task / will be easy after doing the above.

    @zvyn
    Copy link
    Mannequin

    zvyn mannequin commented Mar 5, 2014

    The cleaning up of smtpd.socket was already implemented, so there was nothing to do there.

    What I did:

    • Write two TestCases to check if the IP version is chosen depending on the host-parameter
    • Testing, that everything still works with an IPv6 address by inheriting from SMTPDChannelTest and overriding setUp with an IPv6-Server

    @bitdancer
    Copy link
    Member

    bitdancer commented Apr 11, 2014

    I added some review comments. Since this is a new feature, the patch also needs a 'versionchanged' that indicates that ipv6 support was added.

    @zvyn
    Copy link
    Mannequin

    zvyn mannequin commented Jun 9, 2014

    I applied your suggestions.

    @bitdancer
    Copy link
    Member

    bitdancer commented Jun 9, 2014

    When I run the modified test suite on a machine regrtest tells me that the test modified the environment, specifically the asyncore.socket_map. Presumably there is some missing cleanup logic.

    @zvyn
    Copy link
    Mannequin

    zvyn mannequin commented Jun 10, 2014

    Fixed it.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 11, 2014

    New changeset 1efbc86a200a by R David Murray in branch 'default':
    bpo-14758: add IPv6 support to smtpd.
    http://hg.python.org/cpython/rev/1efbc86a200a

    @bitdancer
    Copy link
    Member

    bitdancer commented Jun 11, 2014

    Thanks, Milan. I had to fix a couple things: you had left the "refactored" methods on the SMTPDServerTest, and somehow your new TestFamilyDetection class got indented under SMTPDServerTest in the new version of the patch. (I also had to update it to compensate for the decode_data patch, which copy-and-pasted the DummyServer calling bugs you fixed in the other tests...)

    @bitdancer
    Copy link
    Member

    bitdancer commented Jun 11, 2014

    Hmm. Looks like the IPv6 support is making the FreeBSD and and OSX buildbots unhappy :(.

    @bitdancer bitdancer reopened this Jun 11, 2014
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 11, 2014

    New changeset d8e0fca7cbe3 by R David Murray in branch 'default':
    bpo-14758: Need to specify the desired socket type in the getaddrinfo call.
    http://hg.python.org/cpython/rev/d8e0fca7cbe3

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 11, 2014

    New changeset 9b0d58b0c712 by R David Murray in branch 'default':
    bpo-14758: Fix the fix (fix getaddrinfo in mock_socket)
    http://hg.python.org/cpython/rev/9b0d58b0c712

    @bitdancer
    Copy link
    Member

    bitdancer commented Jun 11, 2014

    OK, I think this is fixed.

    @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
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants