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

SSL sockets do not retain the parent socket's attributes #52770

Closed
pitrou opened this issue Apr 24, 2010 · 13 comments
Closed

SSL sockets do not retain the parent socket's attributes #52770

pitrou opened this issue Apr 24, 2010 · 13 comments
Labels
stdlib Python modules in the Lib dir

Comments

@pitrou
Copy link
Member

pitrou commented Apr 24, 2010

BPO 8524
Nosy @loewis, @pitrou, @giampaolo, @merwok
Files
  • sockforget.patch
  • sslrefactor.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 2010-08-08.23:26:12.440>
    created_at = <Date 2010-04-24.20:47:46.535>
    labels = ['library']
    title = "SSL sockets do not retain the parent socket's attributes"
    updated_at = <Date 2010-09-06.01:43:20.453>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2010-09-06.01:43:20.453>
    actor = 'eric.araujo'
    assignee = 'none'
    closed = True
    closed_date = <Date 2010-08-08.23:26:12.440>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2010-04-24.20:47:46.535>
    creator = 'pitrou'
    dependencies = []
    files = ['17723', '17726']
    hgrepos = []
    issue_num = 8524
    keywords = ['patch']
    message_count = 13.0
    messages = ['104129', '104130', '104131', '104133', '104138', '104281', '104285', '108212', '108214', '108248', '112861', '113350', '115682']
    nosy_count = 6.0
    nosy_names = ['loewis', 'exarkun', 'janssen', 'pitrou', 'giampaolo.rodola', 'eric.araujo']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue8524'
    versions = ['Python 3.1', 'Python 3.2']

    @pitrou
    Copy link
    Member Author

    pitrou commented Apr 24, 2010

    In 3.x, SSL sockets are created by dup()ing the original socket and then closing it. No attempt is made to conserve the socket's characteristics, such as the timeout and probably other flags.

    I understand that it may be too late to change the design decision of using dup() and closing the original, but perhaps we should make a best effort to retain the original socket's characteristics. Or perhaps this limitation should be clearly documented (since especially 2.x works differently).

    @pitrou pitrou added the stdlib Python modules in the Lib dir label Apr 24, 2010
    @pitrou
    Copy link
    Member Author

    pitrou commented Apr 24, 2010

    It is actually a little funnier. dup() preserves the
    blocking/non-blocking nature of the underlying OS socket, but not the
    "timeout" of the Python socket. As such, a "blocking-with-timeout"
    Python socket gets replaced with a truely non-blocking socket.

    @pitrou
    Copy link
    Member Author

    pitrou commented Apr 24, 2010

    Another consequence is that the following check in __init__:

                        timeout = self.gettimeout()
                        if timeout == 0.0:
                            # non-blocking
                            raise ValueError("do_handshake_on_connect should not be specified for non-blocking sockets")

    could never get triggered since the timeout is reset to None by virtue of creating a new socket object.

    @pitrou pitrou added the type-bug An unexpected behavior, bug, or error label Apr 24, 2010
    @pitrou
    Copy link
    Member Author

    pitrou commented Apr 24, 2010

    I committed a fix+tests for the timeout value in r80456 (py3k) and r80457 (3.1). Apparently the socket objects' own dup() method doesn't try to retain anything else than the timeout.

    I'm leaving this issue as "pending" in case criticism or better options are provided :)

    @exarkun
    Copy link
    Mannequin

    exarkun mannequin commented Apr 25, 2010

    Well, at the risk of stating the obvious, perhaps the dup() thing should be eliminated. The justification for it seems less than clear, and apparently it causes some problems.

    That might be a direction to consider in the long term, though, rather than as a (different) immediate fix for this issue.

    @exarkun exarkun mannequin removed the type-bug An unexpected behavior, bug, or error label Apr 25, 2010
    @pitrou
    Copy link
    Member Author

    pitrou commented Apr 26, 2010

    Well, at the risk of stating the obvious, perhaps the dup() thing
    should be eliminated. The justification for it seems less than clear,
    and apparently it causes some problems.

    I've just found another problem while investigating the cause of some
    sporadic Windows failures:
    http://www.python.org/dev/buildbot/builders/x86%20XP-4%
    203.1/builds/718/steps/test/logs/stdio

    I've reproduced it on an XP VM and the explanation is that, sometimes,
    just after a dup() of a socket, calling getpeername() on the child
    socket fails (while getpeername() on the parent succeeds). It seems very
    timing-sensitive: if I insert enough code after the dup(), the call to
    getpeername() succeeds.

    I will fix the buildbot issue by using a different logic (simply, call
    getpeername() on the parent rather than the child), but this seems to
    confirms that dup() may not be a good idea.

    @exarkun
    Copy link
    Mannequin

    exarkun mannequin commented Apr 26, 2010

    getpeername() "sometimes" failing "soon" after a socket is created is a semi-well-known Windows socket... feature. For whatever that's worth.

    @pitrou
    Copy link
    Member Author

    pitrou commented Jun 19, 2010

    I have tried refactoring the ssl code in order not to inherit from socket anymore, but it turned out suboptimal because chunks of code from the socket class then have to be duplicated.

    Therefore, I instead propose a much simpler approach which is to add a forget() method to socket objects; this method "closes" the socket object (sets the internal fd to -1) without closing the underlying fd at all. This allows to create another socket (actually SSLSocket) from the same fd without having to dup() it.

    Here is a patch; if the principle is accepted, I will add tests and docs.

    @exarkun
    Copy link
    Mannequin

    exarkun mannequin commented Jun 19, 2010

    It might be nice to see the version that avoids the dup() and has the duplicate code instead (interesting trade-off ;). Just for the sake of comparison against the forget() proposal.

    @pitrou
    Copy link
    Member Author

    pitrou commented Jun 20, 2010

    It might be nice to see the version that avoids the dup() and has the
    duplicate code instead (interesting trade-off ;). Just for the sake of
    comparison against the forget() proposal.

    Here it is. There's probably a bit of polishing left to do, in the (unlikely) case this is chosen as a solution.

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 4, 2010

    If nobody chimes in, I will opt for the socket.forget() solution.

    @pitrou
    Copy link
    Member Author

    pitrou commented Aug 8, 2010

    sockforget.patch committed in r83869 (py3k). Thank you for the comments.

    @pitrou pitrou closed this as completed Aug 8, 2010
    @merwok
    Copy link
    Member

    merwok commented Sep 6, 2010

    In case someone lands here from the 3.2 what’s new document: The method has been renamed to detach.

    @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
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants