Return NULL from extend_info_from_node if the node has no allowed address

Modify callers to correctly handle these new NULL returns:
* fix assert in onion_extend_cpath
* warn and discard circuit in circuit_get_open_circ_or_launch
* warn, discard circuit, and tell controller in handle_control_extendcircuit

3 files changed, 45 insertions, 15 deletions

diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index dcb9de3a80..daf0b2af0b 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -2241,9 +2241,11 @@ onion_extend_cpath(origin_circuit_t *circ)
     if (r) {
       /* If we're a client, use the preferred address rather than the
          primary address, for potentially connecting to an IPv6 OR
-         port. */
-      info = extend_info_from_node(r, server_mode(get_options()) == 0);
-      tor_assert(info);
+         port. Servers always want the primary (IPv4) address. */
+      int client = (server_mode(get_options()) == 0);
+      info = extend_info_from_node(r, client);
+      /* Clients can fail to find an allowed address */
+      tor_assert(info || client);
     }
   } else {
     const node_t *r =
@@ -2318,34 +2320,43 @@ extend_info_new(const char *nickname, const char *digest,
  * <b>for_direct_connect</b> is true, in which case the preferred
  * address is used instead. May return NULL if there is not enough
  * info about <b>node</b> to extend to it--for example, if there is no
- * routerinfo_t or microdesc_t.
+ * routerinfo_t or microdesc_t, or if for_direct_connect is true and none of
+ * the node's addresses are allowed by tor's firewall and IP version config.
  **/
 extend_info_t *
 extend_info_from_node(const node_t *node, int for_direct_connect)
 {
   tor_addr_port_t ap;
+  int valid_addr = 0;
 
   if (node->ri == NULL && (node->rs == NULL || node->md == NULL))
     return NULL;
 
-  /* Choose a preferred address first, but fall back to an allowed address*/
+  /* Choose a preferred address first, but fall back to an allowed address.
+   * choose_address returns 1 on success, but get_prim_orport returns 0. */
   if (for_direct_connect)
-    fascist_firewall_choose_address_node(node, FIREWALL_OR_CONNECTION, 0, &ap);
+    valid_addr = fascist_firewall_choose_address_node(node,
+                                                      FIREWALL_OR_CONNECTION,
+                                                      0, &ap);
   else
-    node_get_prim_orport(node, &ap);
+    valid_addr = !node_get_prim_orport(node, &ap);
 
-  log_debug(LD_CIRC, "using %s for %s",
-            fmt_addrport(&ap.addr, ap.port),
-            node->ri ? node->ri->nickname : node->rs->nickname);
+  if (valid_addr)
+    log_debug(LD_CIRC, "using %s for %s",
+              fmt_addrport(&ap.addr, ap.port),
+              node->ri ? node->ri->nickname : node->rs->nickname);
+  else
+    log_warn(LD_CIRC, "Could not choose valid address for %s",
+              node->ri ? node->ri->nickname : node->rs->nickname);
 
-  if (node->ri)
+  if (valid_addr && node->ri)
     return extend_info_new(node->ri->nickname,
                              node->identity,
                              node->ri->onion_pkey,
                              node->ri->onion_curve25519_pkey,
                              &ap.addr,
                              ap.port);
-  else if (node->rs && node->md)
+  else if (valid_addr && node->rs && node->md)
     return extend_info_new(node->rs->nickname,
                              node->identity,
                              node->md->onion_pkey,
diff --git a/src/or/circuituse.c b/src/or/circuituse.c
index e742a5614f..4831f2be76 100644
--- a/src/or/circuituse.c
+++ b/src/or/circuituse.c
@@ -2006,8 +2006,13 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
         if (r && node_has_descriptor(r)) {
           /* We might want to connect to an IPv6 bridge for loading
              descriptors so we use the preferred address rather than
-             the primary.  */
+             the primary. */
           extend_info = extend_info_from_node(r, conn->want_onehop ? 1 : 0);
+          if (!extend_info) {
+            log_warn(LD_CIRC,"Could not make a one-hop connection to %s. "
+                     "Discarding this circuit.", conn->chosen_exit_name);
+            return -1;
+          }
         } else {
           log_debug(LD_DIR, "considering %d, %s",
                     want_onehop, conn->chosen_exit_name);
diff --git a/src/or/control.c b/src/or/control.c
index 66182fe2a4..2c0209ed85 100644
--- a/src/or/control.c
+++ b/src/or/control.c
@@ -2864,12 +2864,26 @@ handle_control_extendcircuit(control_connection_t *conn, uint32_t len,
   }
 
   /* now circ refers to something that is ready to be extended */
+  int first_node = zero_circ;
   SMARTLIST_FOREACH(nodes, const node_t *, node,
   {
-    extend_info_t *info = extend_info_from_node(node, 0);
-    tor_assert(info); /* True, since node_has_descriptor(node) == true */
+    extend_info_t *info = extend_info_from_node(node, first_node);
+    if (first_node && !info) {
+      log_warn(LD_CONTROL,
+               "controller tried to connect to a node that doesn't have any "
+               "addresses that are allowed by the firewall configuration; "
+               "circuit marked for closing.");
+      circuit_mark_for_close(TO_CIRCUIT(circ), -END_CIRC_REASON_CONNECTFAILED);
+      connection_write_str_to_buf("551 Couldn't start circuit\r\n", conn);
+      goto done;
+    } else {
+      /* True, since node_has_descriptor(node) == true and we are extending
+       * to the node's primary address */
+      tor_assert(info);
+    }
     circuit_append_new_exit(circ, info);
     extend_info_free(info);
+    first_node = 0;
   });
 
   /* now that we've populated the cpath, start extending */