msg73342 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) *  |
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) *  |
Date: 2008-10-21 23:03 |
I agree, too.
|
msg76830 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2009-09-29 19:11 |
Gaimpaolo thanks. Please give it a try.
|
msg93324 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) *  |
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) *  |
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) *  |
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) *  |
Date: 2009-09-29 20:14 |
We'll need tests to include it in 2.6.3.
|
msg93335 - (view) |
Author: Barry A. Warsaw (barry) *  |
Date: 2009-09-29 22:30 |
Not gonna make it for 2.6.3rc1
|
msg93337 - (view) |
Author: Bill Janssen (janssen) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2010-03-17 17:27 |
Will apply after 2.6.5.
|
msg101449 - (view) |
Author: Antoine Pitrou (pitrou) *  |
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) *  |
Date: 2010-03-21 19:57 |
(using SSL_MODE_AUTO_RETRY doesn't fix the test_ftplib issue)
|
msg101455 - (view) |
Author: Antoine Pitrou (pitrou) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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).
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:39 | admin | set | github: 48140 |
2010-03-24 16:35:55 | pitrou | set | status: open -> closed resolution: accepted -> fixed messages:
+ msg101641
stage: commit review -> resolved |
2010-03-22 14:57:46 | pitrou | set | messages:
+ msg101513 |
2010-03-22 13:58:23 | pitrou | set | messages:
+ msg101507 |
2010-03-22 08:22:39 | giampaolo.rodola | set | messages:
+ msg101484 |
2010-03-22 07:04:15 | flox | set | nosy:
+ flox messages:
+ msg101483
|
2010-03-21 20:49:37 | pitrou | set | messages:
+ msg101455 |
2010-03-21 19:57:23 | pitrou | set | messages:
+ msg101450 |
2010-03-21 19:40:59 | pitrou | set | messages:
+ msg101449 |
2010-03-17 17:27:48 | pitrou | set | assignee: pitrou resolution: accepted messages:
+ msg101236 stage: patch review -> commit review |
2010-03-17 14:08:46 | jimsmyth | set | nosy:
+ jimsmyth messages:
+ msg101226
|
2009-11-18 20:33:20 | pitrou | set | messages:
+ msg95448 |
2009-11-18 20:32:50 | pitrou | set | versions:
+ 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:24 | exarkun | set | messages:
+ msg94105 |
2009-10-05 21:30:10 | srid | set | messages:
+ msg93625 |
2009-10-05 21:25:30 | rhettinger | set | assignee: barry
messages:
+ msg93623 nosy:
+ rhettinger |
2009-10-01 11:02:33 | pitrou | set | files:
- unnamed |
2009-10-01 00:29:21 | janssen | set | files:
+ unnamed
messages:
+ msg93383 |
2009-09-29 23:09:45 | janssen | set | messages:
+ msg93337 |
2009-09-29 22:30:22 | barry | set | priority: release blocker -> high
messages:
+ msg93335 |
2009-09-29 20:14:13 | barry | set | priority: high -> release blocker
messages:
+ msg93328 |
2009-09-29 20:11:54 | giampaolo.rodola | set | messages:
+ msg93327 |
2009-09-29 20:04:13 | barry | set | priority: release blocker -> high
messages:
+ msg93325 |
2009-09-29 19:51:51 | srid | set | nosy:
+ srid
|
2009-09-29 19:47:31 | giampaolo.rodola | set | messages:
+ msg93324 |
2009-09-29 19:11:40 | barry | set | messages:
+ msg93317 |
2009-09-29 19:10:26 | giampaolo.rodola | set | messages:
+ msg93316 |
2009-09-29 18:54:57 | barry | set | messages:
+ msg93314 |
2009-09-25 20:14:13 | barry | set | priority: release blocker nosy:
+ barry messages:
+ msg93135
|
2009-09-19 20:38:03 | exarkun | set | nosy:
+ exarkun messages:
+ msg92877
|
2009-09-18 16:34:05 | giampaolo.rodola | set | messages:
+ msg92832 |
2009-09-18 16:21:48 | qwavel | set | nosy:
+ qwavel messages:
+ msg92829
|
2008-12-10 21:23:03 | janssen | set | messages:
+ msg77569 |
2008-12-05 01:27:06 | amaury.forgeotdarc | set | messages:
+ msg76961 |
2008-12-04 18:21:27 | janssen | set | messages:
+ msg76907 |
2008-12-03 16:45:02 | amaury.forgeotdarc | set | keywords:
+ patch, needs review nosy:
+ amaury.forgeotdarc messages:
+ msg76832 files:
+ ssl_noblock.patch |
2008-12-03 16:29:42 | giampaolo.rodola | set | messages:
+ msg76830 |
2008-10-21 23:03:46 | janssen | set | messages:
+ msg75057 |
2008-10-21 22:30:58 | josiahcarlson | set | messages:
+ msg75054 |
2008-10-21 06:42:18 | ddvoinikov | set | nosy:
+ ddvoinikov |
2008-09-17 20:35:51 | giampaolo.rodola | create | |