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.

diff --git a/src/or/routerlist.c b/src/or/routerlist.c
index d49814f056d771ed5c1096c67d71abe72dadd5c0..a08b5f3190ccc854f69b57746f2809270af80066 100644 (file)
--- 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);