commit 5458ff20a5a8d76c6e8abe80167bd84fa157038f
Author: Neel Chauhan <neel(a)neelc.org>
Date: Fri Apr 27 19:14:59 2018 +0000
Remove the return value from the fascist_firewall_choose_address_* family of functions
---
changes/ticket24734 | 6 ++++
src/or/circuitbuild.c | 9 ++---
src/or/directory.c | 12 +++----
src/or/policies.c | 90 +++++++++++++++++++++-----------------------------
src/or/policies.h | 14 ++++----
src/test/test_policy.c | 14 +++-----
6 files changed, 63 insertions(+), 82 deletions(-)
diff --git a/changes/ticket24734 b/changes/ticket24734
new file mode 100644
index 000000000..00029ce57
--- /dev/null
+++ b/changes/ticket24734
@@ -0,0 +1,6 @@
+ o Code simplification and refactoring:
+ - Remove the return value for fascist_firewall_choose_address_base(),
+ and sister functions such as fascist_firewall_choose_address_node()
+ and fascist_firewall_choose_address_rs(). Also, while we're here,
+ initialize the ap argument as leaving it uninitialized can pose a
+ security hazard. Closes ticket 24734. Patch by Neel Chauhan.
diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index c33dbbeb2..24c32b710 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -2802,16 +2802,13 @@ extend_info_from_node(const node_t *node, int for_direct_connect)
return NULL;
}
- /* Choose a preferred address first, but fall back to an allowed address.
- * choose_address returns 1 on success, but get_prim_orport returns 0. */
+ /* Choose a preferred address first, but fall back to an allowed address. */
if (for_direct_connect)
- valid_addr = fascist_firewall_choose_address_node(node,
- FIREWALL_OR_CONNECTION,
- 0, &ap);
+ fascist_firewall_choose_address_node(node, FIREWALL_OR_CONNECTION, 0, &ap);
else {
node_get_prim_orport(node, &ap);
- valid_addr = tor_addr_port_is_valid_ap(&ap, 0);
}
+ valid_addr = tor_addr_port_is_valid_ap(&ap, 0);
if (valid_addr)
log_debug(LD_CIRC, "using %s for %s",
diff --git a/src/or/directory.c b/src/or/directory.c
index c419b61d0..d6ce9290a 100644
--- a/src/or/directory.c
+++ b/src/or/directory.c
@@ -794,9 +794,9 @@ directory_choose_address_routerstatus(const routerstatus_t *status,
* Use the preferred address and port if they are reachable, otherwise,
* use the alternate address and port (if any).
*/
- have_or = fascist_firewall_choose_address_rs(status,
- FIREWALL_OR_CONNECTION, 0,
- use_or_ap);
+ fascist_firewall_choose_address_rs(status, FIREWALL_OR_CONNECTION, 0,
+ use_or_ap);
+ have_or = tor_addr_port_is_valid_ap(use_or_ap, 0);
}
/* DirPort connections
@@ -805,9 +805,9 @@ directory_choose_address_routerstatus(const routerstatus_t *status,
indirection == DIRIND_ANON_DIRPORT ||
(indirection == DIRIND_ONEHOP
&& !directory_must_use_begindir(options))) {
- have_dir = fascist_firewall_choose_address_rs(status,
- FIREWALL_DIR_CONNECTION, 0,
- use_dir_ap);
+ fascist_firewall_choose_address_rs(status, FIREWALL_DIR_CONNECTION, 0,
+ use_dir_ap);
+ have_dir = tor_addr_port_is_valid_ap(use_dir_ap, 0);
}
/* We rejected all addresses in the relay's status. This means we can't
diff --git a/src/or/policies.c b/src/or/policies.c
index d03a251c2..e0dbb021c 100644
--- a/src/or/policies.c
+++ b/src/or/policies.c
@@ -825,9 +825,8 @@ fascist_firewall_choose_address(const tor_addr_port_t *a,
* If pref_only, only choose preferred addresses. In either case, choose
* a preferred address before an address that's not preferred.
* If both addresses could be chosen (they are both preferred or both allowed)
- * choose IPv6 if pref_ipv6 is true, otherwise choose IPv4.
- * If neither address is chosen, return 0, else return 1. */
-static int
+ * choose IPv6 if pref_ipv6 is true, otherwise choose IPv4. */
+static void
fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr,
uint16_t ipv4_orport,
uint16_t ipv4_dirport,
@@ -868,15 +867,12 @@ fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr,
if (result) {
tor_addr_copy(&ap->addr, &result->addr);
ap->port = result->port;
- return 1;
- } else {
- return 0;
}
}
/** Like fascist_firewall_choose_address_base(), but takes a host-order IPv4
* address as the first parameter. */
-static int
+static void
fascist_firewall_choose_address_ipv4h(uint32_t ipv4h_addr,
uint16_t ipv4_orport,
uint16_t ipv4_dirport,
@@ -895,11 +891,11 @@ fascist_firewall_choose_address_ipv4h(uint32_t ipv4h_addr,
tor_addr_make_null(&ap->addr, AF_UNSPEC);
ap->port = 0;
- return fascist_firewall_choose_address_base(&ipv4_addr, ipv4_orport,
- ipv4_dirport, ipv6_addr,
- ipv6_orport, ipv6_dirport,
- fw_connection, pref_only,
- pref_ipv6, ap);
+ fascist_firewall_choose_address_base(&ipv4_addr, ipv4_orport,
+ ipv4_dirport, ipv6_addr,
+ ipv6_orport, ipv6_dirport,
+ fw_connection, pref_only,
+ pref_ipv6, ap);
}
/* Some microdescriptor consensus methods have no IPv6 addresses in rs: they
@@ -950,26 +946,25 @@ node_awaiting_ipv6(const or_options_t* options, const node_t *node)
* This should only happen when there's no valid consensus, and rs doesn't
* correspond to a bridge client's bridge.
*/
-int
+void
fascist_firewall_choose_address_rs(const routerstatus_t *rs,
firewall_connection_t fw_connection,
int pref_only, tor_addr_port_t* ap)
{
- if (!rs) {
- return 0;
- }
-
tor_assert(ap);
tor_addr_make_null(&ap->addr, AF_UNSPEC);
ap->port = 0;
+ if (!rs) {
+ return;
+ }
+
const or_options_t *options = get_options();
const node_t *node = node_get_by_id(rs->identity_digest);
if (node && !node_awaiting_ipv6(options, node)) {
- return fascist_firewall_choose_address_node(node, fw_connection, pref_only,
- ap);
+ fascist_firewall_choose_address_node(node, fw_connection, pref_only, ap);
} else {
/* There's no node-specific IPv6 preference, so use the generic IPv6
* preference instead. */
@@ -979,37 +974,31 @@ fascist_firewall_choose_address_rs(const routerstatus_t *rs,
/* Assume IPv4 and IPv6 DirPorts are the same.
* Assume the IPv6 OR and Dir addresses are the same. */
- return fascist_firewall_choose_address_ipv4h(rs->addr,
- rs->or_port,
- rs->dir_port,
- &rs->ipv6_addr,
- rs->ipv6_orport,
- rs->dir_port,
- fw_connection,
- pref_only,
- pref_ipv6,
- ap);
+ fascist_firewall_choose_address_ipv4h(rs->addr, rs->or_port, rs->dir_port,
+ &rs->ipv6_addr, rs->ipv6_orport,
+ rs->dir_port, fw_connection,
+ pref_only, pref_ipv6, ap);
}
}
/** Like fascist_firewall_choose_address_base(), but takes <b>node</b>, and
* looks up the node's IPv6 preference rather than taking an argument
* for pref_ipv6. */
-int
+void
fascist_firewall_choose_address_node(const node_t *node,
firewall_connection_t fw_connection,
int pref_only, tor_addr_port_t *ap)
{
- if (!node) {
- return 0;
- }
-
- node_assert_ok(node);
tor_assert(ap);
tor_addr_make_null(&ap->addr, AF_UNSPEC);
ap->port = 0;
+ if (!node) {
+ return;
+ }
+
+ node_assert_ok(node);
/* Calling fascist_firewall_choose_address_node() when the node is missing
* IPv6 information breaks IPv6-only clients.
* If the node is a hard-coded fallback directory or authority, call
@@ -1019,7 +1008,7 @@ fascist_firewall_choose_address_node(const node_t *node,
* descriptor (routerinfo), or is one of our configured bridges before
* calling this function. */
if (BUG(node_awaiting_ipv6(get_options(), node))) {
- return 0;
+ return;
}
const int pref_ipv6_node = (fw_connection == FIREWALL_OR_CONNECTION
@@ -1037,41 +1026,36 @@ fascist_firewall_choose_address_node(const node_t *node,
node_get_pref_ipv6_dirport(node, &ipv6_dir_ap);
/* Assume the IPv6 OR and Dir addresses are the same. */
- return fascist_firewall_choose_address_base(&ipv4_or_ap.addr,
- ipv4_or_ap.port,
- ipv4_dir_ap.port,
- &ipv6_or_ap.addr,
- ipv6_or_ap.port,
- ipv6_dir_ap.port,
- fw_connection,
- pref_only,
- pref_ipv6_node,
- ap);
+ fascist_firewall_choose_address_base(&ipv4_or_ap.addr, ipv4_or_ap.port,
+ ipv4_dir_ap.port, &ipv6_or_ap.addr,
+ ipv6_or_ap.port, ipv6_dir_ap.port,
+ fw_connection, pref_only,
+ pref_ipv6_node, ap);
}
/** Like fascist_firewall_choose_address_rs(), but takes <b>ds</b>. */
-int
+void
fascist_firewall_choose_address_dir_server(const dir_server_t *ds,
firewall_connection_t fw_connection,
int pref_only,
tor_addr_port_t *ap)
{
- if (!ds) {
- return 0;
- }
-
tor_assert(ap);
tor_addr_make_null(&ap->addr, AF_UNSPEC);
ap->port = 0;
+ if (!ds) {
+ return;
+ }
+
/* A dir_server_t always has a fake_status. As long as it has the same
* addresses/ports in both fake_status and dir_server_t, this works fine.
* (See #17867.)
* This function relies on fascist_firewall_choose_address_rs looking up the
* node if it can, because that will get the latest info for the relay. */
- return fascist_firewall_choose_address_rs(&ds->fake_status, fw_connection,
- pref_only, ap);
+ fascist_firewall_choose_address_rs(&ds->fake_status, fw_connection,
+ pref_only, ap);
}
/** Return 1 if <b>addr</b> is permitted to connect to our dir port,
diff --git a/src/or/policies.h b/src/or/policies.h
index 35220a812..4879acdd8 100644
--- a/src/or/policies.h
+++ b/src/or/policies.h
@@ -55,13 +55,13 @@ int fascist_firewall_allows_dir_server(const dir_server_t *ds,
firewall_connection_t fw_connection,
int pref_only);
-int fascist_firewall_choose_address_rs(const routerstatus_t *rs,
- firewall_connection_t fw_connection,
- int pref_only, tor_addr_port_t* ap);
-int fascist_firewall_choose_address_node(const node_t *node,
- firewall_connection_t fw_connection,
- int pref_only, tor_addr_port_t* ap);
-int fascist_firewall_choose_address_dir_server(const dir_server_t *ds,
+void fascist_firewall_choose_address_rs(const routerstatus_t *rs,
+ firewall_connection_t fw_connection,
+ int pref_only, tor_addr_port_t* ap);
+void fascist_firewall_choose_address_node(const node_t *node,
+ firewall_connection_t fw_connection,
+ int pref_only, tor_addr_port_t* ap);
+void fascist_firewall_choose_address_dir_server(const dir_server_t *ds,
firewall_connection_t fw_connection,
int pref_only, tor_addr_port_t* ap);
diff --git a/src/test/test_policy.c b/src/test/test_policy.c
index f8aa8ac40..f18058586 100644
--- a/src/test/test_policy.c
+++ b/src/test/test_policy.c
@@ -1923,11 +1923,8 @@ test_policies_fascist_firewall_allows_address(void *arg)
tor_addr_port_t chosen_rs_ap; \
tor_addr_make_null(&chosen_rs_ap.addr, AF_INET); \
chosen_rs_ap.port = 0; \
- tt_int_op(fascist_firewall_choose_address_rs(&(fake_rs), \
- (fw_connection), \
- (pref_only), \
- &chosen_rs_ap), \
- OP_EQ, (expect_rv)); \
+ fascist_firewall_choose_address_rs(&(fake_rs), (fw_connection), \
+ (pref_only), &chosen_rs_ap); \
tt_assert(tor_addr_eq(&(expect_ap).addr, &chosen_rs_ap.addr)); \
tt_int_op((expect_ap).port, OP_EQ, chosen_rs_ap.port); \
STMT_END
@@ -1940,11 +1937,8 @@ test_policies_fascist_firewall_allows_address(void *arg)
tor_addr_port_t chosen_node_ap; \
tor_addr_make_null(&chosen_node_ap.addr, AF_INET); \
chosen_node_ap.port = 0; \
- tt_int_op(fascist_firewall_choose_address_node(&(fake_node), \
- (fw_connection), \
- (pref_only), \
- &chosen_node_ap), \
- OP_EQ, (expect_rv)); \
+ fascist_firewall_choose_address_node(&(fake_node),(fw_connection), \
+ (pref_only), &chosen_node_ap); \
tt_assert(tor_addr_eq(&(expect_ap).addr, &chosen_node_ap.addr)); \
tt_int_op((expect_ap).port, OP_EQ, chosen_node_ap.port); \
STMT_END
