homepage

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.

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.

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.

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.

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).

"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 :)

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).

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 :)

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

> 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.

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.

> ..., 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.

> 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.

> 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.

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.

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.

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.

Wouldn't it be nicer if mode was a property?

> Wouldn't it be nicer if mode was a property?
Good point. I guess it would indeed...

Patch should probably be rewritten to add a `mode` property on the new SSLContext object instead.

See issue12197 for a related request.

Related pypy issue: https://bugs.pypy.org/issue1238 

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?

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.

> 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)?

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 

Ok, I should have fixed the original issue. If you want to see an option to enable partial writes, please open a separate issue.