Merge remote-tracking branch 'neel/b23094'

changes/bug23094

@@ -0,0 +1,4 @@
+  o Code simplification and refactoring:
+    - Make hsdir_index in node_t a hsdir_index_t rather than a pointer
+      as hsdir_index is always present. Also, we move hsdir_index_t into
+      or.h. Closes ticket 23094. Patch by Neel Chauhan.

src/or/hs_common.c

@@ -103,7 +103,7 @@ compare_digest_to_fetch_hsdir_index(const void *_key, const void **_member)
 {
   const char *key = _key;
   const node_t *node = *_member;
-  return tor_memcmp(key, node->hsdir_index->fetch, DIGEST256_LEN);
+  return tor_memcmp(key, node->hsdir_index.fetch, DIGEST256_LEN);
 }
 
 /* Helper function: The key is a digest that we compare to a node_t object
@@ -114,7 +114,7 @@ compare_digest_to_store_first_hsdir_index(const void *_key,
 {
   const char *key = _key;
   const node_t *node = *_member;
-  return tor_memcmp(key, node->hsdir_index->store_first, DIGEST256_LEN);
+  return tor_memcmp(key, node->hsdir_index.store_first, DIGEST256_LEN);
 }
 
 /* Helper function: The key is a digest that we compare to a node_t object
@@ -125,7 +125,7 @@ compare_digest_to_store_second_hsdir_index(const void *_key,
 {
   const char *key = _key;
   const node_t *node = *_member;
-  return tor_memcmp(key, node->hsdir_index->store_second, DIGEST256_LEN);
+  return tor_memcmp(key, node->hsdir_index.store_second, DIGEST256_LEN);
 }
 
 /* Helper function: Compare two node_t objects current hsdir_index. */
@@ -134,8 +134,8 @@ compare_node_fetch_hsdir_index(const void **a, const void **b)
 {
   const node_t *node1= *a;
   const node_t *node2 = *b;
-  return tor_memcmp(node1->hsdir_index->fetch,
+  return tor_memcmp(node1->hsdir_index.fetch,
-                    node2->hsdir_index->fetch,
+                    node2->hsdir_index.fetch,
                     DIGEST256_LEN);
 }
 
@@ -145,8 +145,8 @@ compare_node_store_first_hsdir_index(const void **a, const void **b)
 {
   const node_t *node1= *a;
   const node_t *node2 = *b;
-  return tor_memcmp(node1->hsdir_index->store_first,
+  return tor_memcmp(node1->hsdir_index.store_first,
-                    node2->hsdir_index->store_first,
+                    node2->hsdir_index.store_first,
                     DIGEST256_LEN);
 }
 
@@ -156,8 +156,8 @@ compare_node_store_second_hsdir_index(const void **a, const void **b)
 {
   const node_t *node1= *a;
   const node_t *node2 = *b;
-  return tor_memcmp(node1->hsdir_index->store_second,
+  return tor_memcmp(node1->hsdir_index.store_second,
-                    node2->hsdir_index->store_second,
+                    node2->hsdir_index.store_second,
                     DIGEST256_LEN);
 }
 
@@ -1288,18 +1288,15 @@ node_has_hsdir_index(const node_t *node)
 
   /* At this point, since the node has a desc, this node must also have an
    * hsdir index. If not, something went wrong, so BUG out. */
-  if (BUG(node->hsdir_index == NULL)) {
+  if (BUG(tor_mem_is_zero((const char*)node->hsdir_index.fetch,
-    return 0;
-  }
-  if (BUG(tor_mem_is_zero((const char*)node->hsdir_index->fetch,
                           DIGEST256_LEN))) {
     return 0;
   }
-  if (BUG(tor_mem_is_zero((const char*)node->hsdir_index->store_first,
+  if (BUG(tor_mem_is_zero((const char*)node->hsdir_index.store_first,
                           DIGEST256_LEN))) {
     return 0;
   }
-  if (BUG(tor_mem_is_zero((const char*)node->hsdir_index->store_second,
+  if (BUG(tor_mem_is_zero((const char*)node->hsdir_index.store_second,
                           DIGEST256_LEN))) {
     return 0;
   }

src/or/hs_common.h

@@ -156,19 +156,6 @@ typedef struct rend_service_port_config_t {
   char unix_addr[FLEXIBLE_ARRAY_MEMBER];
 } rend_service_port_config_t;
 
-/* Hidden service directory index used in a node_t which is set once we set
- * the consensus. */
-typedef struct hsdir_index_t {
-  /* HSDir index to use when fetching a descriptor. */
-  uint8_t fetch[DIGEST256_LEN];
-
-  /* HSDir index used by services to store their first and second
-   * descriptor. The first descriptor is chronologically older than the second
-   * one and uses older TP and SRV values. */
-  uint8_t store_first[DIGEST256_LEN];
-  uint8_t store_second[DIGEST256_LEN];
-} hsdir_index_t;
-
 void hs_init(void);
 void hs_free_all(void);
 

src/or/hs_control.c

@@ -39,9 +39,8 @@ hs_control_desc_event_requested(const ed25519_public_key_t *onion_pk,
    * can't pick a node without an hsdir_index. */
   hsdir_node = node_get_by_id(hsdir_rs->identity_digest);
   tor_assert(hsdir_node);
-  tor_assert(hsdir_node->hsdir_index);
   /* This is a fetch event. */
-  hsdir_index = hsdir_node->hsdir_index->fetch;
+  hsdir_index = hsdir_node->hsdir_index.fetch;
 
   /* Trigger the event. */
   control_event_hs_descriptor_requested(onion_address, REND_NO_AUTH,

src/or/hs_service.c

@@ -2304,8 +2304,8 @@ upload_descriptor_to_hsdir(const hs_service_t *service,
   /* Logging so we know where it was sent. */
   {
     int is_next_desc = (service->desc_next == desc);
-    const uint8_t *idx = (is_next_desc) ? hsdir->hsdir_index->store_second:
+    const uint8_t *idx = (is_next_desc) ? hsdir->hsdir_index.store_second:
-                                          hsdir->hsdir_index->store_first;
+                                          hsdir->hsdir_index.store_first;
     log_info(LD_REND, "Service %s %s descriptor of revision %" PRIu64
                       " initiated upload request to %s with index %s",
              safe_str_client(service->onion_address),

src/or/nodelist.c

@@ -225,7 +225,6 @@ node_get_or_create(const char *identity_digest)
 
   smartlist_add(the_nodelist->nodes, node);
   node->nodelist_idx = smartlist_len(the_nodelist->nodes) - 1;
-  node->hsdir_index = tor_malloc_zero(sizeof(hsdir_index_t));
 
   node->country = -1;
 
@@ -350,26 +349,26 @@ node_set_hsdir_index(node_t *node, const networkstatus_t *ns)
 
   /* Build the fetch index. */
   hs_build_hsdir_index(node_identity_pk, fetch_srv, fetch_tp,
-                       node->hsdir_index->fetch);
+                       node->hsdir_index.fetch);
 
   /* If we are in the time segment between SRV#N and TP#N, the fetch index is
      the same as the first store index */
   if (!hs_in_period_between_tp_and_srv(ns, now)) {
-    memcpy(node->hsdir_index->store_first, node->hsdir_index->fetch,
+    memcpy(node->hsdir_index.store_first, node->hsdir_index.fetch,
-           sizeof(node->hsdir_index->store_first));
+           sizeof(node->hsdir_index.store_first));
   } else {
     hs_build_hsdir_index(node_identity_pk, store_first_srv, store_first_tp,
-                         node->hsdir_index->store_first);
+                         node->hsdir_index.store_first);
   }
 
   /* If we are in the time segment between TP#N and SRV#N+1, the fetch index is
      the same as the second store index */
   if (hs_in_period_between_tp_and_srv(ns, now)) {
-    memcpy(node->hsdir_index->store_second, node->hsdir_index->fetch,
+    memcpy(node->hsdir_index.store_second, node->hsdir_index.fetch,
-           sizeof(node->hsdir_index->store_second));
+           sizeof(node->hsdir_index.store_second));
   } else {
     hs_build_hsdir_index(node_identity_pk, store_second_srv, store_second_tp,
-                         node->hsdir_index->store_second);
+                         node->hsdir_index.store_second);
   }
 
  done:
@@ -720,7 +719,6 @@ node_free_(node_t *node)
   if (node->md)
     node->md->held_by_nodes--;
   tor_assert(node->nodelist_idx == -1);
-  tor_free(node->hsdir_index);
   tor_free(node);
 }
 

src/or/or.h

@@ -894,8 +894,19 @@ rend_data_v2_t *TO_REND_DATA_V2(const rend_data_t *d)
 struct hs_ident_edge_conn_t;
 struct hs_ident_dir_conn_t;
 struct hs_ident_circuit_t;
-/* Stub because we can't include hs_common.h. */
+
-struct hsdir_index_t;
+/* Hidden service directory index used in a node_t which is set once we set
+ * the consensus. */
+typedef struct hsdir_index_t {
+  /* HSDir index to use when fetching a descriptor. */
+  uint8_t fetch[DIGEST256_LEN];
+
+  /* HSDir index used by services to store their first and second
+   * descriptor. The first descriptor is chronologically older than the second
+   * one and uses older TP and SRV values. */
+  uint8_t store_first[DIGEST256_LEN];
+  uint8_t store_second[DIGEST256_LEN];
+} hsdir_index_t;
 
 /** Time interval for tracking replays of DH public keys received in
  * INTRODUCE2 cells.  Used only to avoid launching multiple
@@ -2561,7 +2572,7 @@ typedef struct node_t {
   /* Hidden service directory index data. This is used by a service or client
    * in order to know what's the hs directory index for this node at the time
    * the consensus is set. */
-  struct hsdir_index_t *hsdir_index;
+  struct hsdir_index_t hsdir_index;
 } node_t;
 
 /** Linked list of microdesc hash lines for a single router in a directory

src/test/test_hs_control.c

@@ -76,9 +76,8 @@ mock_node_get_by_id(const char *digest)
 {
   static node_t node;
   memcpy(node.identity, digest, DIGEST_LEN);
-  node.hsdir_index = tor_malloc_zero(sizeof(hsdir_index_t));
+  memset(node.hsdir_index.fetch, 'C', DIGEST256_LEN);
-  memset(node.hsdir_index->fetch, 'C', DIGEST256_LEN);
+  memset(node.hsdir_index.store_first, 'D', DIGEST256_LEN);
-  memset(node.hsdir_index->store_first, 'D', DIGEST256_LEN);
   return &node;
 }