Code review comment for lp:~rene-hummen/hipl/puzzle-fixes

On 13.04.2011, at 19:15, Diego Biurrun wrote:

> Review: Needs Fixing
> review needs-fixing
>
> On Wed, Apr 13, 2011 at 08:50:53AM +0000, René Hummen wrote:
>> René Hummen has proposed merging lp:~rene-hummen/hipl/puzzle-fixes into lp:hipl.
>>
>> --- lib/core/builder.h 2011-04-02 20:38:36 +0000
>> +++ lib/core/builder.h 2011-04-13 08:50:40 +0000
>> @@ -126,11 +126,11 @@
>> -int hip_build_param_puzzle(struct hip_common *,
>> - uint8_t,
>> - uint8_t,
>> - uint32_t,
>> - uint64_t);
>> +int hip_build_param_puzzle(struct hip_common *const msg,
>> + const uint8_t val_K,
>> + const uint8_t lifetime,
>> + const uint32_t opaque,
>> + const uint8_t *const random_i);
>
> nit:
> Maybe you could add the parameter names while you're changing those
> lines anyway? That one of the most important headers in HIPL lacks
> parameter names in function declarations is extremely annoying.

No, I'm not going to fix builder.h here.

>> --- lib/core/solve.c 2011-01-24 10:21:17 +0000
>> +++ lib/core/solve.c 2011-04-13 08:50:40 +0000
>> @@ -43,108 +46,183 @@
>>
>> +static int hip_single_puzzle_computation(const struct puzzle_hash_input *const puzzle_input,
>> + const uint8_t difficulty)
>> {
>> + unsigned char sha_digest[SHA_DIGEST_LENGTH];
>> + uint32_t truncated_digest = 0;
>> + int err = -1;
>> +
>> + if (difficulty == 0) {
>> + return 0;
>> + }
>> +
>> + HIP_IFEL(difficulty > MAX_PUZZLE_DIFFICULTY,
>> + -1, "difficulty exceeds max. configured difficulty\n");
>> +
>> + HIP_IFEL(hip_build_digest(HIP_DIGEST_SHA1,
>> + puzzle_input,
>> + sizeof(struct puzzle_hash_input),
>> + sha_digest),
>> + -1, "failed to compute hash digest\n");
>> +
>> + if (ffs(truncated_digest) > difficulty) {
>> + err = 0;
>> + } else {
>> + err = 1;
>> + }
>> +
>> +out_err:
>> + return err;
>> +}
>
> You seem to insist on writing this with HIP_IFEL, but just compare how
> much neater this is with plain if-return (slightly redacted):
>
> static int hip_single_puzzle_computation(const struct puzzle_hash_input *const puzzle_input,
> const uint8_t difficulty)
> {
> unsigned char sha_digest[SHA_DIGEST_LENGTH];
> uint32_t truncated_digest = 0;
>
> if (difficulty == 0) {
> return 0;
> }
>
> if (difficulty > MAX_PUZZLE_DIFFICULTY) {
> HIP_ERROR("difficulty exceeds max. configured difficulty\n");
> return -1;
> }
>
> if (hip_build_digest(HIP_DIGEST_SHA1, puzzle_input,
> sizeof(struct puzzle_hash_input), sha_digest) {
> HIP_ERROR("failed to compute hash digest\n");
> return -1;
> }
>
> if (ffs(truncated_digest) > difficulty) {
> return 0;
> }
>
> return 1;
> }
>
> If this does not convince you, I can quote the relevant heuristics from
> "Clean Code" at you :)
>
> You can also make it slightly more compact by merging the if blocks with
> else statements if you prefer.

HIP_IFEL is the way errors are handled throughout the code. I won't change this here.

>> +int hip_solve_puzzle(const uint8_t puzzle[PUZZLE_LENGTH],
>> + const uint8_t difficulty,
>> + const hip_hit_t initiator_hit,
>> + const hip_hit_t responder_hit,
>> + uint8_t solution[PUZZLE_LENGTH])
>> +{
>> + struct puzzle_hash_input compute_puzzle;
>> +
>> + /* solution is correct iff:
>> + * 0 == V := Ltrunc( RHASH( I2.I | I2.hit_i | I2.hit_r | I2.J ), K )
>> + * (RFC 5201 - Appendix A) */
>> + memcpy(compute_puzzle.puzzle, puzzle, PUZZLE_LENGTH);
>> + compute_puzzle.initiator_hit = initiator_hit;
>> + compute_puzzle.responder_hit = responder_hit;
>
> Replace the memcpy by an assignment.

Wouldn't that just copy references?

>> --- lib/tool/pk.c 2011-04-04 13:59:21 +0000
>> +++ lib/tool/pk.c 2011-04-13 08:50:40 +0000
>> @@ -146,11 +146,13 @@
>>
>> HIP_IFEL(!(pz = hip_get_param_readwrite(msg, HIP_PARAM_PUZZLE)),
>> -ENOENT, "Illegal R1 packet (puzzle missing)\n");
>> - memcpy(opaque, pz->opaque, sizeof(pz->opaque));
>> - randi = pz->I;
>>
>> - memset(pz->opaque, 0, sizeof(pz->opaque));
>> - pz->I = 0;
>> + /* temporarily store original puzzle values */
>> + memcpy(opaque, pz->opaque, HIP_PUZZLE_OPAQUE_LEN);
>> + memcpy(rand_i, pz->I, PUZZLE_LENGTH);
>> + /* R1 signature is computed over zero values */
>> + memset(pz->opaque, 0, HIP_PUZZLE_OPAQUE_LEN);
>> + memset(pz->I, 0, PUZZLE_LENGTH);
>
> It seems these could be replaced by assignments as well...

Same here.

>> --- test/lib/core/solve.c 1970-01-01 00:00:00 +0000
>> +++ test/lib/core/solve.c 2011-04-13 08:50:40 +0000
>
> This file did not pass through uncrustify.

Of course it did not. Did you have a look recently at what the output would look like?

>> @@ -0,0 +1,200 @@
>> +/*
>> + * Copyright (c) 2010 Aalto University and RWTH Aachen University.
>
> Happy new year!
>
>> +START_TEST(test_hip_solve_puzzle_NULL_solution)
>> +{
>> + uint8_t puzzle[PUZZLE_LENGTH] = { 0 };
>> + uint8_t difficulty = 0;
>> + hip_hit_t initiator;
>> + hip_hit_t responder;
>
> uint8_t puzzle[PUZZLE_LENGTH] = { 0 };
> uint8_t difficulty = 0;
> hip_hit_t initiator, responder;
>
>> + RAND_bytes((unsigned char *)&puzzle, sizeof(uint64_t));
>
> Why not use the right type to begin with?
>
>> +START_TEST(test_hip_solve_puzzle_0_K)
>> +{
>> + uint8_t puzzle[PUZZLE_LENGTH] = { 0 };
>> + uint8_t difficulty = 0;
>> + hip_hit_t initiator;
>> + hip_hit_t responder;
>> + uint8_t solution[PUZZLE_LENGTH] = { 0 };
>
> uint8_t puzzle[PUZZLE_LENGTH] = { 0 };
> uint8_t solution[PUZZLE_LENGTH] = { 0 };
> uint8_t difficulty = 0;
> hip_hit_t initiator, responder;
>
>> + RAND_bytes((unsigned char *)&puzzle, sizeof(PUZZLE_LENGTH));
>
> Why not use the right type to begin with?
>
>> +START_TEST(test_hip_solve_puzzle_5_K)
>> +{
>> + uint8_t puzzle[PUZZLE_LENGTH] = { 0 };
>> + uint8_t difficulty = 5;
>> + hip_hit_t initiator;
>> + hip_hit_t responder;
>> + uint8_t solution[PUZZLE_LENGTH] = { 0 };
>
> uint8_t puzzle[PUZZLE_LENGTH] = { 0 };
> uint8_t solution[PUZZLE_LENGTH] = { 0 };
> uint8_t difficulty = 5;
> hip_hit_t initiator, responder;
>
>> + RAND_bytes((unsigned char *)&puzzle, sizeof(uint64_t));
>
> Why not use the right type to begin with?

Then I would need to cast here: hip_solve_puzzle().

>> +START_TEST(test_hip_test_hip_verify_puzzle_solution_against_real_solution)
>> +{
>> + char solution_dump[] = "\x01\x41\x00\x14\x05\x00\x48\x49\x0b\xbd\xd0\xb4"
>> + "\x92\xbb\xea\xe3\xa9\x71\x97\xdf\x12\xe1\x2c\x9f";
>> + struct hip_solution *solution = (struct hip_solution *) solution_dump;
>
> This variable indirection looks unnecessary, the cast makes it doubly ugly.

I didn't manage to get it work any other way.

René

--
Dipl.-Inform. Rene Hummen, Ph.D. Student
Chair of Communication and Distributed Systems
RWTH Aachen University, Germany
tel: +49 241 80 20772
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/