From 870fd18b8fe880c6194415f6744b08a3989a0166 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 29 Dec 2008 01:47:28 +0000 Subject: [PATCH] Refactor some exit-policy-related functions that showed up in oprofile. Specifically, split compare_tor_addr_to_addr_policy() from a loop with a bunch of complicated ifs inside into some ifs, each with a simple loop. Rearrange router_find_exact_exit_enclave() to run a little faster. Bizarrely, router_policy_rejects_all() shows up on oprofile, so precalculate it per routerinfo. svn:r17802 --- ChangeLog | 4 + src/or/or.h | 8 +- src/or/policies.c | 193 +++++++++++++++++++++++++++---------------- src/or/routerlist.c | 12 +-- src/or/routerparse.c | 2 + 5 files changed, 138 insertions(+), 81 deletions(-) diff --git a/ChangeLog b/ChangeLog index be485d2074..3eef25c4f1 100644 --- a/ChangeLog +++ b/ChangeLog @@ -5,6 +5,10 @@ Changes in version 0.2.1.10-alpha - 2009-01-?? like Vidalia can show bridge operators that they're actually making a difference. + o Minor bugfixes (performance): + - Squeeze 2-5% out of client performance (according to oprofile) by + improving the implementation of some policy-manipulation functions. + o Minor bugfixes: - Make get_interface_address() function work properly again; stop guessing the wrong parts of our address as our address. diff --git a/src/or/or.h b/src/or/or.h index 2d539e6b73..139cb133d6 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -1418,6 +1418,8 @@ typedef struct { * a hidden service directory. */ unsigned int is_hs_dir:1; /**< True iff this router is a hidden service * directory according to the authorities. */ + unsigned int policy_is_reject_star:1; /**< True iff the exit policy for this + * router rejects everything. */ /** Tor can use this router for general positions in circuits. */ #define ROUTER_PURPOSE_GENERAL 0 @@ -3852,14 +3854,14 @@ int policies_parse_from_options(or_options_t *options); addr_policy_t *addr_policy_get_canonical_entry(addr_policy_t *ent); int cmp_addr_policies(smartlist_t *a, smartlist_t *b); addr_policy_result_t compare_tor_addr_to_addr_policy(const tor_addr_t *addr, - uint16_t port, smartlist_t *policy); + uint16_t port, const smartlist_t *policy); addr_policy_result_t compare_addr_to_addr_policy(uint32_t addr, - uint16_t port, smartlist_t *policy); + uint16_t port, const smartlist_t *policy); int policies_parse_exit_policy(config_line_t *cfg, smartlist_t **dest, int rejectprivate, const char *local_address); void policies_set_router_exitpolicy_to_reject_all(routerinfo_t *exitrouter); int exit_policy_is_general_exit(smartlist_t *policy); -int policy_is_reject_star(smartlist_t *policy); +int policy_is_reject_star(const smartlist_t *policy); int getinfo_helper_policies(control_connection_t *conn, const char *question, char **answer); int policy_write_item(char *buf, size_t buflen, addr_policy_t *item, diff --git a/src/or/policies.c b/src/or/policies.c index ece48b16e3..b44af88d6e 100644 --- a/src/or/policies.c +++ b/src/or/policies.c @@ -556,10 +556,11 @@ addr_policy_get_canonical_entry(addr_policy_t *e) return found->policy; } -/** As compare_to_addr_to_addr_policy, but instead of a tor_addr_t, takes +/** As compare_tor_addr_to_addr_policy, but instead of a tor_addr_t, takes * in host order. */ addr_policy_result_t -compare_addr_to_addr_policy(uint32_t addr, uint16_t port, smartlist_t *policy) +compare_addr_to_addr_policy(uint32_t addr, uint16_t port, + const smartlist_t *policy) { /*XXXX deprecate this function when possible. */ tor_addr_t a; @@ -567,87 +568,133 @@ compare_addr_to_addr_policy(uint32_t addr, uint16_t port, smartlist_t *policy) return compare_tor_addr_to_addr_policy(&a, port, policy); } +/** Helper for compare_tor_addr_to_addr_policy. Implements the case where + * addr and port are both known. */ +static addr_policy_result_t +compare_known_tor_addr_to_addr_policy(const tor_addr_t *addr, uint16_t port, + const smartlist_t *policy) +{ + /* We know the address and port, and we know the policy, so we can just + * compute an exact match. */ + SMARTLIST_FOREACH_BEGIN(policy, addr_policy_t *, tmpe) { + /* Address is known */ + if (!tor_addr_compare_masked(addr, &tmpe->addr, tmpe->maskbits, + CMP_SEMANTIC)) { + if (port >= tmpe->prt_min && port <= tmpe->prt_max) { + /* Exact match for the policy */ + return tmpe->policy_type == ADDR_POLICY_ACCEPT ? + ADDR_POLICY_ACCEPTED : ADDR_POLICY_REJECTED; + } + } + } SMARTLIST_FOREACH_END(tmpe); + + /* accept all by default. */ + return ADDR_POLICY_ACCEPTED; +} + +/** Helper for compare_tor_addr_to_addr_policy. Implements the case where + * addr is known but port is not. */ +static addr_policy_result_t +compare_known_tor_addr_to_addr_policy_noport(const tor_addr_t *addr, + const smartlist_t *policy) +{ + /* We look to see if there's a definite match. If so, we return that + match's value, unless there's an intervening possible match that says + something different. */ + int maybe_accept = 0, maybe_reject = 0; + + SMARTLIST_FOREACH_BEGIN(policy, addr_policy_t *, tmpe) { + if (!tor_addr_compare_masked(addr, &tmpe->addr, tmpe->maskbits, + CMP_SEMANTIC)) { + if (tmpe->prt_min <= 1 && tmpe->prt_max >= 65535) { + /* Definitely matches, since it covers all ports. */ + if (tmpe->policy_type == ADDR_POLICY_ACCEPT) { + /* If we already hit a clause that might trigger a 'reject', than we + * can't be sure of this certain 'accept'.*/ + return maybe_reject ? ADDR_POLICY_PROBABLY_ACCEPTED : + ADDR_POLICY_ACCEPTED; + } else { + return maybe_accept ? ADDR_POLICY_PROBABLY_REJECTED : + ADDR_POLICY_REJECTED; + } + } else { + /* Might match. */ + if (tmpe->policy_type == ADDR_POLICY_REJECT) + maybe_reject = 1; + else + maybe_accept = 1; + } + } + } SMARTLIST_FOREACH_END(tmpe); + + /* accept all by default. */ + return maybe_reject ? ADDR_POLICY_PROBABLY_ACCEPTED : ADDR_POLICY_ACCEPTED; +} + +/** Helper for compare_tor_addr_to_addr_policy. Implements the case where + * port is known but address is not. */ +static addr_policy_result_t +compare_unknown_tor_addr_to_addr_policy(uint16_t port, + const smartlist_t *policy) +{ + /* We look to see if there's a definite match. If so, we return that + match's value, unless there's an intervening possible match that says + something different. */ + int maybe_accept = 0, maybe_reject = 0; + + SMARTLIST_FOREACH_BEGIN(policy, addr_policy_t *, tmpe) { + if (tmpe->prt_min <= port && port <= tmpe->prt_max) { + if (tmpe->maskbits == 0) { + /* Definitely matches, since it covers all addresses. */ + if (tmpe->policy_type == ADDR_POLICY_ACCEPT) { + /* If we already hit a clause that might trigger a 'reject', than we + * can't be sure of this certain 'accept'.*/ + return maybe_reject ? ADDR_POLICY_PROBABLY_ACCEPTED : + ADDR_POLICY_ACCEPTED; + } else { + return maybe_accept ? ADDR_POLICY_PROBABLY_REJECTED : + ADDR_POLICY_REJECTED; + } + } else { + /* Might match. */ + if (tmpe->policy_type == ADDR_POLICY_REJECT) + maybe_reject = 1; + else + maybe_accept = 1; + } + } + } SMARTLIST_FOREACH_END(tmpe); + + /* accept all by default. */ + return maybe_reject ? ADDR_POLICY_PROBABLY_ACCEPTED : ADDR_POLICY_ACCEPTED; +} + /** Decide whether a given addr:port is definitely accepted, * definitely rejected, probably accepted, or probably rejected by a * given policy. If addr is 0, we don't know the IP of the - * target address. If port is 0, we don't know the port of the - * target address. - * - * For now, the algorithm is pretty simple: we look for definite and - * uncertain matches. The first definite match is what we guess; if - * it was preceded by no uncertain matches of the opposite policy, - * then the guess is definite; otherwise it is probable. (If we - * have a known addr and port, all matches are definite; if we have an - * unknown addr/port, any address/port ranges other than "all" are - * uncertain.) + * target address. If port is 0, we don't know the port of the + * target address. (At least one of addr and port must be + * provided. If you want to know whether a policy would definitely reject + * an unknown address:port, use policy_is_reject_star().) * * We could do better by assuming that some ranges never match typical * addresses (127.0.0.1, and so on). But we'll try this for now. */ addr_policy_result_t compare_tor_addr_to_addr_policy(const tor_addr_t *addr, uint16_t port, - smartlist_t *policy) + const smartlist_t *policy) { - int maybe_reject = 0; - int maybe_accept = 0; - int match = 0; - int maybe = 0; - int i, len; - int addr_is_unknown; - addr_is_unknown = tor_addr_is_null(addr); - - len = policy ? smartlist_len(policy) : 0; - - for (i = 0; i < len; ++i) { - addr_policy_t *tmpe = smartlist_get(policy, i); - maybe = 0; - if (addr_is_unknown) { - /* Address is unknown. */ - if ((port >= tmpe->prt_min && port <= tmpe->prt_max) || - (!port && tmpe->prt_min<=1 && tmpe->prt_max>=65535)) { - /* The port definitely matches. */ - if (tmpe->maskbits == 0) { - match = 1; - } else { - maybe = 1; - } - } else if (!port) { - /* The port maybe matches. */ - maybe = 1; - } - } else { - /* Address is known */ - if (!tor_addr_compare_masked(addr, &tmpe->addr, tmpe->maskbits, - CMP_SEMANTIC)) { - if (port >= tmpe->prt_min && port <= tmpe->prt_max) { - /* Exact match for the policy */ - match = 1; - } else if (!port) { - maybe = 1; - } - } - } - if (maybe) { - if (tmpe->policy_type == ADDR_POLICY_REJECT) - maybe_reject = 1; - else - maybe_accept = 1; - } - if (match) { - if (tmpe->policy_type == ADDR_POLICY_ACCEPT) { - /* If we already hit a clause that might trigger a 'reject', than we - * can't be sure of this certain 'accept'.*/ - return maybe_reject ? ADDR_POLICY_PROBABLY_ACCEPTED : - ADDR_POLICY_ACCEPTED; - } else { - return maybe_accept ? ADDR_POLICY_PROBABLY_REJECTED : - ADDR_POLICY_REJECTED; - } - } + if (!policy) { + /* no policy? accept all. */ + return ADDR_POLICY_ACCEPTED; + } else if (tor_addr_is_null(addr)) { + tor_assert(port != 0); + return compare_unknown_tor_addr_to_addr_policy(port, policy); + } else if (port == 0) { + return compare_known_tor_addr_to_addr_policy_noport(addr, policy); + } else { + return compare_known_tor_addr_to_addr_policy(addr, port, policy); } - - /* accept all by default. */ - return maybe_reject ? ADDR_POLICY_PROBABLY_ACCEPTED : ADDR_POLICY_ACCEPTED; } /** Return true iff the address policy a covers every case that @@ -854,7 +901,7 @@ exit_policy_is_general_exit(smartlist_t *policy) /** Return false if policy might permit access to some addr:port; * otherwise if we are certain it rejects everything, return true. */ int -policy_is_reject_star(smartlist_t *policy) +policy_is_reject_star(const smartlist_t *policy) { if (!policy) /*XXXX disallow NULL policies? */ return 1; diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 71baf6ea8d..c4de128666 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -1449,16 +1449,19 @@ router_find_exact_exit_enclave(const char *address, uint16_t port) { uint32_t addr; struct in_addr in; + tor_addr_t a; if (!tor_inet_aton(address, &in)) return NULL; /* it's not an IP already */ addr = ntohl(in.s_addr); + tor_addr_from_ipv4h(&a, addr); + SMARTLIST_FOREACH(routerlist->routers, routerinfo_t *, router, { - if (router->is_running && - router->addr == addr && - compare_addr_to_addr_policy(addr, port, router->exit_policy) == + if (router->addr == addr && + router->is_running && + compare_tor_addr_to_addr_policy(&a, port, router->exit_policy) == ADDR_POLICY_ACCEPTED) return router; }); @@ -3645,8 +3648,7 @@ router_exit_policy_all_routers_reject(uint32_t addr, uint16_t port, int router_exit_policy_rejects_all(routerinfo_t *router) { - return compare_addr_to_addr_policy(0, 0, router->exit_policy) - == ADDR_POLICY_REJECTED; + return router->policy_is_reject_star; } /** Add to the list of authorized directory servers one at diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 3497ffbeef..d401bf1f3b 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -1365,6 +1365,8 @@ router_parse_entry_from_string(const char *s, const char *end, goto err; }); policy_expand_private(&router->exit_policy); + if (policy_is_reject_star(router->exit_policy)) + router->policy_is_reject_star = 1; if ((tok = find_opt_by_keyword(tokens, K_FAMILY)) && tok->n_args) { int i;