Code review comment for lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2

On Sat, Jul 18, 2009 at 03:04:46AM -0000, Jean-Paul Calderone wrote:

> 5. Hm. The way PKCS12 handles its CA certificates list has a hole. In trunk, crypto_PKCS12_New always creates cacerts, and makes it a tuple. This is good, since get_ca_certificates will return it directly, and if it were a list, callers could modify it in ways which would probably result in a crash later. With this branch, they can pass a list to set_ca_certificates. It might be valid at the time they pass it in, but then it can be mutated later. Requiring a tuple in crypto_PKCS12_set_ca_certificates instead of any sequence is probably a good idea.

Yeah, good point. crypto_PKCS12_export() has a commented-out assert
that could fix this, but I like your tuple conversion idea better.
I'll give it a try.

> 6. All the code handling the key, cert, and cacerts fields of crypto_PKCS12Obj structures would probably be simpler if only one of NULL or Py_None were possible values. I think that may really already be the case anyway. crypto_PKCS12_New starts of setting key and cert to NULL, but then immediately either sets them to a PyObject* or goes to the error case. What happens when the NULL checks for key, cert, and cacerts are removed, and just the Py_None checks are left in place (where necessary)?

Agreed. The PyObjects of crypto_PKCS12Obj are never NULL.
Fixed. The checks are gone.

Can we ever traverse an object after a clear? I left in that NULL check.

In theory could the PyDECREF() of self->key in crypto_PKCS12_clear()
call a desctructor which could access the PKCS12 object and cause
trouble with the now NULL self->cert? I suppose we should try
for a test case. Agressively eliminating NULL checks makes
me nervious, and if you let me, I'd leave them in.

> 8. Still in crypto_PKCS12_set_ca_certificates, PySequence_Length and PySequence_GetItem can have errors that should be checked for. I'm not sure if PyTuple_GetSize and PyTuple_GetItem (cf point 5) also can, maybe not.

Can PySequence_Length() fail after PySequence_Check() retuned
success? There is little point to PySequence_Check() then, as we
should just call PySequence_Length() and compare to zero. Likewise,
for PySequence_GetItem() if we within the range of PySequence_length().
I'd love to see a failing test case for this, if you can make one.

> 11. test_export_and_load is too long. :) Any chance this can be broken into a few shorter pieces?

Fixed. The monster has been slain. Where is my knighthood? :-)

>
> --
> https://code.launchpad.net/~rick-fdd/pyopenssl/pkcs12_mod_and_export2/+merge/8962
> You are the owner of lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2.

We should create set and get functions for friendly_name
instead of a parameter of export, so our API can better support
reading them from loaded certificates.

--
Rick