classification
Title: ssl.SSLSocket.write may fail on non-blocking sockets
Type: enhancement Stage: resolved
Components: Extension Modules Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Ben.Darnell, Ken.Giusti, benjamin.peterson, cbay, giampaolo.rodola, haypo, janssen, jcea, pitrou, python-dev, r.david.murray
Priority: normal Keywords: patch

Created on 2010-03-26 15:30 by cbay, last changed 2013-05-25 11:09 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
_ssl.c.patch cbay, 2010-03-26 15:30
test_ssl.py.patch cbay, 2010-03-26 15:35
test_ssl.py.patch.v2 cbay, 2010-03-26 18:05
ssl_mode.patch cbay, 2010-04-07 11:49 implements SSLSocket.get_mode/set_mode, made on trunk 44327.
Messages (27)
msg101753 - (view) Author: Cyril (cbay) Date: 2010-03-26 15:30
ssl.SSLSocket.write on non-blocking sockets will fail with:

_ssl.c:1217: error:1409F07F:SSL routines:SSL3_WRITE_PENDING:bad write retry

on a write retry, if the buffer address has changed between the initial call and the retry (when the initial call returned 0 bytes written, which means you should try again later).

From OpenSSL docs (http://www.openssl.org/docs/ssl/SSL_CTX_set_mode.html):

SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER
Make it possible to retry SSL_write() with changed buffer location (the buffer contents must stay the same). This is not the default to avoid the misconception that non-blocking SSL_write() behaves like non-blocking write().

Attached patch fixes the problem (tested on Python 2.6.5, 2.7 trunk) by calling SSL_CTX_set_mode with SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER. It's a single line patch.
msg101754 - (view) Author: Cyril (cbay) Date: 2010-03-26 15:35
The following test case exhibits the bug, but I'm not sure it will fail every time as it depends on 2 things:

 - your connection speed (I guess)
 - I used the following trick to have 2 identical strings with a different id (memory address):

data = (('xx'[0] + 'xx'[1:])*10000, ('xx'[0] + 'xx'[1:])*10000)

I'm not sure it will work all the time though.
msg101757 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2010-03-26 17:47
If I understood correctly, the patch only concerns non blocking socket if SSL_write() returns 0? If SSL_write() returns a non zero value, can you use: ssl_socket.send(data[count:])?

About the string identifier trick, you should add an assertion to ensure that identifiers are differents. Example:
--------
a = 'x' * 20000
# create a copy with a different memory address
b = a[0:] + a[1:]
assert (a == b) and (a is not b)
data = a, b
--------

See also issue #8222: enabling SSL_MODE_AUTO_RETRY on SSL sockets.
msg101761 - (view) Author: Cyril (cbay) Date: 2010-03-26 18:05
You're right about the assert, I've just uploaded a new patch.

In non-blocking mode, ssl_socket.send(data) will return either 0 (which means nothing was sent, you'll have to try again), or len(data) when everything was sent. It can't return anything inbetween. This is because SSL_MODE_ENABLE_PARTIAL_WRITE is not enabled.

In my opinion, SSL_MODE_ENABLE_PARTIAL_WRITE should probably be enabled, although I don't know if it would have any consequence on existing code. Note that _ssl.c header has:

   XXX should partial writes be enabled, SSL_MODE_ENABLE_PARTIAL_WRITE?

However, it's totally unrelated to our bug. Issue #8222 is also unrelated since SSL_MODE_AUTO_RETRY only applies to blocking sockets.

By the way, this bug was triaged "test needed". Am I missing anything? This is my first reported bug, I'm not sure about the process.
msg101762 - (view) Author: Cyril (cbay) Date: 2010-03-26 18:09
I forgot to talk about the conditions in which I stumbled upon that bug. I use a cStringIO.StringIO as a send buffer. When the socket is ready to send data, I call ssl_socket.send(send_buffer.getvalue()).

Unfortunately, two consecutive calls to send_buffer.getvalue() may return a different object (i.e. a string with a different memory address).
msg101763 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-03-26 18:26
"test needed" is in reference to your assertion that you weren't sure your test would fail reliably.  A test that fails some times and passes some times is...suboptimal when dealing with a buildbot testing infrastructure :)
msg101765 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-03-26 18:36
Since this error seems to be aimed at warning about potential programming errors, I'm not sure it should be silenced. The obvious fix should be to pass the same argument every time (until the data finally gets written).
msg101767 - (view) Author: Cyril (cbay) Date: 2010-03-26 18:41
r.david.murray: ah, sure :) However, I'm not sure a test case is absolutely required for this issue for two reasons:

 - the fix is trivial: it's a one-liner that enables a SSL mode that explicitely authorizes SSL_write to be called a second time with a a different memory pointer than the first time. Since memory pointers are opaque to Python programmers anyway, I doubt it could break code (unless you'd expect the failure, of course :) )

 - tests about SSL in non-blocking mode are almost inexistant, I think. The only one I could find tests the handshake. See issue #3890 for instance. Probably because writing tests in non-blocking mode isn't easy.

However, my test may be correct, I'm just not sure it will pass everywhere :)
msg101768 - (view) Author: Cyril (cbay) Date: 2010-03-26 18:47
pitrou: that's debatable, since the Python programmer has no control over memory pointers. As I said, I have a cStringIO buffer, and two consecutive calls to buffer.getvalue() yield different objects. What can I do about it? I think it's a rather sane scenario, and I don't feel I'm doing anything wrong.

If you think the programmer should be alerted about it, however, then we should at least say it explicitely in the documentation and probably return an explicit Python error. I had to google quite a bit before finding out what this error meant:

error:1409F07F:SSL routines:SSL3_WRITE_PENDING:bad write retry
msg101769 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-03-26 18:58
> pitrou: that's debatable, since the Python programmer has no control
> over memory pointers.

No, but he has control over whether he always uses the same object, or
generates a new argument everytime.

>  As I said, I have a cStringIO buffer, and two consecutive calls to
> buffer.getvalue() yield different objects. What can I do about it? I
> think it's a rather sane scenario, and I don't feel I'm doing anything
> wrong.

Hmm, indeed. What you can do, very simply, is cache the getvalue()
result once you have generated it.

> If you think the programmer should be alerted about it, however, then
> we should at least say it explicitely in the documentation and
> probably return an explicit Python error. I had to google quite a bit
> before finding out what this error meant:
> 
> error:1409F07F:SSL routines:SSL3_WRITE_PENDING:bad write retry

Indeed, this is cryptic.

By the way, I've found a thread explaining this in greater detail:
http://readlist.com/lists/openssl.org/openssl-users/0/1794.html

Basically, even when SSL_write() says the write must be retried, it does
process and buffer some of your data, so that if you retry with
different data, some junk will be written out on the SSL socket.
msg101770 - (view) Author: Cyril (cbay) Date: 2010-03-26 19:09
Switching to a documentation issue is fine to me. Indeed I can just cache the result of StringIO.getvalue(), although it feels a bit crude.

I won't be able to create a documentation patch since English is not my primary language. While you're at it, the doc says about SSLSocket.write:

Returns the number of bytes written.

It actually either returns 0 or len(data), at least as long as we don't have SSL partial writes. That's a different behaviour from regular sockets, and I had to look in _ssl.c to figure out why I never had values inbetween.
msg101826 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2010-03-27 13:29
> ..., the doc says about SSLSocket.write:
> 
> Returns the number of bytes written.
> 
> It actually either returns 0 or len(data), at least as long as we don't
>  have SSL partial writes. That's a different behaviour from regular
>  sockets, and I had to look in _ssl.c to figure out why I never had values
>  inbetween.

You should open a new issue for this point.
msg101944 - (view) Author: Cyril (cbay) Date: 2010-03-30 16:43
> Hmm, indeed. What you can do, very simply, is cache the getvalue()
> result once you have generated it.

After some thoughts, it's not really an option: my cStringIO.StringIO buffer is, well a buffer. To append data to the buffer, I call buffer.write(). When I've got a chance to send data over the socket (remember, it's async, so I don't really know when it's going to happen), I call buffer.getvalue().

If socket.write() returns zero byte written, I'll have to wait until I get another chance to send my buffer. But in the meantime, some more data might get appended to the buffer, and the string returned by getvalue() will be different from the first call (and thus, I can't really cache it).

I could find some tricks (like using multiple buffers), but it would be ugly.
msg101946 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-03-30 16:46
> If socket.write() returns zero byte written, I'll have to wait until I
> get another chance to send my buffer. But in the meantime, some more
> data might get appended to the buffer, and the string returned by
> getvalue() will be different from the first call (and thus, I can't
> really cache it).
> 
> I could find some tricks (like using multiple buffers), but it would
> be ugly.

Right. I think we should somehow support your use case, but I'm not sure
whether it should be the default.
msg101986 - (view) Author: Cyril (cbay) Date: 2010-03-31 09:26
I had a look at how M2Crypto and pyOpenSSL handled this:

 - M2Crypto has wrappers around SSL_set_mode that let you set the modes you want. From their changelog [1], it was required to be able to operate with Twisted. By default, though, they only set SSL_MODE_AUTO_RETRY.

 - pyOpenSSL enables everything by default, and there's no set_mode wrapper. Here is the relevant code:

/* Some initialization that's required to operate smoothly in Python */
SSL_CTX_set_mode(self->ctx, SSL_MODE_ENABLE_PARTIAL_WRITE |
                            SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER |
                            SSL_MODE_AUTO_RETRY);

I don't see any other possible alternative. I'm not sure which one is better. Implementing a set_mode wrapper with no mode set by default has no compatibility issues, although we'd still have that 'bad write retry' OpenSSL error.

On the other hand, setting SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER by default is easy but we lose some security (and, possibly, some compatibility problems, although I doubt anyone relies on the 'bad write retry' error).

What do you think? I'd be ready to write the patch for the set_mode wrapper if you want.
msg102535 - (view) Author: Cyril (cbay) Date: 2010-04-07 11:49
Here is a patch that implements SSLSocket.get_mode/set_mode, with the SSL_MODE_ENABLE_PARTIAL_WRITE and SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER constants defined in the ssl module.

The patch contains a test case and documentation. It's made against trunk 44327 and also applies nicely with --fuzz=3 on a 2.6.5.

There are no compatibility issues as no specific mode is set by default. It's up to the application to call SSLSocket.set_mode before use. I've tested my own use case with a set_mode(SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER), it works nicely.
msg102541 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-04-07 14:41
The patch adds a new feature, which makes it unsuitable for 2.6. I guess it could be applied to the 2.7 trunk, although a beta is being released and I'm not sure new features are really welcome afterwards. This one is really small and non-controversial, though, so I'd advocate accepting it.

The patch itself looks good.
msg102563 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2010-04-07 20:48
Wouldn't it be nicer if mode was a property?
msg102746 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-04-09 20:14
> Wouldn't it be nicer if mode was a property?

Good point. I guess it would indeed...
msg109637 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-07-08 22:48
Patch should probably be rewritten to add a `mode` property on the new SSLContext object instead.
msg138119 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-06-10 17:19
See issue12197 for a related request.
msg168547 - (view) Author: Ben Darnell (Ben.Darnell) * Date: 2012-08-19 06:15
Related pypy issue: https://bugs.pypy.org/issue1238
msg189763 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-05-21 15:08
I'm thinking that perhaps we should simply enable 
SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER by default. Ben, what do you think? Does the current behaviour allow to catch bugs?
msg189789 - (view) Author: Ben Darnell (Ben.Darnell) * Date: 2013-05-22 03:32
I vote for enabling SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER by default.  It can catch mistakes (i.e. failure to check the return value of send) in Python just as easily as in C, but I don't think those mistakes are common enough to be worth the headache of this error.  The false positive rate of this error is higher in Python than in C because we don't have direct control over memory and pointers.

As for partial writes, I'm not sure if it's backwards compatible to turn them on by default, but it might be nice if the option were exposed. Partial writes may have less benefit in Python than in C since we'd have to reallocate and copy a string instead of just moving a pointer.
msg189952 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-05-25 10:57
> As for partial writes, I'm not sure if it's backwards compatible to
> turn them on by default, but it might be nice if the option were
> exposed. Partial writes may have less benefit in Python than in C
> since we'd have to reallocate and copy a string instead of just moving 
> a pointer.

You can slice a memoryview() to avoid a copy. But I'm not sure of the point of partial writes here: can't you just send slices that are small enough (e.g. 4KB each)?
msg189954 - (view) Author: Roundup Robot (python-dev) Date: 2013-05-25 11:02
New changeset 60310223d075 by Antoine Pitrou in branch 'default':
Issue #8240: Set the SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER flag on SSL sockets.
http://hg.python.org/cpython/rev/60310223d075
msg189955 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-05-25 11:09
Ok, I should have fixed the original issue. If you want to see an option to enable partial writes, please open a separate issue.
History
Date User Action Args
2013-05-25 11:09:59pitrousetstatus: open -> closed
resolution: fixed
messages: + msg189955

stage: needs patch -> resolved
2013-05-25 11:02:41python-devsetnosy: + python-dev
messages: + msg189954
2013-05-25 10:57:18pitrousetmessages: + msg189952
2013-05-22 03:32:11Ben.Darnellsetmessages: + msg189789
2013-05-21 15:08:15pitrousettype: behavior -> enhancement
messages: + msg189763
versions: + Python 3.4, - Python 3.3
2013-05-21 13:00:07Ken.Giustisetnosy: + Ken.Giusti
2012-08-19 06:15:51Ben.Darnellsetnosy: + Ben.Darnell
messages: + msg168547
2011-06-16 14:25:30jceasetnosy: + jcea
2011-06-10 17:19:05pitrousetmessages: + msg138119
versions: + Python 3.3, - Python 3.2
2010-07-08 22:48:53pitrousetmessages: + msg109637
2010-07-08 22:48:03pitroulinkissue6587 superseder
2010-07-08 22:47:49pitrousetstage: commit review -> needs patch
versions: + Python 3.2, - Python 2.6, Python 2.7
2010-04-09 20:14:33pitrousetmessages: + msg102746
2010-04-07 20:48:44benjamin.petersonsetmessages: + msg102563
2010-04-07 14:41:16pitrousetnosy: + benjamin.peterson

messages: + msg102541
stage: test needed -> commit review
2010-04-07 11:49:42cbaysetfiles: + ssl_mode.patch

messages: + msg102535
2010-03-31 09:26:18cbaysetmessages: + msg101986
2010-03-30 16:46:46pitrousetmessages: + msg101946
2010-03-30 16:43:19cbaysetmessages: + msg101944
2010-03-27 13:29:48hayposetmessages: + msg101826
2010-03-26 19:09:14cbaysetmessages: + msg101770
2010-03-26 18:58:57pitrousetmessages: + msg101769
2010-03-26 18:47:39cbaysetmessages: + msg101768
2010-03-26 18:41:45cbaysetmessages: + msg101767
2010-03-26 18:36:19pitrousetnosy: + pitrou
messages: + msg101765
2010-03-26 18:30:35pitrousetnosy: + janssen
2010-03-26 18:26:54r.david.murraysetnosy: + r.david.murray
messages: + msg101763
2010-03-26 18:09:22cbaysetmessages: + msg101762
2010-03-26 18:05:02cbaysetfiles: + test_ssl.py.patch.v2

messages: + msg101761
2010-03-26 17:47:46hayposetmessages: + msg101757
2010-03-26 16:37:09hayposetnosy: + haypo
2010-03-26 15:57:19r.david.murraysetpriority: normal
type: behavior
stage: test needed
2010-03-26 15:57:01r.david.murraysetnosy: + giampaolo.rodola
components: + Extension Modules, - Library (Lib)
2010-03-26 15:35:19cbaysetfiles: + test_ssl.py.patch

messages: + msg101754
2010-03-26 15:30:33cbaycreate