-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ssl.SSLSocket.write may fail on non-blocking sockets #52487
Comments
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 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:
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 bpo-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 bpo-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:
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 |
No, but he has control over whether he always uses the same object, or
Hmm, indeed. What you can do, very simply, is cache the getvalue()
Indeed, this is cryptic. By the way, I've found a thread explaining this in greater detail: Basically, even when SSL_write() says the write must be retried, it does |
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. |
You should open a new issue for this point. |
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. |
Right. I think we should somehow support your use case, but I'm not sure |
I had a look at how M2Crypto and pyOpenSSL handled this:
/* Some initialization that's required to operate smoothly in Python */ 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? |
Good point. I guess it would indeed... |
Patch should probably be rewritten to add a |
See bpo-12197 for a related request. |
Related pypy issue: https://bugs.pypy.org/issue1238 |
I'm thinking that perhaps we should simply enable |
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. |
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': |
Ok, I should have fixed the original issue. If you want to see an option to enable partial writes, please open a separate issue. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: