Add support for sending bytearrays. #622

Lukasa · 2017-04-25T09:58:40Z

Resolves #621.

This patch adds support for sending bytearrays by using ffi.from_buffer for both bytearrays and byte strings.

In general this patch is a good idea, however, it has some caveats. Most notable is that bytearray and byte string support for from_buffer only landed in cffi 1.8, but our lowest supported cryptography version only requires cffi 1.4. That may make this patch unacceptable. The minimum we could get away with would be cffi 1.7, and only then if we add separate code paths for byte strings and byte arrays.

On the other hand, if we could go up to requiring cffi 1.10 then we could remove the conditionals for handling memoryview and buffer objects, as ffi.from_buffer also supports them directly.

Anyway, here's a PoC, we can discuss whether it's acceptable or not.

codecov · 2017-04-25T10:04:33Z

Codecov Report

Merging #622 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #622      +/-   ##
=========================================
+ Coverage   96.78%   96.8%   +0.02%     
=========================================
  Files          16      16              
  Lines        5628    5667      +39     
  Branches      392     391       -1     
=========================================
+ Hits         5447    5486      +39     
  Misses        121     121              
  Partials       60      60

Impacted Files	Coverage Δ
src/OpenSSL/SSL.py	`94.82% <100%> (+0.05%)`	⬆️
tests/test_ssl.py	`99.1% <100%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7706e14...3c4357d. Read the comment docs.

hynek · 2017-04-25T10:11:07Z

Hm. I would assume most people run the latest cffi anyways. Since we already have a conditional, how about we externalize it into a function _is_acceptable_for_from_buffer() whose implementation depends on the cffi version?

N.B. I’m not telling, I’m asking. :)

Lukasa · 2017-04-25T10:15:55Z

I'm certainly open to that approach. It'd be interesting for @alex or @reaperhulk to weigh in with some relative usage data of different cffi versions, or at least with gut feelings, to work out how valuable that code will be, especially as it will suck to test. 😉

hynek · 2017-04-25T10:19:55Z

I think we could make it suck a lot less with something like this:

_is_acceptable_for_from_buffer = _make_buffer_checker(cffi.__version__)

Lukasa · 2017-04-25T10:21:32Z

Yeah, that's true, that could absolutely work.

reaperhulk · 2017-04-25T12:53:32Z

We already have some code in cryptography dependent on cffi >= 1.7, but we don't require it so we do this:

https://github.com/pyca/cryptography/blob/master/src/cryptography/hazmat/primitives/ciphers/base.py#L148-L160

We get coverage on that branch by testing an old pypy with coverage enabled. https://github.com/pyca/cryptography/blob/master/.travis.yml#L27

The only real consequence to upping minimum cffi version is that you leave behind older PyPys (since they can't upgrade it). cryptography decided against doing that, but this project can make its own call.

reaperhulk · 2017-04-25T13:02:31Z

Here are the number of downloads by implementation for the last month for pyopenssl:

1	2044707	CPython
2	108733	null	 
3	2238	PyPy	 
4	26	Jython	 
5	1	IronPython

reaperhulk · 2017-04-25T13:06:13Z

And here's the top 10 versions within that PyPy umbrella:

1	816	5.4.1	 
2	536	5.6.0	 
3	134	5.3.1	 
4	122	4.0.1	 
5	117	5.7.0	 
6	110	5.7.1	 
7	62	2.4.0	 
8	61	5.0.1	 
9	50	5.4.0	 
10	50	2.5.0

A (very) quick check shows that PyPy2 5.2 shipped with cffi 1.8 (5.1 shipped with 1.6).

hynek · 2017-04-26T08:35:25Z

I would just go the hybrid way I’ve sketched out. It seems like something we can test easily and we’ve learned the hard way, that dependencies don’t get auto-updated. :|

hynek · 2017-04-26T19:07:07Z

src/OpenSSL/SSL.py

+    """
+    cffi_version = tuple(
+        int(component) for component in
+        _CFFI_VERSION_RE.match(cffi_version_string).group(1, 2)


why not just cffi_version_string.split(".")? A RE seems overkill here?

Why not just from packaging.version import parse? Having pyOpenSSL depend on packaging (which is a dependency of both setuptools and cryptography) doesn't seem unreasonable.

@hynek The main reason is that you can conceivably have a version string like 1.8dev4, which will not behave nicely under .split('.'). It's possible I'm being too defensive here, but I was trying to avoid having everything explode in situations I didn't entirely forsee.

@reaperhulk That seems reasonable to me, if it's ok with @hynek I'll happily do that instead.

hynek · 2017-04-28T08:32:34Z

tests/test_ssl.py

+        buffer_factory = _make_buffer_checker(cffi_version)
+        cdata_buffer = buffer_factory(data)
+        # We need an explicit length here because cffi on recent PyPy doesn't
+        # 't correctly construct buffers when it knows the length of the cdata


't looks like an oops :)

hynek · 2017-04-28T08:33:24Z

@njsmith would this make you happy?

njsmith · 2017-04-29T04:20:35Z

src/OpenSSL/SSL.py

+    :param cffi_version_string: The version of CFFI in use, from
+        ffi.__version__.
+    """
+    cffi_version = parse(cffi_version_string)


A quick check shows that cffi has had a pre-parsed version tuple at cffi.__version_info__ for some time (I checked 1.0.0 and it had it). That might be nicer than pulling in a dependency on packaging?

njsmith · 2017-04-29T04:28:51Z

src/OpenSSL/SSL.py

+        if isinstance(buf, _memoryview):
+            buf = buf.tobytes()
+        if isinstance(buf, _buffer):
+            buf = str(buf)


It may not matter much in practice, but I think on recent CFFI these lines are unnecessary and are inefficient in that they impose an extra copy?

I might add a comment on the str line noting that this branch can only be taken on py2 (because py3 has no buffer objects)

They are, but the code refactor required to fix it and keep the tests in place was not something I wanted to undertake on this PR.

I don't think it matters really?

Yeah, not really; I only mention it because I had a moment of "wtf you can't call str on a buffer!" that took a few minutes to resolve :-)

njsmith · 2017-04-29T04:30:14Z

src/OpenSSL/SSL.py

+        ffi.__version__.
+    """
+    cffi_version = parse(cffi_version_string)
+    if cffi_version >= Version("1.7"):


The docs say "bytearray objects were supported in version 1.7 onwards ... and byte strings were supported in version 1.8 onwards." So the minimum version should probably be 1.8, not 1.7?

njsmith · 2017-04-29T04:41:06Z

Some comments above to do with as you wish. In general I am not too picky though, because I'm just using PyOpenSSL in my test suite, so I don't care about speed or dependencies or any of that :-). I was just surprised when I had to add a call to bytes(...) so I figured I'd be a good citizen and report it upstream...

Lukasa · 2017-04-29T11:21:20Z

Aside from the two points I addressed the rest are things I want to take action on, I'll do that sometime soon.

hynek · 2017-05-31T09:02:04Z

oh looks like this only needs docs + changelog? completely lost track of it!

dholth · 2019-07-08T14:08:44Z

I was about to suggest this same bugfix! I guess you don't like just letting cffi decide whether the buffer passed is acceptable or not? In SSL send() e.g. try: buf = _ffi.from_buffer(buf) except: pyopenssl error about buffers

It almost certainly calls from_buffer internally as part of any wrapped function that takes char*?

reaperhulk · 2019-07-08T15:41:31Z

@dholth I think at this point if you want to submit a new PR based on this we can look at that (@Lukasa is working on other projects and this is no longer an area of focus). When this PR was originally filed cryptography didn't require cffi >=1.8, but it does now so from_buffer will Just Work™.

dholth · 2019-07-08T16:35:41Z

Obsoleted by #852

mhils · 2022-05-20T12:54:11Z

This is fixed indeed, thanks @dholth! 🍰

dholth · 2022-05-20T13:12:58Z

Hooray!

Lukasa added 2 commits April 25, 2017 10:55

Add failing tests for sending bytearrays

b8e4968

Allow bytearray objects.

31dd761

Lukasa mentioned this pull request Apr 25, 2017

bytearray is a bytes-like object #621

Open

hynek mentioned this pull request Apr 25, 2017

Add an informative __main__.py #620

Merged

Lukasa force-pushed the bytearray branch 4 times, most recently from 2d47b8b to c80ed17 Compare April 26, 2017 15:24

hynek reviewed Apr 26, 2017

View reviewed changes

Lukasa force-pushed the bytearray branch from c80ed17 to be910b3 Compare April 27, 2017 09:44

hynek reviewed Apr 28, 2017

View reviewed changes

Lukasa force-pushed the bytearray branch from be910b3 to 6fac2e3 Compare April 28, 2017 08:48

njsmith reviewed Apr 29, 2017

View reviewed changes

Be compatible with older CFFI releases

3c4357d

Lukasa force-pushed the bytearray branch from 6fac2e3 to 3c4357d Compare April 29, 2017 13:22

chezou mentioned this pull request Apr 24, 2019

to_td function does not work from "requests" library v0.16.0 treasure-data/pandas-td#7

Closed

Base automatically changed from master to main February 13, 2021 17:50

mhils closed this May 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sending bytearrays. #622

Add support for sending bytearrays. #622

Lukasa commented Apr 25, 2017

codecov bot commented Apr 25, 2017 •

edited

Loading

hynek commented Apr 25, 2017

Lukasa commented Apr 25, 2017

hynek commented Apr 25, 2017

Lukasa commented Apr 25, 2017

reaperhulk commented Apr 25, 2017

reaperhulk commented Apr 25, 2017

reaperhulk commented Apr 25, 2017

hynek commented Apr 26, 2017

hynek Apr 26, 2017

reaperhulk Apr 26, 2017

Lukasa Apr 27, 2017

hynek Apr 27, 2017

hynek Apr 28, 2017

Lukasa Apr 28, 2017

hynek commented Apr 28, 2017

njsmith Apr 29, 2017

njsmith Apr 29, 2017

Lukasa Apr 29, 2017

njsmith Apr 29, 2017

njsmith Apr 29, 2017

njsmith commented Apr 29, 2017

Lukasa commented Apr 29, 2017

hynek commented May 31, 2017

dholth commented Jul 8, 2019

reaperhulk commented Jul 8, 2019

dholth commented Jul 8, 2019

mhils commented May 20, 2022

dholth commented May 20, 2022

Add support for sending bytearrays. #622

Add support for sending bytearrays. #622

Conversation

Lukasa commented Apr 25, 2017

codecov bot commented Apr 25, 2017 • edited Loading

Codecov Report

hynek commented Apr 25, 2017

Lukasa commented Apr 25, 2017

hynek commented Apr 25, 2017

Lukasa commented Apr 25, 2017

reaperhulk commented Apr 25, 2017

reaperhulk commented Apr 25, 2017

reaperhulk commented Apr 25, 2017

hynek commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hynek commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njsmith commented Apr 29, 2017

Lukasa commented Apr 29, 2017

hynek commented May 31, 2017

dholth commented Jul 8, 2019

reaperhulk commented Jul 8, 2019

dholth commented Jul 8, 2019

mhils commented May 20, 2022

dholth commented May 20, 2022

codecov bot commented Apr 25, 2017 •

edited

Loading