classification
Title: SSL sockets do not retain the parent socket's attributes
Type: Stage:
Components: Library (Lib) Versions: Python 3.1, Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eric.araujo, exarkun, giampaolo.rodola, janssen, loewis, pitrou
Priority: normal Keywords: patch

Created on 2010-04-24 20:47 by pitrou, last changed 2010-09-06 01:43 by eric.araujo. This issue is now closed.

Files
File name Uploaded Description Edit
sockforget.patch pitrou, 2010-06-19 22:14
sslrefactor.patch pitrou, 2010-06-20 20:51
Messages (13)
msg104129 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-04-24 20:47
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).
msg104130 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-04-24 21:11
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.
msg104131 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-04-24 21:20
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.
msg104133 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-04-24 22:10
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 :)
msg104138 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2010-04-25 01:21
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.
msg104281 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-04-26 21:47
> 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.
msg104285 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2010-04-26 23:14
getpeername() "sometimes" failing "soon" after a socket is created is a semi-well-known Windows socket... feature.  For whatever that's worth.
msg108212 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-06-19 22:14
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.
msg108214 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2010-06-19 22:36
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.
msg108248 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-06-20 20:51
> 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.
msg112861 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-08-04 18:56
If nobody chimes in, I will opt for the socket.forget() solution.
msg113350 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-08-08 23:26
sockforget.patch committed in r83869 (py3k). Thank you for the comments.
msg115682 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-09-06 01:43
In case someone lands here from the 3.2 what’s new document: The method has been renamed to detach.
History
Date User Action Args
2010-09-06 01:43:20eric.araujosetnosy: + eric.araujo
messages: + msg115682
2010-08-08 23:26:12pitrousetstatus: open -> closed
resolution: fixed
messages: + msg113350
2010-08-04 18:56:03pitrousetmessages: + msg112861
2010-07-21 16:19:28amaury.forgeotdarclinkissue8293 superseder
2010-06-20 20:51:52pitrousetfiles: + sslrefactor.patch

messages: + msg108248
2010-06-19 22:36:15exarkunsetmessages: + msg108214
2010-06-19 22:14:13pitrousetfiles: + sockforget.patch

nosy: + loewis
messages: + msg108212

keywords: + patch
2010-04-26 23:14:17exarkunsetmessages: + msg104285
2010-04-26 21:47:52pitrousetmessages: + msg104281
2010-04-25 01:21:01exarkunsetstatus: pending -> open
priority: high -> normal
type: behavior ->


nosy: + exarkun
messages: + msg104138
resolution: fixed -> (no value)
stage: resolved ->
2010-04-24 22:10:43pitrousetstatus: open -> pending
resolution: fixed
messages: + msg104133

stage: resolved
2010-04-24 21:21:32pitrousetpriority: normal -> high
type: behavior
2010-04-24 21:20:57pitrousetmessages: + msg104131
2010-04-24 21:11:57pitrousetmessages: + msg104130
2010-04-24 20:47:46pitroucreate