From c6de05d529dddebbdd7f9bcd36ca69d9a5e17d66 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 4 Jul 2016 11:33:41 -0400 Subject: [PATCH 1/3] sr: Remove useless code in disk_state_update() This patch also updates a comment in the same function for accuracy. Found by Coverity issue 1362985. Partily fixes #19567. Signed-off-by: David Goulet --- src/or/shared_random_state.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/or/shared_random_state.c b/src/or/shared_random_state.c index 73df9c43fb..870b992837 100644 --- a/src/or/shared_random_state.c +++ b/src/or/shared_random_state.c @@ -617,26 +617,24 @@ disk_state_update(void) /* Reset current disk state. */ disk_state_reset(); - /* First, update elements that we don't need to iterate over a list to - * construct something. */ + /* First, update elements that we don't need to do a construction. */ sr_disk_state->Version = sr_state->version; sr_disk_state->ValidUntil = sr_state->valid_until; sr_disk_state->ValidAfter = sr_state->valid_after; /* Shared random values. */ next = &sr_disk_state->SharedRandValues; - *next = NULL; if (sr_state->previous_srv != NULL) { *next = line = tor_malloc_zero(sizeof(config_line_t)); line->key = tor_strdup(dstate_prev_srv_key); disk_state_put_srv_line(sr_state->previous_srv, line); + /* Go to the next shared random value. */ next = &(line->next); } if (sr_state->current_srv != NULL) { *next = line = tor_malloc_zero(sizeof(*line)); line->key = tor_strdup(dstate_cur_srv_key); disk_state_put_srv_line(sr_state->current_srv, line); - next = &(line->next); } /* Parse the commits and construct config line(s). */ From 7d04638a608a150b80442198df401b8be5ea8a3a Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 4 Jul 2016 11:40:06 -0400 Subject: [PATCH 2/3] test: Fix shared random buffer overrun Encoded commit has an extra byte at the end for the NUL terminated byte and the test was overrunning the payload buffer by one byte. Found by Coverity issue 1362984. Fixes #19567 Signed-off-by: David Goulet --- src/test/test_shared_random.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/test/test_shared_random.c b/src/test/test_shared_random.c index d001785340..2bdce7d2d5 100644 --- a/src/test/test_shared_random.c +++ b/src/test/test_shared_random.c @@ -932,13 +932,14 @@ test_utils(void *arg) /* Testing commitments_are_the_same(). Currently, the check is to test the * value of the encoded commit so let's make sure that actually works. */ { - /* Payload of 55 bytes that is the length of - * sr_commit_t->encoded_commit. */ + /* Payload of 57 bytes that is the length of sr_commit_t->encoded_commit. + * 56 bytes of payload and a NUL terminated byte at the end ('\x00') + * which comes down to SR_COMMIT_BASE64_LEN + 1. */ const char *payload = "\x5d\xb9\x60\xb6\xcc\x51\x68\x52\x31\xd9\x88\x88\x71\x71\xe0\x30" "\x59\x55\x7f\xcd\x61\xc0\x4b\x05\xb8\xcd\xc1\x48\xe9\xcd\x16\x1f" "\x70\x15\x0c\xfc\xd3\x1a\x75\xd0\x93\x6c\xc4\xe0\x5c\xbe\xe2\x18" - "\xc7\xaf\x72\xb6\x7c\x9b\x52"; + "\xc7\xaf\x72\xb6\x7c\x9b\x52\x00"; sr_commit_t commit1, commit2; memcpy(commit1.encoded_commit, payload, sizeof(commit1.encoded_commit)); memcpy(commit2.encoded_commit, payload, sizeof(commit2.encoded_commit)); From cc34929abc3a5423372d1e12d47710ed366d43b4 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 4 Jul 2016 11:44:10 -0400 Subject: [PATCH 3/3] sr: Fix comment in shared_random.h Signed-off-by: David Goulet --- src/or/shared_random.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/or/shared_random.h b/src/or/shared_random.h index 51f3b550d8..0a2ec63a44 100644 --- a/src/or/shared_random.h +++ b/src/or/shared_random.h @@ -34,11 +34,11 @@ #define SR_SRV_MSG_LEN \ (SR_SRV_TOKEN_LEN + sizeof(uint64_t) + sizeof(uint32_t) + DIGEST256_LEN) -/* Length of base64 encoded commit NOT including the NULL terminated byte. - * Formula is taken from base64_encode_size. */ +/* Length of base64 encoded commit NOT including the NUL terminated byte. + * Formula is taken from base64_encode_size. This adds up to 56 bytes. */ #define SR_COMMIT_BASE64_LEN \ (((SR_COMMIT_LEN - 1) / 3) * 4 + 4) -/* Length of base64 encoded reveal NOT including the NULL terminated byte. +/* Length of base64 encoded reveal NOT including the NUL terminated byte. * Formula is taken from base64_encode_size. This adds up to 56 bytes. */ #define SR_REVEAL_BASE64_LEN \ (((SR_REVEAL_LEN - 1) / 3) * 4 + 4)