commit 672e2a5461c1a1f9160cadce4d9f1574856bf2cd Author: Nick Mathewson nickm@torproject.org Date: Thu Mar 16 11:04:58 2017 -0400

Consdiff: extract router ID hash iteration functions

There was a frequent block of code that did "find the next router line, see if we've hit the end of the list, get the ID hash from the line, and enforce well-ordering." Per Ahf's review, I'm extracting it to its own function. --- src/or/consdiff.c | 94 +++++++++++++++++++++++++++++++------------------------ 1 file changed, 53 insertions(+), 41 deletions(-)

diff --git a/src/or/consdiff.c b/src/or/consdiff.c index 2a97797..510ebd9 100644 --- a/src/or/consdiff.c +++ b/src/or/consdiff.c @@ -492,6 +492,46 @@ base64cmp(const cdline_t *hash1, const cdline_t *hash2) } }

+/** Structure used to remember the previous and current identity hash of + * the "r " lines in a consensus, to enforce well-ordering. */ +typedef struct router_id_iterator_t { + cdline_t last_hash; + cdline_t hash; +} router_id_iterator_t; + +/** + * Initializer for a router_id_iterator_t. + */ +#define ROUTER_ID_ITERATOR_INIT { { NULL, 0 }, { NULL, 0 } } + +/** Given an index *<b>idxp</b> into the consensus at <b>cons</b>, advance + * the index to the next router line ("r ...") in the consensus, or to + * an index one after the end of the list if there is no such line. + * + * Use <b>iter</b> to record the hash of the found router line, if any, + * and to enforce ordering on the hashes. If the hashes are mis-ordered, + * return -1. Else, return 0. + **/ +static int +find_next_router_line(const smartlist_t *cons, + const char *consname, + int *idxp, + router_id_iterator_t *iter) +{ + *idxp = next_router(cons, *idxp); + if (*idxp < smartlist_len(cons)) { + memcpy(&iter->last_hash, &iter->hash, sizeof(cdline_t)); + if (get_id_hash(smartlist_get(cons, *idxp), &iter->hash) < 0 || + base64cmp(&iter->hash, &iter->last_hash) <= 0) { + log_warn(LD_CONSDIFF, "Refusing to generate consensus diff because " + "the %s consensus doesn't have its router entries sorted " + "properly.", consname); + return -1; + } + } + return 0; +} + /** Generate an ed diff as a smartlist from two consensuses, also given as * smartlists. Will return NULL if the diff could not be generated, which can * happen if any lines the script had to add matched "." or if the routers @@ -529,8 +569,8 @@ gen_ed_diff(const smartlist_t *cons1, const smartlist_t *cons2, int start1=0, start2=0;

/* To check that hashes are ordered properly */ - cdline_t hash1 = { NULL, 0 }, hash2 = { NULL, 0 }; - cdline_t last_hash1 = { NULL, 0 }, last_hash2 = { NULL, 0 }; + router_id_iterator_t iter1 = ROUTER_ID_ITERATOR_INIT; + router_id_iterator_t iter2 = ROUTER_ID_ITERATOR_INIT;

/* i1 and i2 are initialized at the first line of each consensus. They never * reach past len1 and len2 respectively, since next_router doesn't let that @@ -545,30 +585,14 @@ gen_ed_diff(const smartlist_t *cons1, const smartlist_t *cons2, * yet at the end. */ if (i1 < len1) { - i1 = next_router(cons1, i1); - if (i1 != len1) { - memcpy(&last_hash1, &hash1, sizeof(cdline_t)); - if (get_id_hash(smartlist_get(cons1, i1), &hash1) < 0 || - base64cmp(&hash1, &last_hash1) <= 0) { - log_warn(LD_CONSDIFF, "Refusing to generate consensus diff because " - "the base consensus doesn't have its router entries " - "sorted properly."); - goto error_cleanup; - } + if (find_next_router_line(cons1, "base", &i1, &iter1) < 0) { + goto error_cleanup; } }

if (i2 < len2) { - i2 = next_router(cons2, i2); - if (i2 != len2) { - memcpy(&last_hash2, &hash2, sizeof(cdline_t)); - if (get_id_hash(smartlist_get(cons2, i2), &hash2) < 0 || - base64cmp(&hash2, &last_hash2) <= 0) { - log_warn(LD_CONSDIFF, "Refusing to generate consensus diff because " - "the target consensus doesn't have its router entries " - "sorted properly."); - goto error_cleanup; - } + if (find_next_router_line(cons2, "target", &i2, &iter2) < 0) { + goto error_cleanup; } }

@@ -587,10 +611,12 @@ gen_ed_diff(const smartlist_t *cons1, const smartlist_t *cons2, * consensus has already reached the end, both are extended to their * respecting ends since we are done. */ - int cmp = base64cmp(&hash1, &hash2); + int cmp = base64cmp(&iter1.hash, &iter2.hash); while (cmp != 0) { if (i1 < len1 && cmp < 0) { - i1 = next_router(cons1, i1); + if (find_next_router_line(cons1, "base", &i1, &iter1) < 0) { + goto error_cleanup; + } if (i1 == len1) { /* We finished the first consensus, so grab all the remaining * lines of the second consensus and finish up. @@ -598,16 +624,10 @@ gen_ed_diff(const smartlist_t *cons1, const smartlist_t *cons2, i2 = len2; break; } - memcpy(&last_hash1, &hash1, sizeof(cdline_t)); - if (get_id_hash(smartlist_get(cons1, i1), &hash1) < 0 || - base64cmp(&hash1, &last_hash1) <= 0) { - log_warn(LD_CONSDIFF, "Refusing to generate consensus diff " - "because the base consensus doesn't have its router entries " - "sorted properly."); + } else if (i2 < len2 && cmp > 0) { + if (find_next_router_line(cons2, "target", &i2, &iter2) < 0) { goto error_cleanup; } - } else if (i2 < len2 && cmp > 0) { - i2 = next_router(cons2, i2); if (i2 == len2) { /* We finished the second consensus, so grab all the remaining * lines of the first consensus and finish up. @@ -615,20 +635,12 @@ gen_ed_diff(const smartlist_t *cons1, const smartlist_t *cons2, i1 = len1; break; } - memcpy(&last_hash2, &hash2, sizeof(cdline_t)); - if (get_id_hash(smartlist_get(cons2, i2), &hash2) < 0 || - base64cmp(&hash2, &last_hash2) <= 0) { - log_warn(LD_CONSDIFF, "Refusing to generate consensus diff " - "because the target consensus doesn't have its router entries " - "sorted properly."); - goto error_cleanup; - } } else { i1 = len1; i2 = len2; break; } - cmp = base64cmp(&hash1, &hash2); + cmp = base64cmp(&iter1.hash, &iter2.hash); } }