Merge remote-tracking branch 'dgoulet/bug14391_026_v2'

3 files changed, 113 insertions, 66 deletions

diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index 2a1a2f0fd2..670905f136 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -1508,23 +1508,45 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
     log_info(LD_REND,"Got a hidden service request for ID '%s'",
              safe_str_client(rend_data->onion_address));
 
-    /* see if we already have a hidden service descriptor cached for this
-     * address. */
+    /* Lookup the given onion address. If invalid, stop right now else we
+     * might have it in the cache or not, it will be tested later on. */
+    unsigned int refetch_desc = 0;
     rend_cache_entry_t *entry = NULL;
     const int rend_cache_lookup_result =
       rend_cache_lookup_entry(rend_data->onion_address, -1, &entry);
     if (rend_cache_lookup_result < 0) {
-      /* We should already have rejected this address! */
-      log_warn(LD_BUG,"Invalid service name '%s'",
-               safe_str_client(rend_data->onion_address));
-      connection_mark_unattached_ap(conn, END_STREAM_REASON_TORPROTOCOL);
-      return -1;
+      switch (-rend_cache_lookup_result) {
+      case EINVAL:
+        /* We should already have rejected this address! */
+        log_warn(LD_BUG,"Invalid service name '%s'",
+            safe_str_client(rend_data->onion_address));
+        connection_mark_unattached_ap(conn, END_STREAM_REASON_TORPROTOCOL);
+        return -1;
+      case ENOENT:
+        refetch_desc = 1;
+        break;
+      default:
+        log_warn(LD_BUG, "Unknown cache lookup error %d",
+            rend_cache_lookup_result);
+        return -1;
+      }
     }
 
     /* Help predict this next time. We're not sure if it will need
      * a stable circuit yet, but we know we'll need *something*. */
     rep_hist_note_used_internal(now, 0, 1);
 
+    /* Now we have a descriptor but is it usable or not? If not, refetch.
+     * Also, a fetch could have been requested if the onion address was not
+     * found in the cache previously. */
+    if (refetch_desc || !rend_client_any_intro_points_usable(entry)) {
+      base_conn->state = AP_CONN_STATE_RENDDESC_WAIT;
+      log_info(LD_REND, "Unknown descriptor %s. Fetching.",
+          safe_str_client(rend_data->onion_address));
+      rend_client_refetch_v2_renddesc(rend_data);
+      return 0;
+    }
+
     /* Look up if we have client authorization configured for this hidden
      * service.  If we do, associate it with the rend_data. */
     rend_service_authorization_t *client_auth =
@@ -1538,22 +1560,13 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
       rend_data->auth_type = client_auth->auth_type;
     }
 
-    /* Now, we either launch an attempt to connect to the hidden service,
-     * or we launch an attempt to look up its descriptor, depending on
-     * whether we had the descriptor. */
-    if (rend_cache_lookup_result == 0) {
-      base_conn->state = AP_CONN_STATE_RENDDESC_WAIT;
-      log_info(LD_REND, "Unknown descriptor %s. Fetching.",
-               safe_str_client(rend_data->onion_address));
-      rend_client_refetch_v2_renddesc(rend_data);
-    } else { /* rend_cache_lookup_result > 0 */
-      base_conn->state = AP_CONN_STATE_CIRCUIT_WAIT;
-      log_info(LD_REND, "Descriptor is here. Great.");
-      if (connection_ap_handshake_attach_circuit(conn) < 0) {
-        if (!base_conn->marked_for_close)
-          connection_mark_unattached_ap(conn, END_STREAM_REASON_CANT_ATTACH);
-        return -1;
-      }
+    /* We have the descriptor so launch a connection to the HS. */
+    base_conn->state = AP_CONN_STATE_CIRCUIT_WAIT;
+    log_info(LD_REND, "Descriptor is here. Great.");
+    if (connection_ap_handshake_attach_circuit(conn) < 0) {
+      if (!base_conn->marked_for_close)
+        connection_mark_unattached_ap(conn, END_STREAM_REASON_CANT_ATTACH);
+      return -1;
     }
     return 0;
   }
diff --git a/src/or/rendclient.c b/src/or/rendclient.c
index 162e0ac53e..14f311c1d5 100644
--- a/src/or/rendclient.c
+++ b/src/or/rendclient.c
@@ -141,7 +141,7 @@ rend_client_send_introduction(origin_circuit_t *introcirc,
   int r, v3_shift = 0;
   char payload[RELAY_PAYLOAD_SIZE];
   char tmp[RELAY_PAYLOAD_SIZE];
-  rend_cache_entry_t *entry;
+  rend_cache_entry_t *entry = NULL;
   crypt_path_t *cpath;
   off_t dh_offset;
   crypto_pk_t *intro_key = NULL;
@@ -158,8 +158,13 @@ rend_client_send_introduction(origin_circuit_t *introcirc,
   tor_assert(!(rendcirc->build_state->onehop_tunnel));
 #endif
 
-  if (rend_cache_lookup_entry(introcirc->rend_data->onion_address, -1,
-                              &entry) < 1) {
+  r = rend_cache_lookup_entry(introcirc->rend_data->onion_address, -1,
+                              &entry);
+  /* An invalid onion address is not possible else we have a big issue. */
+  tor_assert(r != -EINVAL);
+  if (r < 0 || !rend_client_any_intro_points_usable(entry)) {
+    /* If the descriptor is not found or the intro points are not usable
+     * anymore, trigger a fetch. */
     log_info(LD_REND,
              "query %s didn't have valid rend desc in cache. "
              "Refetching descriptor.",
@@ -739,7 +744,7 @@ rend_client_refetch_v2_renddesc(const rend_data_t *rend_query)
     return;
   }
   /* Before fetching, check if we already have a usable descriptor here. */
-  if (rend_cache_lookup_entry(rend_query->onion_address, -1, &e) > 0 &&
+  if (rend_cache_lookup_entry(rend_query->onion_address, -1, &e) == 0 &&
       rend_client_any_intro_points_usable(e)) {
     log_info(LD_REND, "We would fetch a v2 rendezvous descriptor, but we "
                       "already have a usable descriptor here. Not fetching.");
@@ -853,17 +858,26 @@ rend_client_report_intro_point_failure(extend_info_t *failed_intro,
   connection_t *conn;
 
   r = rend_cache_lookup_entry(rend_query->onion_address, -1, &ent);
-  if (r<0) {
-    log_warn(LD_BUG, "Malformed service ID %s.",
-             escaped_safe_str_client(rend_query->onion_address));
-    return -1;
-  }
-  if (r==0) {
-    log_info(LD_REND, "Unknown service %s. Re-fetching descriptor.",
-             escaped_safe_str_client(rend_query->onion_address));
-    rend_client_refetch_v2_renddesc(rend_query);
-    return 0;
+  if (r < 0) {
+    /* Either invalid onion address or cache entry not found. */
+    switch (-r) {
+    case EINVAL:
+      log_warn(LD_BUG, "Malformed service ID %s.",
+          escaped_safe_str_client(rend_query->onion_address));
+      return -1;
+    case ENOENT:
+      log_info(LD_REND, "Unknown service %s. Re-fetching descriptor.",
+          escaped_safe_str_client(rend_query->onion_address));
+      rend_client_refetch_v2_renddesc(rend_query);
+      return 0;
+    default:
+      log_warn(LD_BUG, "Unknown cache lookup returned code: %d", r);
+      return -1;
+    }
   }
+  /* The intro points are not checked here if they are usable or not because
+   * this is called when an intro point circuit is closed thus there must be
+   * at least one intro point that is usable and is about to be flagged. */
 
   for (i = 0; i < smartlist_len(ent->parsed->intro_nodes); i++) {
     rend_intro_point_t *intro = smartlist_get(ent->parsed->intro_nodes, i);
@@ -1062,7 +1076,7 @@ rend_client_desc_trynow(const char *query)
       continue;
     assert_connection_ok(base_conn, now);
     if (rend_cache_lookup_entry(rend_data->onion_address, -1,
-                                &entry) == 1 &&
+                                &entry) == 0 &&
         rend_client_any_intro_points_usable(entry)) {
       /* either this fetch worked, or it failed but there was a
        * valid entry from before which we should reuse */
@@ -1126,13 +1140,17 @@ rend_client_note_connection_attempt_ended(const char *onion_address)
 extend_info_t *
 rend_client_get_random_intro(const rend_data_t *rend_query)
 {
+  int ret;
   extend_info_t *result;
   rend_cache_entry_t *entry;
 
-  if (rend_cache_lookup_entry(rend_query->onion_address, -1, &entry) < 1) {
-      log_warn(LD_REND,
-               "Query '%s' didn't have valid rend desc in cache. Failing.",
-               safe_str_client(rend_query->onion_address));
+  ret = rend_cache_lookup_entry(rend_query->onion_address, -1, &entry);
+  if (ret < 0 || !rend_client_any_intro_points_usable(entry)) {
+    log_warn(LD_REND,
+             "Query '%s' didn't have valid rend desc in cache. Failing.",
+             safe_str_client(rend_query->onion_address));
+    /* XXX: Should we refetch the descriptor here if the IPs are not usable
+     * anymore ?. */
     return NULL;
   }
 
diff --git a/src/or/rendcommon.c b/src/or/rendcommon.c
index 5711f9e936..775d2b91c3 100644
--- a/src/or/rendcommon.c
+++ b/src/or/rendcommon.c
@@ -919,36 +919,52 @@ rend_valid_service_id(const char *query)
   return 1;
 }
 
-/** If we have a cached rend_cache_entry_t for the service ID <b>query</b>
- * with <b>version</b>, set *<b>e</b> to that entry and return 1.
- * Else return 0. If <b>version</b> is nonnegative, only return an entry
- * in that descriptor format version. Otherwise (if <b>version</b> is
- * negative), return the most recent format we have.
- */
+/** Lookup in the client cache the given service ID <b>query</b> for
+ * <b>version</b>.
+ *
+ * Return 0 if found and if <b>e</b> is non NULL, set it with the entry
+ * found. Else, a negative value is returned and <b>e</b> is untouched.
+ * -EINVAL means that <b>query</b> is not a valid service id.
+ * -ENOENT means that no entry in the cache was found. */
 int
 rend_cache_lookup_entry(const char *query, int version, rend_cache_entry_t **e)
 {
-  char key[REND_SERVICE_ID_LEN_BASE32+2]; /* <version><query>\0 */
+  int ret = 0;
+  char key[REND_SERVICE_ID_LEN_BASE32 + 2]; /* <version><query>\0 */
+  rend_cache_entry_t *entry = NULL;
+  static const int default_version = 2;
+
   tor_assert(rend_cache);
-  if (!rend_valid_service_id(query))
-    return -1;
-  *e = NULL;
-  if (version != 0) {
-    tor_snprintf(key, sizeof(key), "2%s", query);
-    *e = strmap_get_lc(rend_cache, key);
+  tor_assert(query);
+
+  if (!rend_valid_service_id(query)) {
+    ret = -EINVAL;
+    goto end;
   }
-  if (!*e && version != 2) {
-    tor_snprintf(key, sizeof(key), "0%s", query);
-    *e = strmap_get_lc(rend_cache, key);
+
+  switch (version) {
+  case 0:
+    log_warn(LD_REND, "Cache lookup of a v0 renddesc is deprecated.");
+    break;
+  case 2:
+    /* Default is version 2. */
+  default:
+    tor_snprintf(key, sizeof(key), "%d%s", default_version, query);
+    entry = strmap_get_lc(rend_cache, key);
+    break;
   }
-  if (!*e)
-    return 0;
-  tor_assert((*e)->parsed && (*e)->parsed->intro_nodes);
-  /* XXX023 hack for now, to return "not found" if there are no intro
-   * points remaining. See bug 997. */
-  if (! rend_client_any_intro_points_usable(*e))
-    return 0;
-  return 1;
+  if (!entry) {
+    ret = -ENOENT;
+    goto end;
+  }
+  tor_assert(entry->parsed && entry->parsed->intro_nodes);
+
+  if (e) {
+    *e = entry;
+  }
+
+end:
+  return ret;
 }
 
 /** Lookup the v2 service descriptor with base32-encoded <b>desc_id</b> and