commit 9cf6af76eb250dc0716185e349a92b2def6981d3 Author: Nick Mathewson nickm@torproject.org Date: Wed May 25 11:52:52 2016 -0400
Fix a double-free bug in routerlist_reparse_old
I introduced this bug when I moved signing_key_cert into signed_descriptor_t. Bug not in any released Tor. Fixes bug 19175, and another case of 19128.
Just like signed_descriptor_from_routerinfo(), routerlist_reparse_old() copies the fields from one signed_descriptor_t to another, and then clears the fields from the original that would have been double-freed by freeing the original. But when I fixed the s_d_f_r() bug [#19128] in 50cbf220994c7cec593, I missed the fact that the code was duplicated in r_p_o().
Duplicated code strikes again!
For a longer-term solution here, I am not only adding the missing fix to r_p_o(): I am also extracting the duplicated code into a new function.
Many thanks to toralf for patiently sending me stack traces until one made sense. --- src/or/routerlist.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/or/routerlist.c b/src/or/routerlist.c index d49814f..a08b5f3 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -2938,6 +2938,19 @@ signed_descriptor_free(signed_descriptor_t *sd) tor_free(sd); }
+/** Copy src into dest, and steal all references inside src so that when + * we free src, we don't mess up dest. */ +static void +signed_descriptor_move(signed_descriptor_t *dest, + signed_descriptor_t *src) +{ + tor_assert(dest != src); + memcpy(dest, src, sizeof(signed_descriptor_t)); + src->signed_descriptor_body = NULL; + src->signing_key_cert = NULL; + dest->routerlist_index = -1; +} + /** Extract a signed_descriptor_t from a general routerinfo, and free the * routerinfo. */ @@ -2947,10 +2960,7 @@ signed_descriptor_from_routerinfo(routerinfo_t *ri) signed_descriptor_t *sd; tor_assert(ri->purpose == ROUTER_PURPOSE_GENERAL); sd = tor_malloc_zero(sizeof(signed_descriptor_t)); - memcpy(sd, &(ri->cache_info), sizeof(signed_descriptor_t)); - sd->routerlist_index = -1; - ri->cache_info.signed_descriptor_body = NULL; - ri->cache_info.signing_key_cert = NULL; + signed_descriptor_move(sd, &ri->cache_info); routerinfo_free(ri); return sd; } @@ -3436,9 +3446,7 @@ routerlist_reparse_old(routerlist_t *rl, signed_descriptor_t *sd) 0, 1, NULL, NULL); if (!ri) return NULL; - memcpy(&ri->cache_info, sd, sizeof(signed_descriptor_t)); - sd->signed_descriptor_body = NULL; /* Steal reference. */ - ri->cache_info.routerlist_index = -1; + signed_descriptor_move(&ri->cache_info, sd);
routerlist_remove_old(rl, sd, -1);