Code review comment for lp:~hipl-core/hipl/certificate-exchange

On Thu, Mar 08, 2012 at 06:41:47PM +0100, Diego Biurrun wrote:
> review needs-fixing
>
> On Fri, Mar 02, 2012 at 06:08:20PM +0000, Diego Biurrun wrote:
> > Diego Biurrun has proposed merging lp:~hipl-core/hipl/certificate-exchange into lp:hipl.
> >
> > --- test/lib/core/cert.c 1970-01-01 00:00:00 +0000
> > +++ test/lib/core/cert.c 2012-03-02 18:07:40 +0000
> > @@ -0,0 +1,189 @@
> > +
> > +START_TEST(test_cert_DER_encoding)
> > +{
> > + int len = 0;
> > + X509 *cert = NULL;
> > + X509 *cert_decoded = NULL;
> > + unsigned char *buf = NULL;
> > +
> > + HIP_DEBUG("Test DER en/decoding of X509 certificates.\n");
> > +
> > + fail_unless((cert = cert_load_x509_certificate(TEST_CERT,
> > + ENCODING_FORMAT_PEM)) != NULL,
> > + NULL);
> > + fail_unless((len = cert_X509_to_DER(cert, &buf)) > 0, NULL);
> > + fail_unless((cert_decoded = cert_DER_to_X509(buf, len)) != NULL, NULL);
> > + fail_unless(X509_cmp(cert, cert_decoded) == 0, NULL);
> > +
> > + fail_unless((len = cert_X509_to_DER(NULL, &buf)) < 0, NULL);
> > + fail_unless((len = cert_X509_to_DER(cert, NULL)) < 0, NULL);
> > + fail_unless((len = cert_X509_to_DER(NULL, NULL)) < 0, NULL);
> > +
> > + fail_unless((cert_decoded = cert_DER_to_X509(NULL, len)) == NULL, NULL);
> > + fail_unless((cert_decoded = cert_DER_to_X509(buf, len - 1)) == NULL, NULL);
> > + fail_unless((cert_decoded = cert_DER_to_X509(buf, len + 1)) == NULL, NULL);
> > + fail_unless((cert_decoded = cert_DER_to_X509(buf, 0)) == NULL, NULL);
> > +
> > + X509_free(cert);
> > + X509_free(cert_decoded);
>
> buf does not need to be freed?
>
> > +START_TEST(test_cert_match_public_key)
> > +{
> > + int err = 0;
> > + RSA *rsa = NULL;
> > + X509 *cert = NULL;
> > + EVP_PKEY *pkey = NULL;
> > +
> > + HIP_DEBUG("Test matching of public keys.\n");
> > +
> > + fail_unless((err = load_rsa_private_key(TEST_KEY, &rsa)) == 0, NULL);
> > + fail_unless((cert = cert_load_x509_certificate(TEST_CERT,
> > + ENCODING_FORMAT_PEM)) != NULL,
> > + NULL);
> > + pkey = EVP_PKEY_new();
> > + EVP_PKEY_assign_RSA(pkey, rsa);
> > + fail_unless((err = cert_match_public_key(cert, pkey)) == 1, NULL);
> > +
> > + fail_unless((err = cert_match_public_key(NULL, pkey)) == 0, NULL);
> > + fail_unless((err = cert_match_public_key(cert, NULL)) == 0, NULL);
> > + fail_unless((err = cert_match_public_key(NULL, NULL)) == 0, NULL);
> > +
> > + EVP_PKEY_free(pkey);
> > + X509_free(cert);
>
> RSA_free(rsa);
>
> .. at least if the doxy of load_rsa_private_key() is to be believed.
>
> > +START_TEST(test_cert_verify_chain)
> > +{
> > + int err = 0;
> > + X509 *cert = NULL;
> > + STACK_OF(X509) * chain = NULL;
> > +
> > + HIP_DEBUG("Test verification of certificate chains.\n");
> > +
> > + fail_unless((cert = cert_load_x509_certificate(TEST_CERT,
> > + ENCODING_FORMAT_PEM)) != NULL,
> > + NULL);
> > + fail_unless((chain = sk_X509_new_null()) != NULL, NULL);
> > + sk_X509_push(chain, cert);
> > + fail_unless((err = cert_verify_chain(cert, NULL, chain, NULL)) == 0, NULL);
> > +
> > + fail_unless((err = cert_verify_chain(NULL, NULL, chain, NULL)) != 0, NULL);
> > + fail_unless((err = cert_verify_chain(cert, NULL, NULL, NULL)) != 0, NULL);
> > +
> > + HIP_DEBUG("Successfully passed test for verification of certificate chains.\n");
> > +}
>
> I would guess that cert needs to be freed and chain popped or so - OpenSSL
> documentation is nothing but horrific.
>
> > +START_TEST(test_cert_get_X509_from_msg)
> > +{
> > + int len = 0;
> > + X509 *cert = NULL, *cert2 = NULL;
> > + struct hip_common *msg = NULL;
> > + unsigned char *buf = NULL;
> > +
> > + HIP_DEBUG("Test certificate extraction functionality.\n");
> > +
> > + fail_unless((cert = cert_load_x509_certificate(TEST_CERT,
> > + ENCODING_FORMAT_PEM)) != NULL,
> > + NULL);
> > + msg = hip_msg_alloc();
>
> Memory allocation can fail.
>
> > + hip_build_network_hdr(msg, HIP_UPDATE, 0, &in6addr_any, &in6addr_any);
> > + fail_unless((len = cert_X509_to_DER(cert, &buf)) > 0, NULL);
> > + fail_unless(hip_build_param_cert(msg, 0, 1, 1, HIP_CERT_X509V3, buf, len) == 0, NULL);
> > + fail_unless((cert2 = cert_get_X509_from_msg(msg)) != NULL, NULL);
> > + fail_unless(X509_cmp(cert, cert2) == 0, NULL);
> > +
> > + X509_free(cert);
> > + X509_free(cert2);
>
> What about msg and buf?

I fixed all the memory leaks in the unit tests and a few in the code.
At least all the ones that valgrind detected at the lowest warning
level. Turning up the warning volume drowns me in messages, as I
feared...

Diego