use _ffi.from_buffer() to support bytearray

[dholth]

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]

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

[dholth]

Like this one? https://github.com/pyca/pyopenssl/pull/852/files#diff-72cbea3491325683d8c77fbeea9dd31aR2799

[dholth]

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

[njsmith]

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]

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"

[dholth]

bugfix

[dholth]

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]

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

[njsmith]

@dholth btw, you have a flake8 failure:

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

[dholth]

Missed it by that much

On Tue, Jul 9, 2019, at 5:59 PM, Nathaniel J. Smith wrote: @dholth <https://github.com/dholth> btw, you have a flake8 failure: `tests/test_ssl.py:2103:80: E501 line too long (80 > 79 characters)

`

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#852?email_source=notifications&email_token=AABSZEU5GNR6SKVWROUUQW3P6UC5FA5CNFSM4H65NEKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZRVGJY#issuecomment-509825831>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABSZEV4U5VX3YGTUTRBP3DP6UC5FANCNFSM4H65NEKA>.

[dholth]

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

[reaperhulk]

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

[rtkbkish]

This change was a lifesaver.
We were POSTing on different threads using pypy. We kept getting Error: [('SSL routines', 'SSL3_WRITE_PENDING', 'bad write retry')] when a write was retried after a WantWriteError.
Updating to this release cleared up the issue.