commit 59f5c3d26337ba82be2a7cb2af02ba77e02c1b87 Author: David Goulet <dgoulet@torproject.org> Date: Wed Jun 24 10:35:27 2020 -0400 addr: Refactor find_my_address() to simplify it Instead of a complex if/else block, use a table of functions that have the same interface and each of them attempt to find the address one after the other. Pointed out by nickm's during review. Signed-off-by: David Goulet <dgoulet@torproject.org> --- src/app/config/resolve_addr.c | 177 +++++++++++++++++++++++------------------- 1 file changed, 99 insertions(+), 78 deletions(-) diff --git a/src/app/config/resolve_addr.c b/src/app/config/resolve_addr.c index dada4dabf..cd19402f9 100644 --- a/src/app/config/resolve_addr.c +++ b/src/app/config/resolve_addr.c @@ -22,19 +22,6 @@ /** Maximum "Address" statement allowed in our configuration. */ #define MAX_CONFIG_ADDRESS 2 -/** Errors use when finding our IP address. Some are transient and some are - * persistent, just attach semantic to the values. They are all negative so - * one can do a catch all. */ - -#define ERR_FAIL_RESOLVE -1 /* Hostname resolution failed. */ -#define ERR_GET_HOSTNAME -2 /* Unable to get local hostname. */ -#define ERR_NO_OPT_ADDRESS -3 /* No Address in config. */ -#define ERR_TOO_MANY_ADDRESS -4 /* Too many Address for one family. */ -#define ERR_GET_INTERFACE -5 /* Unable to query network interface. */ -#define ERR_UNUSABLE_ADDRESS -6 /* Unusable address. It is internal. */ -#define ERR_DEFAULT_DIRAUTH -7 /* Using default authorities. */ -#define ERR_ADDRESS_IS_INTERNAL -8 /* IP is internal. */ - /** Ease our life. Arrays containing state per address family. These are to * add semantic to the code so we know what is accessed. */ #define IDX_NULL 0 /* Index to zeroed address object. */ @@ -42,6 +29,18 @@ #define IDX_IPV6 2 /* Index to AF_INET6. */ #define IDX_SIZE 3 /* How many indexes do we have. */ +/** Function in our address function table return one of these code. */ +typedef enum { + /* The address has been found. */ + FN_RET_OK = 0, + /* The failure requirements were not met and thus it is recommended that the + * caller stops the search. */ + FN_RET_BAIL = 1, + /* The address was not found or failure is transient so the caller should go + * to the next method. */ + FN_RET_NEXT = 2, +} fn_address_ret_t; + /** Last resolved addresses. */ static tor_addr_t last_resolved_addrs[IDX_SIZE]; @@ -80,6 +79,11 @@ resolved_addr_reset_last(int family) tor_addr_make_null(&last_resolved_addrs[af_to_idx(family)], family); } +/** Errors returned by address_can_be_used() in order for the caller to know + * why the address is denied or not. */ +#define ERR_DEFAULT_DIRAUTH -1 /* Using default authorities. */ +#define ERR_ADDRESS_IS_INTERNAL -2 /* IP is internal. */ + /** @brief Return true iff the given IP address can be used as a valid * external resolved address. * @@ -151,11 +155,12 @@ address_can_be_used(const tor_addr_t *addr, const or_options_t *options, * @return Return 0 on success that is an address has been found or resolved * successfully. Return error code ERR_* found at the top of the file. */ -static int +static fn_address_ret_t get_address_from_config(const or_options_t *options, int warn_severity, int family, const char **method_out, char **hostname_out, tor_addr_t *addr_out) { + int ret; bool explicit_ip = false; int num_valid_addr = 0; @@ -172,7 +177,8 @@ get_address_from_config(const or_options_t *options, int warn_severity, if (!options->Address) { log_info(LD_CONFIG, "No Address option found in configuration."); - return ERR_NO_OPT_ADDRESS; + /* No Address statement, inform caller to try next method. */ + return FN_RET_NEXT; } for (const config_line_t *cfg = options->Address; cfg != NULL; @@ -199,11 +205,10 @@ get_address_from_config(const or_options_t *options, int warn_severity, num_valid_addr++; continue; } else { - /* If we have hostname we are unable to resolve, it is an persistent - * error and thus we stop right away. */ + /* Hostname that can't be resolved, this is a fatal error. */ log_fn(warn_severity, LD_CONFIG, "Could not resolve local Address '%s'. Failing.", cfg->value); - return ERR_FAIL_RESOLVE; + return FN_RET_BAIL; } } @@ -211,18 +216,32 @@ get_address_from_config(const or_options_t *options, int warn_severity, log_fn(warn_severity, LD_CONFIG, "No Address option found for family %s in configuration.", fmt_af_family(family)); - return ERR_NO_OPT_ADDRESS; + /* No Address statement for family, inform caller to try next method. */ + return FN_RET_NEXT; } if (num_valid_addr >= MAX_CONFIG_ADDRESS) { + /* Too many Address for same family. This is a fatal error. */ log_fn(warn_severity, LD_CONFIG, "Found %d Address statement of address family %s. " "Only one is allowed.", num_valid_addr, fmt_af_family(family)); - return ERR_TOO_MANY_ADDRESS; + return FN_RET_BAIL; } /* Great, we found an address. */ - return address_can_be_used(addr_out, options, warn_severity, explicit_ip); + ret = address_can_be_used(addr_out, options, warn_severity, explicit_ip); + if (ret != 0) { + /* One of the requirement of this interface is if an internal Address is + * used, custom authorities must be defined else it is a fatal error. + * Furthermore, if the Address was resolved to an internal interface, we + * stop immediately. */ + return FN_RET_BAIL; + } + + /* Address can be used. We are done. */ + log_fn(warn_severity, LD_CONFIG, "Address found in configuration: %s", + fmt_addr(addr_out)); + return FN_RET_OK; } /** @brief Get IP address from the local hostname by calling gethostbyname() @@ -240,7 +259,7 @@ get_address_from_config(const or_options_t *options, int warn_severity, * @return Return 0 on success that is an address has been found and resolved * successfully. Return error code ERR_* found at the top of the file. */ -static int +static fn_address_ret_t get_address_from_hostname(const or_options_t *options, int warn_severity, int family, const char **method_out, char **hostname_out, tor_addr_t *addr_out) @@ -259,24 +278,33 @@ get_address_from_hostname(const or_options_t *options, int warn_severity, if (tor_gethostname(hostname, sizeof(hostname)) < 0) { log_fn(warn_severity, LD_NET, "Error obtaining local hostname"); - return ERR_GET_HOSTNAME; + /* Unable to obtain the local hostname is a fatal error. */ + return FN_RET_BAIL; } if (tor_addr_lookup(hostname, family, addr_out)) { log_fn(warn_severity, LD_NET, "Could not resolve local hostname '%s'. Failing.", hostname); - return ERR_FAIL_RESOLVE; + /* Unable to resolve, inform caller to try next method. */ + return FN_RET_NEXT; } ret = address_can_be_used(addr_out, options, warn_severity, false); - if (ret < 0) { - return ret; + if (ret == ERR_DEFAULT_DIRAUTH) { + /* Non custom authorities, inform caller to try next method. */ + return FN_RET_NEXT; + } else if (ret == ERR_ADDRESS_IS_INTERNAL) { + /* Internal address is a fatal error. */ + return FN_RET_BAIL; } /* addr_out contains the address of the local hostname. */ *method_out = "GETHOSTNAME"; *hostname_out = tor_strdup(hostname); - return 0; + /* Found it! */ + log_fn(warn_severity, LD_CONFIG, "Address found from local hostname: %s", + fmt_addr(addr_out)); + return FN_RET_OK; } /** @brief Get IP address from a network interface. @@ -286,40 +314,49 @@ get_address_from_hostname(const or_options_t *options, int warn_severity, * @param family IP address family. Only AF_INET and AF_INET6 are supported. * @param method_out OUT: Always "INTERFACE" on success which is detailed in * the control-spec.txt as actions for "STATUS_SERVER". + * @param hostname_out OUT: String containing the local hostname. For this + * function, it is always set to NULL. * @param addr_out OUT: Tor address found attached to the interface. * * @return Return 0 on success that is an address has been found. Return * error code ERR_* found at the top of the file. */ -static int +static fn_address_ret_t get_address_from_interface(const or_options_t *options, int warn_severity, int family, const char **method_out, - tor_addr_t *addr_out) + char **hostname_out, tor_addr_t *addr_out) { int ret; tor_assert(method_out); + tor_assert(hostname_out); tor_assert(addr_out); /* Set them to NULL for safety reasons. */ *method_out = NULL; + *hostname_out = NULL; log_debug(LD_CONFIG, "Attempting to get address from network interface"); if (get_interface_address6(warn_severity, family, addr_out) < 0) { log_fn(warn_severity, LD_CONFIG, "Could not get local interface IP address."); - return ERR_GET_INTERFACE; + /* Unable to get IP from interface. Inform caller to try next method. */ + return FN_RET_NEXT; } ret = address_can_be_used(addr_out, options, warn_severity, false); if (ret < 0) { - return ret; + /* Unable to use address. Inform caller to try next method. */ + return FN_RET_NEXT; } *method_out = "INTERFACE"; - return 0; + /* Found it! */ + log_fn(warn_severity, LD_CONFIG, "Address found from interface: %s", + fmt_addr(addr_out)); + return FN_RET_OK; } /** @brief Update the last resolved address cache using the given address. @@ -393,6 +430,21 @@ update_resolved_cache(const tor_addr_t *addr, const char *method_used, *done_one_resolve = true; } +/** Address discovery function table. The order matters as in the first one is + * executed first and so on. */ +static fn_address_ret_t + (*fn_address_table[])( + const or_options_t *options, int warn_severity, int family, + const char **method_out, char **hostname_out, tor_addr_t *addr_out) = +{ + /* These functions are in order for our find address algorithm. */ + get_address_from_config, + get_address_from_hostname, + get_address_from_interface, +}; +/** Length of address table as in how many functions. */ +static const size_t fn_address_table_len = ARRAY_LENGTH(fn_address_table); + /** @brief Attempt to find our IP address that can be used as our external * reachable address. * @@ -465,8 +517,7 @@ find_my_address(const or_options_t *options, int family, int warn_severity, tor_addr_t *addr_out, const char **method_out, char **hostname_out) { - int ret; - const char *method_used; + const char *method_used = NULL; char *hostname_used = NULL; tor_addr_t my_addr; @@ -481,51 +532,24 @@ find_my_address(const or_options_t *options, int family, int warn_severity, * Step 1: Discover address by attempting 3 different methods consecutively. */ - /* Attempt #1: Get address from configuration. */ - ret = get_address_from_config(options, warn_severity, family, &method_used, - &hostname_used, &my_addr); - if (ret == 0) { - log_fn(warn_severity, LD_CONFIG, "Address found in configuration: %s", - fmt_addr(&my_addr)); - } else { - /* Unable to resolve an Address statement is a failure. Also, using - * default dirauth error means that the configured address is internal - * which is only accepted if custom authorities are used. */ - if (ret == ERR_FAIL_RESOLVE || ret == ERR_DEFAULT_DIRAUTH) { + /* Go over the function table. They are in order. */ + for (size_t idx = 0; idx < fn_address_table_len; idx++) { + fn_address_ret_t ret = fn_address_table[idx](options, warn_severity, + family, &method_used, + &hostname_used, &my_addr); + if (ret == FN_RET_BAIL) { return false; + } else if (ret == FN_RET_OK) { + goto found; } - - /* Attempt #2: Get local hostname and resolve it. */ - ret = get_address_from_hostname(options, warn_severity, family, - &method_used, &hostname_used, &my_addr); - if (ret == 0) { - log_fn(warn_severity, LD_CONFIG, "Address found from local hostname: " - "%s", fmt_addr(&my_addr)); - } else if (ret < 0) { - /* Unable to get the hostname results in a failure. If the address is - * internal, we stop right away. */ - if (ret == ERR_GET_HOSTNAME || ret == ERR_ADDRESS_IS_INTERNAL) { - return false; - } - - /* Attempt #3: Get address from interface. */ - ret = get_address_from_interface(options, warn_severity, family, - &method_used, &my_addr); - if (ret == 0) { - log_fn(warn_severity, LD_CONFIG, "Address found from interface: %s", - fmt_addr(&my_addr)); - } else { - /* We've exhausted our attempts. Failure. */ - log_fn(warn_severity, LD_CONFIG, "Unable to find our IP address."); - return false; - } - } + tor_assert(ret == FN_RET_NEXT); } - tor_assert(method_used); - /* From here, my_addr is a valid IP address of "family" and can be used as - * our external IP address. */ + /* We've exhausted our attempts. Failure. */ + log_fn(warn_severity, LD_CONFIG, "Unable to find our IP address."); + return false; + found: /* * Step 2: Update last resolved address cache and inform the control port. */ @@ -535,10 +559,7 @@ find_my_address(const or_options_t *options, int family, int warn_severity, *method_out = method_used; } if (hostname_out) { - *hostname_out = NULL; - if (hostname_used) { - *hostname_out = hostname_used; - } + *hostname_out = hostname_used; } else { tor_free(hostname_used); }