Hi,
On 20-07-18 13:20, Antonio Quartulli wrote:
> 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)
Agreed, will do.
>> + {
>> + 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?
I don't think this leaks data; buf_write_alloc returns NULL if there is
not enough space available in dst, and won't change dst in that case.
So nothing to clean up in that case?
>> + }
>> + 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.
Thanks!
-Steffan
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
