Skip to content
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

use _ffi.from_buffer() to support bytearray #852

Merged
merged 14 commits into from Nov 18, 2019

Conversation

@dholth
Copy link
Contributor

dholth commented Jul 8, 2019

If you try to send a dict it produces this error message: TypeError: a bytes-like object is required, not 'dict'

It doesn't accept bytearray without from_buffer (cffi doesn't automatically convert that type for char* arguments)

@reaperhulk

This comment has been minimized.

Copy link
Member

reaperhulk commented Jul 8, 2019

Let’s put a test for a few invalid types here so we’ll know if cffi ever changes its exceptions on this.

@dholth

This comment has been minimized.

@dholth

This comment has been minimized.

Copy link
Contributor Author

dholth commented Jul 8, 2019

Same fix will need to apply to sendall(). Not sure about bio, since it is the other side of the encryption.

@njsmith

This comment has been minimized.

Copy link
Contributor

njsmith commented Jul 8, 2019

Technically speaking, you should call ffi.release on the cffi buffer after you're done with it. (Or, I think it has a context manager version too?)

The reason is: the way Python's buffer protocol works, as long as someone holds a reference to the raw data in the buffer, the original object is pinned. So for example, if you call from_buffer on a bytearray, and then try to resize the bytearray, you'll get an error, because if python let you do that then the buffer would start pointing into nothingness. So that's great, but it means that if we go creating a buffer inside this function, then we need to make sure that buffer is gone again when we return to the user, or else they'll be very confused when attempting to resize their bytearray randomly fails depending on whether the buffer's been garbage collected or not.

Extra annoyances:

  • the ability to explicitly release buffers was only added in cffi 0.12
  • it's impossible to actually test this, because currently on cpython the refcount gc rooms destructors deterministically, while on pypy they haven't actually implemented the buffer locking functionality. But I think it's still a good idea, for future proofing.
@dholth

This comment has been minimized.

Copy link
Contributor Author

dholth commented Jul 8, 2019

OK @njsmith how do you like this one? It has a compat no-op context manager for from_buffer.

Unfortunately from_buffer is smarter than the test mock. I moved the length test past from_buffer in one of the commits because the error message from from_buffer() "wrong type" is nicer than the error message from len() "doesn't have a length"

total_sent += result
left_to_send -= result

return total_sent

This comment has been minimized.

Copy link
@dholth

dholth Jul 8, 2019

Author Contributor

bugfix

dholth added 8 commits Jul 8, 2019
@dholth

This comment has been minimized.

Copy link
Contributor Author

dholth commented Jul 9, 2019

OK folks, it's about as good as I'm likely to make it.

Includes a bugfix for sendall() which previously did not return the number of bytes written.

Includes a test to make sure bio_write throws not when passed bytes, bytearray.

@njsmith

This comment has been minimized.

Copy link
Contributor

njsmith commented Jul 9, 2019

I'm not very familiar with pyopenssl's internals, but FWIW this looks fine to me.

@njsmith

This comment has been minimized.

Copy link
Contributor

njsmith commented Jul 9, 2019

@dholth btw, you have a flake8 failure:

tests/test_ssl.py:2103:80: E501 line too long (80 > 79 characters)
@dholth

This comment has been minimized.

Copy link
Contributor Author

dholth commented Jul 9, 2019

@dholth

This comment has been minimized.

Copy link
Contributor Author

dholth commented Jul 13, 2019

This likely improves performance too, with the simplified checking and doesn't-copy nature of from_buffer.

Copy link
Member

reaperhulk left a comment

Sorry about the ridiculous amount of time this review took. Thanks for working on this, looks good!

@reaperhulk reaperhulk merged commit 079c963 into pyca:master Nov 18, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.