Code review comment for lp:~mmzeeman/pyopenssl/pkcs7-extensions

Revision history for this message
Ziga Seilnacht (zseil) wrote :

Hello,

I'm not sure if I'm the right person for reviewing this, but
here it goes:

1. X509Obj_Sequence_2_Stack:
 - `stack` should be freed in the error branch of the for loop.
 - `sk_X509_push` can fail in low memory conditions. Its return
   value should be checked and appropriate action taken.

2. crypto_pkcs7_sign:
 - What happens if the PKey key contains only a public key or
   no key at all? It would be nice if this case was tested.

3. X509Stack_2_Sequence:
 - Both `X509_dup` and `crypto_X509_New` can fail in low memory
   conditions. An appropriate action should be taken in that case.

4. crypto_PKCS7_get0_signers:
 - `signer_certs` don't get freed if the list was created without
   errors. This looks wrong to me.
 - Why are `signer_certs` freed with `sk_X509_pop_free` instead
   of the more conventional `sk_X509_free`?
 - I don't like the name of this method. Is there a good reason
   for 0 in the name, apart from compatibility with the original
   OpenSSL name?

5. Documentation:
 - Docstrings should describe the required parameters and their
   types, preferably in epydoc format.
 - Latex documentation needs to be written.

6. Tests:
 - TestCase methods should have docstrings explaining what they
   are testing.
 - It would be good if there were some tests against "known good"
   serialized data, either stored as a string in the module or
   generated by spawning openssl.

I'm not qualified to say anything about the API itself, since I
find the win32 API intriguing :)

Thanks for working on this!

Regards,
Ziga

review: Needs Fixing

« Back to merge proposal