classification
Title: ssl.SSLSocket implements methods that are overriden by socket.socket.__init__ and methods with incorrect names.
Type: Stage:
Components: Library (Lib) Versions: Python 3.0, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: janssen Nosy List: amaury.forgeotdarc, giampaolo.rodola, hodgestar, janssen
Priority: normal Keywords: patch

Created on 2008-06-21 17:13 by hodgestar, last changed 2008-09-08 20:52 by hodgestar. This issue is now closed.

Files
File name Uploaded Description Edit
trunk-ssl-methods.patch hodgestar, 2008-09-05 12:00 Add recv_into and recvfrom_into methods.
3k-ssl-methods.patch hodgestar, 2008-09-05 12:01 Add recvfrom_into method.
3k-ssl-tests.patch hodgestar, 2008-09-05 15:27 Tests for 3.0.
trunk-ssl-tests.patch hodgestar, 2008-09-05 16:49 Update of trunk-ssl-methods.patch with tests and fixes for bugs found.
Messages (17)
msg68517 - (view) Author: Simon Cross (hodgestar) Date: 2008-06-21 17:13
It appears that ssl.SSLSocket attempts to override some of the methods
socket.socket delegates to the underlying _socket.socket methods.
However, this overriding fails because socket.socket.__init__ replaces
all the methods mentioned in _delegate_methods.

These methods are: recv, recvfrom, recv_into, recvfrom_into, send and
sendto.

ssl.SSLSocket implements recv and send. Neither of these overrides will
actually work.  ssl.SSLSocket also implements recv_from and send_to
which seem to be attempts to override recvfrom and sendto.

In the Py3k branch it looks like the overriding done by
socket.socket.__init__ is gone so that these methods are now potentially
overridable. This could potentially be emulated in trunk by checking for
the methods using hasattr(...) before overriding them.

I'm happy to create patches which fix the method names and clean up the
methods on both branches and add tests. I just want to check that I have
an accurate picture of what's needed before starting on them.
msg68913 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-06-28 21:31
Thanks for pointing this out.  I've got a patch for the overrides and
the misnamings already.

We don't have implementations for recvfrom_into, or recv_into.  If you'd
care to create a patch for that, it would help.
msg68921 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-06-28 22:22
I've committed the patch which I have for 2.6.

Still need renamings for 3.0, and implementations
of recvfrom_into and recv_into.
msg72448 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-09-04 01:19
Simon, Python 3.x now has recvfrom and recv_into, but not recvfrom_into.
 If you'd like to work up a patch for that, I'll add it to the next
release cycle.
msg72586 - (view) Author: Simon Cross (hodgestar) Date: 2008-09-05 12:00
I've attached a patch for trunk / 2.6 that adds recv_into and
recvfrom_into methods to ssl.SSLSocket and does a minor re-ordering of
the nasty lambdas in __init__. The recv_into method is a port of the one
from 3.0. The recvfrom_into method is a simple "fail if we have an SSL
object or pass down if we don't".

For the record, I'm against the lambdas in SSLSocket.__init__ and would
prefer the solution I suggested previously of putting a hasattr(...)
check into socket.socket (I can submit a patch if useful).

As is probably abundantly clear at the moment :), none of the SSLSocket
methods in questions have tests in test_ssl -- I will move on to trying
to add them next.

I'm also going to attach a patch for 3.0 which adds recvfrom_into (a
port of the 2.6 one).
msg72588 - (view) Author: Simon Cross (hodgestar) Date: 2008-09-05 12:01
Attach recvfrom_into method patch for 3.0.
msg72601 - (view) Author: Simon Cross (hodgestar) Date: 2008-09-05 15:27
Tests for recv* and send* methods in 3.0 (2.6 tests coming shortly).
msg72603 - (view) Author: Simon Cross (hodgestar) Date: 2008-09-05 16:49
Test for recv* and send* in 2.6.

The tests revealed some bugs so this patch fixes those and supercedes
trunk-ssl-methods.patch.

Of particular note is that recv_into called self.read(buf, nbytes) which
isn't supported in _ssl.c in 2.6. I've worked around it by creating a
temporary buffer in the Python code. This isn't great for large
messages, but the alternative is to modify _ssl.c itself (which I
suspect is unlikely to make it in to 2.6 at this stage).
msg72604 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-09-05 16:58
> self.read(buf, nbytes)

Shouldn't this function be named readinto()?
msg72606 - (view) Author: Simon Cross (hodgestar) Date: 2008-09-05 17:05
>> self.read(buf, nbytes)

> Shouldn't this function be named readinto()?

There is no readinto function (and I suspect one is unlikely to be added
now). In Py3k SSLSocket.read takes both len and buffer as optional
arguments.
msg72607 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-09-05 17:08
I asked this because I find the signature misleading:
   read(len)
or read(buffer, len)

io.py, for example, defines read(len) and read_into(buffer).
msg72608 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-09-05 17:09
Simon, thanks, this patch looks good to me (2.6 only, right?).  I'll try
it out and report back.  If it looks good, I'll commit it.
msg72609 - (view) Author: Simon Cross (hodgestar) Date: 2008-09-05 17:15
@Bill Janssen:

There are currently two patch sets which I think should be applied. For
2.6 it's just trunk-ssl-tests.patch. For 3.0 it's 3k-ssl-methods.patch
and 3k-ssl-tests.patch.

@Amaury Forgeot d'Arc

I agree it's not great nomenclature but that's a whole separate can of
worms and not one I want to open in this bug (and in any case, it has
practically zero hope of making it into 2.6/3.0 at this point).
msg72611 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-09-05 17:24
Re: nomenclature

I think this is partly a design bug on my part, supporting the old
pre-2.6 "read" and "write" methods on the SSL context.  Users should
really call "makefile" to get something they can "read" and "write";
those methods should, in 2.6, only be internal, and not exposed outside
the class.  Of course, that would complicate the socket.ssl
compatibility code.
msg72781 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-09-08 16:38
I've applied Simon's patch to the 2.6 trunk.
msg72782 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-09-08 16:45
And for the 3K branch.  Thanks!
msg72788 - (view) Author: Simon Cross (hodgestar) Date: 2008-09-08 20:52
And thanks for looking at them and applying! :)
History
Date User Action Args
2008-09-08 20:52:59hodgestarsetmessages: + msg72788
2008-09-08 16:45:50janssensetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg72782
2008-09-08 16:38:11janssensetmessages: + msg72781
2008-09-05 17:24:28janssensetmessages: + msg72611
2008-09-05 17:15:18hodgestarsetmessages: + msg72609
2008-09-05 17:09:56janssensetmessages: + msg72608
2008-09-05 17:08:48amaury.forgeotdarcsetmessages: + msg72607
2008-09-05 17:05:03hodgestarsetmessages: + msg72606
2008-09-05 16:58:58amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg72604
2008-09-05 16:49:18hodgestarsetfiles: + trunk-ssl-tests.patch
messages: + msg72603
2008-09-05 15:27:50hodgestarsetfiles: + 3k-ssl-tests.patch
messages: + msg72601
2008-09-05 12:01:37hodgestarsetfiles: + 3k-ssl-methods.patch
messages: + msg72588
2008-09-05 12:00:25hodgestarsetfiles: + trunk-ssl-methods.patch
keywords: + patch
messages: + msg72586
2008-09-04 01:19:05janssensetmessages: + msg72448
2008-06-28 22:22:51janssensetmessages: + msg68921
2008-06-28 21:31:03janssensetassignee: janssen
resolution: accepted
messages: + msg68913
nosy: + janssen
2008-06-21 21:20:37giampaolo.rodolasetnosy: + giampaolo.rodola
2008-06-21 17:13:39hodgestarcreate