Code review comment for lp:~hipl-core/hipl/hipv2-dh-ecdh

On 10/30/2012 03:52 AM, Diego Biurrun wrote:
> Review: Needs Fixing
>> + if (EC_POINT_get_affine_coordinates_GFp(EC_KEY_get0_group(key),
>> + EC_KEY_get0_public_key(key),
>> + pubx, puby, NULL) == 0) {
>> + HIP_ERROR("Failed to get x,y coordinates from the ECDH key\n");
>> + return -1;
>> + }
>> +
>> + len = BN_num_bytes(pubx);
>> + HIP_IFEL(outlen < len * 2, -1, "Output buffer too small\n");
>> +
>> + bn2bin_safe(pubx, out, outlen / 2);
>> + bn2bin_safe(puby, out + outlen / 2, outlen / 2);
>> +
>> + return outlen;
>> +
>> +out_err:
>> + BN_free(pubx);
>> + BN_free(puby);
>> + return err;
>> +}
> The use of the "err" variable is completely silly; it is never assigned
> to after initialization. Just get rid of it.

The "err" is used by HIP_IFEL.
BTW, I found another problem that I shouldn't do "return -1" after
"EC_POINT_get_affine_coordinates_GFp" because the failure of the
EC_POINT function will cause memleak on pubx and puby. It should be
fixed now.

>> --- libhipl/dh.c 2012-05-12 10:21:32 +0000
>> +++ libhipl/dh.c 2012-10-28 16:34:23 +0000
>> @@ -95,66 +97,288 @@
>> }
>>
>> /**
>> - * create a shared secret based on the public key of the peer
>> + * Store the bytes of the current ECDH public key in the given buffer.
>> + *
>> + * A new ECDH key will be created if it doesn't exist,
>> + *
>> + * @param buffer buffer to store the public part of the ECDH key
>> + * @param bufsize size of the @c buffer
>> + * @param group_id group ID of the ECDH key
>> + * @return the number of bytes written to the buffer, -1 on error.
>> + */
>> +int hip_insert_ecdh(uint8_t *buffer, int bufsize, int group_id)
>> +{
>> + EC_KEY *key;
>> + int ret;
>> +
>> + if (!hip_is_ecdh_group(group_id)) {
>> + HIP_ERROR("Invalid group id for ECDH: %d\n", group_id);
>> + return -1;
>> + }
>> +
>> + if (ecdh_table[group_id] == NULL) {
>> + key = hip_generate_ecdh_key(group_id);
>> + if (key == NULL) {
>> + HIP_ERROR("Failed to generate an ECDH key for group: %d\n",
>> + group_id);
>> + return -1;
>> + }
>> + ecdh_table[group_id] = key;
>> + }
>> +
>> + key = ecdh_table[group_id];
>> + if ((ret = hip_encode_ecdh_publickey(key, buffer, bufsize)) < 0) {
>> + HIP_ERROR("Failed to encode the ECDH public key\n");
>> + return -1;
>> + }
>> +
>> + return ret;
>> +}
> This looks awfully similar to hip_insert_dh().
>
>> +static int regen_dh_key(const int group_id)
>> {
>> DH *tmp, *okey;
>> +
>> + tmp = hip_generate_dh_key(group_id);
>> + if (!tmp) {
>> + HIP_INFO("Failed to generate a DH key for group: %d\n", group_id);
>> + return -1;
>> + }
>> +
>> + okey = dh_table[group_id];
>> + dh_table[group_id] = tmp;
>> +
>> + DH_free(okey);
>> + return 0;
>> +}
>> +
>> +#ifdef HAVE_EC_CRYPTO
>> +static int regen_ecdh_key(const int group_id)
>> +{
>> + EC_KEY *tmp, *okey;
>> +
>> + tmp = hip_generate_ecdh_key(group_id);
>> + if (!tmp) {
>> + HIP_INFO("Failed to generate an ECDH key for group: %d\n", group_id);
>> + return -1;
>> + }
>> +
>> + okey = ecdh_table[group_id];
>> + ecdh_table[group_id] = tmp;
>> +
>> + EC_KEY_free(okey);
>> + return 0;
>> +}
>> +#endif /* HAVE_EC_CRYPTO */
> These two also look awfully similar.
>
>
For these two comments, I don't think they are so similar that worth
merging together. They have similar logic but operate on different data
structures, also they are quite short.

All other comments should be fixed. See
http://bazaar.launchpad.net/~hipl-core/hipl/hipv2-dh-ecdh/revision/6269?remember=6269&compare_revid=6265
<http://bazaar.launchpad.net/%7Ehipl-core/hipl/hipv2-dh-ecdh/revision/6269?remember=6269&compare_revid=6265>
for my changes. Thanks.

--
Best regards,
Xin Gu