pyOpenSSL

Merge lp://staging/~ppergame/pyopenssl/next-proto into lp://staging/~exarkun/pyopenssl/trunk

next-proto
Merge into trunk

Proposed by Pavel Pergamenshchik on 2012-05-18

Status:	Work in progress
Proposed branch:	lp://staging/~ppergame/pyopenssl/next-proto
Merge into:	lp://staging/~exarkun/pyopenssl/trunk
Diff against target:	751 lines (+641/-0) 5 files modified OpenSSL/ssl/connection.c (+37/-0) OpenSSL/ssl/connection.h (+2/-0) OpenSSL/ssl/context.c (+374/-0) OpenSSL/ssl/context.h (+4/-0) OpenSSL/test/test_ssl.py (+224/-0)
To merge this branch:	bzr merge lp://staging/~ppergame/pyopenssl/next-proto
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Jean-Paul Calderone		2012-05-18	Needs Fixing on 2012-10-21
Review via email: mp+106293@code.staging.launchpad.net

Description of the change

Next Protocol Negotiation support. I tried not to leak any memory at the OpenSSL API boundaries

Revision history for this message

Jean-Paul Calderone (exarkun) wrote on 2012-10-21:

I just noticed that https://code.launchpad.net/~myers-1/pyopenssl/npn/+merge/92416 also exists. Which one of these should I choose to move forward on?

Revision history for this message

Jean-Paul Calderone (exarkun) wrote on 2012-10-21:

Looks like this one is more likely to see further activity, so I'll proceed here.

  1. There are some new XXXs in the code (copied from templates, I guess). It'd be nice if they were actually addressed instead. Since it's impractical to exercise malloc failures here, don't worry about unit testing. Just think really hard.
  2. Reference counting in ssl_Context_set_next_protos_advertised_cb is incorrect. I think it is possible for `self->next_protos_advertised_callback` to be the only thing keeping `self` alive, or perhaps for a weak reference callback or `__del__` on `self->next_protos_advertised_callback` to re-enter some part of ssl_Context_*. Since `self->next_protos_advertised_callback` is DECREFed while it is still in the `self` structure, this will probably lead to brokenness. I think this is what the Py_CLEAR macro (not heavily used throughout pyOpenSSL) is for. Same applies for next_protos_advertised_userdata, next_proto_select_callback, and next_proto_select_userdata. Maybe the (existing and new) code in `ssl_Context_clear` is fine though? I'm not clear on the re-entrancy issues of tp_clear, and I don't feel like digging them up.
  3. Some Python 3 issues in the Python code - except syntax (test_ssl.py, line 1125) and byte string literal syntax (b("foo") instead of "foo" to construct a byte string). I'm already set up to develop/test against Python 3.3 so I can take care of this if it's too much hassle for you, after the above points are addressed.

Thanks.

review: Needs Fixing

Unmerged revisions

178. By Pavel Pergamenshchik on 2012-05-18: rename get0 -> get, add proper test skipping
177. By Pavel Pergamenshchik on 2012-05-18: skip test if no NPN support in openssl
176. By Pavel Pergamenshchik on 2012-05-18: move the endif to the correct spot
175. By Pavel Pergamenshchik on 2012-05-18: pass the Connection to python next proto callbacks, use Connection threadstate
174. By Pavel Pergamenshchik on 2012-05-17: conditional compilation, more exceptions, more tests
173. By Pavel Pergamenshchik on 2012-05-15: test select returning none
172. By Pavel Pergamenshchik on 2012-05-15: get0_next_proto_negotiated tests
171. By Pavel Pergamenshchik on 2012-05-15: get0_next_proto_negotiated
170. By Pavel Pergamenshchik on 2012-05-10: npn advertisement compiles
169. By Pavel Pergamenshchik <ppergame@ppergame-glaptop> on 2012-05-10: untested next_proto_advertised_cb