Hi, On 05/07/18 01:53, Steffan Karger wrote: [CUT]
> +bool
> +crypto_pem_decode(const char *name, struct buffer *dst,
> + const struct buffer *src)
> +{
> + bool ret = false;
> + BIO *bio;
> +
> + if (!(bio = BIO_new_mem_buf((char *)BPTR(src), BLEN(src))))
minor style niptick: I think it's common habit to have the assignment on
one line and the if-condition on the next one, especially when it's
about functions allocating some kind of object. Check your own
crypto_pem_encode() in this patch :-)
For the sake of consistency, I think this should be split in two lines:
BIO *bio = BIO_new_mem_buf((char *)BPTR(src), BLEN(src));
if (!bio)
> + {
> + crypto_msg(M_FATAL, "Cannot open memory BIO for PEM decode");
> + }
> +
> + char *name_read = NULL;
> + char *header_read = NULL;
> + uint8_t *data_read = NULL;
> + long data_read_len = 0;
> + if (!PEM_read_bio(bio, &name_read, &header_read, &data_read,
> + &data_read_len))
> + {
> + dmsg(D_CRYPT_ERRORS, "%s: PEM decode failed", __func__);
> + goto cleanup;
> + }
> +
> + if (strcmp(name, name_read))
> + {
> + dmsg(D_CRYPT_ERRORS,
> + "%s: unexpected PEM name (got '%s', expected '%s')",
> + __func__, name_read, name);
> + goto cleanup;
> + }
> +
> + uint8_t *dst_data = buf_write_alloc(dst, data_read_len);
> + if (!dst_data)
> + {
> + dmsg(D_CRYPT_ERRORS, "%s: dst too small (%i, needs %li)", __func__,
> + BCAP(dst), data_read_len);
> + goto cleanup;
Am I wrong or we are leaking dst_data in case of failure? or are we
assuming that this memory is now owned by dst and therefore its owner
(the caller) will take care of this?
> + }
> + memcpy(dst_data, data_read, data_read_len);
> +
> + ret = true;
> +cleanup:> + OPENSSL_free(name_read);
> + OPENSSL_free(header_read);
> + OPENSSL_free(data_read);
> + if (!BIO_free(bio))
> + {
> + ret = false;;
> + }
> +
> + return ret;
> +}
> +
[CUT]
Other than those small remarks the patch looks good.
Therefore:
Acked-by: Antonio Quartulli <anto...@openvpn.net>
This code does not do anything yet, as these functions are not used at
the moment (will be in one of the next patches), but they are tested
thanks to the new unit_test and everything seems to be working as expected.
Cheers,
--
Antonio Quartulli
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
