classification
Title: ssl.SSLSocket.recv() implementation may not work with non-blocking sockets
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: pitrou Nosy List: amaury.forgeotdarc, barry, ddvoinikov, exarkun, flox, giampaolo.rodola, janssen, jimsmyth, josiah.carlson, josiahcarlson, pitrou, qwavel, rhettinger, srid
Priority: high Keywords: needs review, patch

Created on 2008-09-17 20:35 by giampaolo.rodola, last changed 2010-03-24 16:35 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
ssl_noblock.patch amaury.forgeotdarc, 2008-12-03 16:45
Messages (37)
msg73342 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2008-09-17 20:35
As discussed on the python-dev ml I noticed something in the ssl.py code
which seems to be wrong. This is the ssl.SSLSocket.recv() method:

    def recv (self, buflen=1024, flags=0): 
        if self._sslobj: 
            if flags != 0: 
                raise ValueError( 
                    "non-zero flags not allowed in calls to sendall() 
on %s" % 
                    self.__class__) 
            while True: 
                try: 
                    return self.read(buflen) 
                except SSLError, x: 
                    if x.args[0] == SSL_ERROR_WANT_READ: 
                        continue 
                    else: 
                        raise x 
        else: 
            return socket.recv(self, buflen, flags) 

I don't know the low levels but that while statement which continues 
in case of SSL_ERROR_WANT_READ seems to be wrong (blocking), at least 
when dealing with non-blocking sockets. I think the proper way of 
doing recv() here is letting SSL_ERROR_WANT_READ propagate and let the 
upper application (e.g. asyncore) deal with it.
msg75054 - (view) Author: Josiah Carlson (josiahcarlson) * Date: 2008-10-21 22:30
I agree with Giampaolo.  In the case of non-blocking sockets, if reading 
from the ssl stream fails because there is no data on the socket, then 
sitting in a while loop is just going to busy-wait until data is 
discovered.

Never mind that the reference to "sendall" should be replaced by recv.

Whether to 'continue' or 'raise' should be determined by whether the 
socket is blocking.
msg75057 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-10-21 23:03
I agree, too.
msg76830 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2008-12-03 16:29
Wouldn't it be better to fix this before 3.0 gets released?
msg76832 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-12-03 16:45
I fear that is is too late for 3.0: the branch is already "frozen",
except maybe for special cases.

In any case a patch is needed. 
I am not a ssl expert, I just implemented the "let SSL_ERROR_WANT_READ
propagate".
Please review.
msg76907 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-12-04 18:21
Thanks for the patch.  It looks pretty good to me, but I can't help
thinking that there must be a better way of handling the recv() case; I
don't like copying that buffer several times (from the SSL code to
Python, from the Python to the buffer).
msg76961 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-12-05 01:27
> there must be a better way of handling the recv() case
This is a completely unrelated issue IMO; and FWIW, io.open() is not 
better in this aspect.
msg77569 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2008-12-10 21:23
Unrelated?  OK, but in fact I fixed this in 3.0 by providing a different
internal API to _ssl.c.  But you're right, that's an optimization.
msg92829 - (view) Author: Tom (qwavel) Date: 2009-09-18 16:21
I have just encountered this bug on Python 2.6.2 on Windows.  I hope the
fix makes it into 2.6.3.  Thanks for the patch.
msg92832 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2009-09-18 16:34
The patch looks good as it just removes the while loop.
This is a high priority issue and should be fixed as soon as possible, 
imho, as it makes impossible to use ssl module with non blocking sockets.
msg92877 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2009-09-19 20:38
Wouldn't it be nice to add a test for this case, to demonstrate that
non-blocking reads really are possible?
msg93135 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2009-09-25 20:14
I'll make this a release blocker, but I agree a test would be useful to
have.  Let's try to get this in for 2.6.3.
msg93314 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2009-09-29 18:54
I'd like to accept this for 2.6.3, but I'd also really like a test for
this change.  Giampaolo, do you think you could whip up a test for this.
 I know it's short notice.
msg93316 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2009-09-29 19:10
I could give it a try but I'm not sure I'll be in time for 2.6.3 release.
msg93317 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2009-09-29 19:11
Gaimpaolo thanks.  Please give it a try.
msg93324 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2009-09-29 19:47
Uhm... I'm sorry but actually I'm not sure about this patch anymore.
Now that I look at ssl.py again I'm noticing that send() is trapped in a 
"while True" loop as well and the patch doesn't cover it.

Not sure if that has been added recently or it was already there at the 
time I submitted the report but it's another thing that need to be 
fixed.


Moreover, I'm sure that removing the "while" loop is good for non-
blocking sockets but what about blocking ones?
Are SSL_ERROR_WANT_READ and SSL_ERROR_WANT_WRITE supposed to be raised 
in a blocking environment?
If they aren't then the current patch just needs to take care of send() 
method too, then it's fine.
msg93325 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2009-09-29 20:04
Thanks for the feedback Giampaolo.  It sounds like this patch is not yet
fully baked so I'll defer it to Python 2.6.4.
msg93327 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2009-09-29 20:11
You're very welcome.
My only concern is about the blocking environment.
If someone is able to confirm that those exceptions aren't raised with 
blocking sockets then modifying the patch is very trivial and maybe we 
could even make it for 2.6.3.
msg93328 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2009-09-29 20:14
We'll need tests to include it in 2.6.3.
msg93335 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2009-09-29 22:30
Not gonna make it for 2.6.3rc1
msg93337 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2009-09-29 23:09
I wonder if there is any way to test this, aside from the tests that
are already in the test suite?  The bug here is that the code effectively
does a blocking read on a non-blocking socket, and we can't tell the
difference.  The fact that this patch passes all the existing tests and
also seems to be a fix (through inspection) may have to be sufficient.

After all, the tests for the previous SSL support (Python 2.5 and
earlier) were extremely sketchy.
msg93383 - (view) Author: Bill Janssen (janssen) * (Python committer) Date: 2009-10-01 00:29
On Tue, Sep 29, 2009 at 12:47 PM, Giampaolo Rodola'
<report@bugs.python.org>wrote:

>
> Giampaolo Rodola' <billiejoex@users.sourceforge.net> added the comment:
>
> Uhm... I'm sorry but actually I'm not sure about this patch anymore.
> Now that I look at ssl.py again I'm noticing that send() is trapped in a
> "while True" loop as well and the patch doesn't cover it.
>
> Not sure if that has been added recently or it was already there at the
> time I submitted the report but it's another thing that need to be
> fixed.
>
>
> Moreover, I'm sure that removing the "while" loop is good for non-
> blocking sockets but what about blocking ones?
> Are SSL_ERROR_WANT_READ and SSL_ERROR_WANT_WRITE supposed to be raised
> in a blocking environment?
>

No.

> If they aren't then the current patch just needs to take care of send()
> method too, then it's fine.
>

Yes, it's fine.

Bill
msg93623 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-10-05 21:25
2.6.4?
msg93625 - (view) Author: Sridhar Ratnakumar (srid) Date: 2009-10-05 21:30
From http://bugs.python.org/msg93606 .. it seems that 2.6.4 may happen 
very soon.
msg94105 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2009-10-15 18:49
Antoine asked me to give my opinion on the non-blocking SSL API.  I can
say that the current behavior of SSLSocket.recv is certainly not very
good and probably makes SSLSocket useless for any non-blocking
application.  Letting the underlying want-read/want-write error bubble
up to the caller definitely makes more sense than blocking.

I haven't had a chance to look into the new SSL code in depth, so don't
construe this as a thorough review of the change, though.
msg95447 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-11-18 20:32
Based on the various comments the current patch should be ok, shouldn't it?
Although it would certainly be better with a patch ;)
msg95448 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-11-18 20:33
Oops. I meant "better with tests" of course...
msg101226 - (view) Author: Jim Smyth (jimsmyth) Date: 2010-03-17 14:08
Is there any chance of this being resolved in time for any 2.x release? I have a non-blocking application that I recently switched over to SSL and ran into this bug. I can work around it by trying to shorten the messages and beefing up the buflen, but these solutions are not ideal.
msg101236 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-03-17 17:27
Will apply after 2.6.5.
msg101449 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-03-21 19:40
I committed the patch to trunk (r79226), only to discover that it caused occasional failures in test_ftplib:

$ ./python -m test.regrtest -F test_ftplib
test_ftplib
test_ftplib
Exception in thread Thread-116:
Traceback (most recent call last):
  File "/home/antoine/cpython/__svn__/Lib/threading.py", line 530, in __bootstrap_inner
    self.run()
  File "/home/antoine/cpython/__svn__/Lib/test/test_ftplib.py", line 223, in run
    asyncore.loop(timeout=0.1, count=1)
  File "/home/antoine/cpython/__svn__/Lib/asyncore.py", line 211, in loop
    poll_fun(timeout, map)
  File "/home/antoine/cpython/__svn__/Lib/asyncore.py", line 148, in poll
    read(obj)
  File "/home/antoine/cpython/__svn__/Lib/asyncore.py", line 80, in read
    obj.handle_error()
  File "/home/antoine/cpython/__svn__/Lib/asyncore.py", line 76, in read
    obj.handle_read_event()
  File "/home/antoine/cpython/__svn__/Lib/test/test_ftplib.py", line 284, in handle_read_event
    super(SSLConnection, self).handle_read_event()
  File "/home/antoine/cpython/__svn__/Lib/asyncore.py", line 421, in handle_read_event
    self.handle_read()
  File "/home/antoine/cpython/__svn__/Lib/test/test_ftplib.py", line 39, in handle_read
    self.baseclass.last_received_data += self.recv(1024)
  File "/home/antoine/cpython/__svn__/Lib/test/test_ftplib.py", line 302, in recv
    return super(SSLConnection, self).recv(buffer_size)
  File "/home/antoine/cpython/__svn__/Lib/asyncore.py", line 370, in recv
    data = self.socket.recv(buffer_size)
  File "/home/antoine/cpython/__svn__/Lib/ssl.py", line 96, in <lambda>
    self.recv = lambda buflen=1024, flags=0: SSLSocket.recv(self, buflen, flags)
  File "/home/antoine/cpython/__svn__/Lib/ssl.py", line 215, in recv
    return self.read(buflen)
  File "/home/antoine/cpython/__svn__/Lib/ssl.py", line 136, in read
    return self._sslobj.read(len)
SSLError: [Errno 2] _ssl.c:1335: The operation did not complete (read)

test test_ftplib failed -- Traceback (most recent call last):
  File "/home/antoine/cpython/__svn__/Lib/test/test_ftplib.py", line 491, in test_storlines
    self.client.storlines('stor', f)
  File "/home/antoine/cpython/__svn__/Lib/ftplib.py", line 749, in storlines
    return self.voidresp()
  File "/home/antoine/cpython/__svn__/Lib/ftplib.py", line 224, in voidresp
    resp = self.getresp()
  File "/home/antoine/cpython/__svn__/Lib/ftplib.py", line 210, in getresp
    resp = self.getmultiline()
  File "/home/antoine/cpython/__svn__/Lib/ftplib.py", line 196, in getmultiline
    line = self.getline()
  File "/home/antoine/cpython/__svn__/Lib/ftplib.py", line 183, in getline
    line = self.file.readline()
  File "/home/antoine/cpython/__svn__/Lib/socket.py", line 445, in readline
    data = self._sock.recv(self._rbufsize)
  File "/home/antoine/cpython/__svn__/Lib/ssl.py", line 96, in <lambda>
    self.recv = lambda buflen=1024, flags=0: SSLSocket.recv(self, buflen, flags)
  File "/home/antoine/cpython/__svn__/Lib/ssl.py", line 215, in recv
    return self.read(buflen)
  File "/home/antoine/cpython/__svn__/Lib/ssl.py", line 136, in read
    return self._sslobj.read(len)
SSLError: The read operation timed out


Giampaolo, do you think the test is flaky?
msg101450 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-03-21 19:57
(using SSL_MODE_AUTO_RETRY doesn't fix the test_ftplib issue)
msg101455 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-03-21 20:49
The intuitive explanation seems to be:
- there are some bytes available for reading on the *TCP socket*, therefore asyncore calls the read handler
- however, there are not enough bytes for OpenSSL to actually decrypt any data, which is why we get SSL_ERROR_WANT_READ when trying to read from the *SSL socket*

The following patch seems to fix test_ftplib; any thoughts?


Index: Lib/test/test_ftplib.py
===================================================================
--- Lib/test/test_ftplib.py	(révision 79224)
+++ Lib/test/test_ftplib.py	(copie de travail)
@@ -293,7 +293,9 @@
             try:
                 return super(SSLConnection, self).send(data)
             except ssl.SSLError, err:
-                if err.args[0] in (ssl.SSL_ERROR_EOF, ssl.SSL_ERROR_ZERO_RETURN):
+                if err.args[0] in (ssl.SSL_ERROR_EOF, ssl.SSL_ERROR_ZERO_RETURN,
+                                   ssl.SSL_ERROR_WANT_READ,
+                                   ssl.SSL_ERROR_WANT_WRITE):
                     return 0
                 raise
 
@@ -301,6 +303,9 @@
             try:
                 return super(SSLConnection, self).recv(buffer_size)
             except ssl.SSLError, err:
+                if err.args[0] in (ssl.SSL_ERROR_WANT_READ,
+                                   ssl.SSL_ERROR_WANT_WRITE):
+                    return ''
                 if err.args[0] in (ssl.SSL_ERROR_EOF, ssl.SSL_ERROR_ZERO_RETURN):
                     self.handle_close()
                     return ''
msg101483 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2010-03-22 07:04
This last issue seems related to SSL 0.9.8m:
http://bugs.python.org/issue8108
msg101484 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-03-22 08:22
> The intuitive explanation seems to be:
> - there are some bytes available for reading on the *TCP socket*, 
> therefore asyncore calls the read handler
> - however, there are not enough bytes for OpenSSL to actually decrypt 
> any data, which is why we get SSL_ERROR_WANT_READ when trying to read 
> from the *SSL socket*
> The following patch seems to fix test_ftplib; any thoughts?

The patch seems ok to me. This is how it was supposed to be in the first place if ssl.py behaved as expected with non blocking sockets.
msg101507 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-03-22 13:58
> This last issue seems related to SSL 0.9.8m:
> http://bugs.python.org/issue8108

I don't think so:

$ rpm -qv openssl
openssl-0.9.8k-5.1mdv2010.0
msg101513 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-03-22 14:57
> The patch seems ok to me. This is how it was supposed to be in the
> first place if ssl.py behaved as expected with non blocking sockets.

Ok, patch applied.

In light of the recv() and recv_into() implementation change, I also
think we should enable SSL_MODE_AUTO_RETRY for SSL sockets. It prevents
blocking read() calls from getting SSL_ERROR_WANT_READ at all.
(previously, we would loop manually in recv() and recv_into(); letting
the C OpenSSL runtime do it for us is certainly more efficient)

See description in
http://www.openssl.org/docs/ssl/SSL_CTX_set_mode.html:

« SSL_MODE_AUTO_RETRY

        Never bother the application with retries if the transport is
        blocking. If a renegotiation take place during normal operation,
        a SSL_read(3) or SSL_write(3) would return with -1 and indicate
        the need to retry with SSL_ERROR_WANT_READ. In a non-blocking
        environment applications must be prepared to handle incomplete
        read/write operations. In a blocking environment, applications
        are not always prepared to deal with read/write operations
        returning without success report. The flag SSL_MODE_AUTO_RETRY
        will cause read/write operations to only return after the
        handshake and successful completion. »
msg101641 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-03-24 16:35
Ok, I've opened a separate issue (issue8222) for the SSL_MODE_AUTO_RETRY suggestion.

This very issue has been fixed in r79226 (trunk), r79287 (py3k), r79290 (3.1) and r79291 (2.6).
History
Date User Action Args
2010-03-24 16:35:55pitrousetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg101641

stage: commit review -> resolved
2010-03-22 14:57:46pitrousetmessages: + msg101513
2010-03-22 13:58:23pitrousetmessages: + msg101507
2010-03-22 08:22:39giampaolo.rodolasetmessages: + msg101484
2010-03-22 07:04:15floxsetnosy: + flox
messages: + msg101483
2010-03-21 20:49:37pitrousetmessages: + msg101455
2010-03-21 19:57:23pitrousetmessages: + msg101450
2010-03-21 19:40:59pitrousetmessages: + msg101449
2010-03-17 17:27:48pitrousetassignee: pitrou
resolution: accepted
messages: + msg101236
stage: patch review -> commit review
2010-03-17 14:08:46jimsmythsetnosy: + jimsmyth
messages: + msg101226
2009-11-18 20:33:20pitrousetmessages: + msg95448
2009-11-18 20:32:50pitrousetversions: + Python 3.1, Python 2.7, Python 3.2, - Python 3.0
nosy: + pitrou

messages: + msg95447

assignee: barry -> (no value)
stage: patch review
2009-10-15 18:49:24exarkunsetmessages: + msg94105
2009-10-05 21:30:10sridsetmessages: + msg93625
2009-10-05 21:25:30rhettingersetassignee: barry

messages: + msg93623
nosy: + rhettinger
2009-10-01 11:02:33pitrousetfiles: - unnamed
2009-10-01 00:29:21janssensetfiles: + unnamed

messages: + msg93383
2009-09-29 23:09:45janssensetmessages: + msg93337
2009-09-29 22:30:22barrysetpriority: release blocker -> high

messages: + msg93335
2009-09-29 20:14:13barrysetpriority: high -> release blocker

messages: + msg93328
2009-09-29 20:11:54giampaolo.rodolasetmessages: + msg93327
2009-09-29 20:04:13barrysetpriority: release blocker -> high

messages: + msg93325
2009-09-29 19:51:51sridsetnosy: + srid
2009-09-29 19:47:31giampaolo.rodolasetmessages: + msg93324
2009-09-29 19:11:40barrysetmessages: + msg93317
2009-09-29 19:10:26giampaolo.rodolasetmessages: + msg93316
2009-09-29 18:54:57barrysetmessages: + msg93314
2009-09-25 20:14:13barrysetpriority: release blocker
nosy: + barry
messages: + msg93135

2009-09-19 20:38:03exarkunsetnosy: + exarkun
messages: + msg92877
2009-09-18 16:34:05giampaolo.rodolasetmessages: + msg92832
2009-09-18 16:21:48qwavelsetnosy: + qwavel
messages: + msg92829
2008-12-10 21:23:03janssensetmessages: + msg77569
2008-12-05 01:27:06amaury.forgeotdarcsetmessages: + msg76961
2008-12-04 18:21:27janssensetmessages: + msg76907
2008-12-03 16:45:02amaury.forgeotdarcsetkeywords: + patch, needs review
nosy: + amaury.forgeotdarc
messages: + msg76832
files: + ssl_noblock.patch
2008-12-03 16:29:42giampaolo.rodolasetmessages: + msg76830
2008-10-21 23:03:46janssensetmessages: + msg75057
2008-10-21 22:30:58josiahcarlsonsetmessages: + msg75054
2008-10-21 06:42:18ddvoinikovsetnosy: + ddvoinikov
2008-09-17 20:35:51giampaolo.rodolacreate