Code review comment for lp:~ppergame/pyopenssl/next-proto

Revision history for this message
Jean-Paul Calderone (exarkun) wrote :

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

« Back to merge proposal