From ba5053b45dd14bd2fb982a29360fe0e1dc4525dc Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Sun, 6 Dec 2015 21:28:21 +1100 Subject: [PATCH 1/4] Refactor policies_parse_exit_policy_internal Move logging of redundant policy entries in policies_parse_exit_policy_internal into its own function. Closes ticket 17608; patch from "juce". --- changes/feature17608 | 4 ++ src/or/policies.c | 103 +++++++++++++++++++++++-------------------- 2 files changed, 60 insertions(+), 47 deletions(-) create mode 100644 changes/feature17608 diff --git a/changes/feature17608 b/changes/feature17608 new file mode 100644 index 0000000000..d56bb7d4a7 --- /dev/null +++ b/changes/feature17608 @@ -0,0 +1,4 @@ + o Minor feature (refactoring): + - Move logging of redundant policy entries in + policies_parse_exit_policy_internal into its own function. + Closes ticket 17608; patch from "juce". diff --git a/src/or/policies.c b/src/or/policies.c index 126ba465df..275bab2708 100644 --- a/src/or/policies.c +++ b/src/or/policies.c @@ -1124,54 +1124,12 @@ policies_parse_exit_policy_reject_private( "reject *:563,reject *:1214,reject *:4661-4666," \ "reject *:6346-6429,reject *:6699,reject *:6881-6999,accept *:*" -/** Parse the exit policy cfg into the linked list *dest. - * - * If ipv6_exit is false, prepend "reject *6:*" to the policy. - * - * If rejectprivate is true: - * - prepend "reject private:*" to the policy. - * - prepend entries that reject publicly routable addresses on this exit - * relay by calling policies_parse_exit_policy_reject_private - * - * If cfg doesn't end in an absolute accept or reject and if - * add_default_policy is true, add the default exit - * policy afterwards. - * - * Return -1 if we can't parse cfg, else return 0. - * - * This function is used to parse the exit policy from our torrc. For - * the functions used to parse the exit policy from a router descriptor, - * see router_add_exit_policy. +/** + * Iterates through *dest and logs a warning with first + * redundant entry if found */ -static int -policies_parse_exit_policy_internal(config_line_t *cfg, - smartlist_t **dest, - int ipv6_exit, - int rejectprivate, - const smartlist_t *configured_addresses, - int reject_interface_addresses, - int reject_configured_port_addresses, - int add_default_policy) -{ - if (!ipv6_exit) { - append_exit_policy_string(dest, "reject *6:*"); - } - if (rejectprivate) { - /* Reject IPv4 and IPv6 reserved private netblocks */ - append_exit_policy_string(dest, "reject private:*"); - /* Reject IPv4 and IPv6 publicly routable addresses on this exit relay */ - policies_parse_exit_policy_reject_private( - dest, ipv6_exit, - configured_addresses, - reject_interface_addresses, - reject_configured_port_addresses); - } - if (parse_addr_policy(cfg, dest, -1)) - return -1; - - /* Before we add the default policy and final rejects, check to see if - * there are any lines after accept *:* or reject *:*. These lines have no - * effect, and are most likely an error. */ +static void +policies_log_first_redundant_entry(smartlist_t** dest) { int found_final_effective_entry = 0; int first_redundant_entry = 0; for (int i = 0; i < smartlist_len(*dest); ++i) { @@ -1227,6 +1185,57 @@ policies_parse_exit_policy_internal(config_line_t *cfg, "accept/reject *:* as the last entry in any exit policy.)", line); } +} + +/** Parse the exit policy cfg into the linked list *dest. + * + * If ipv6_exit is false, prepend "reject *6:*" to the policy. + * + * If rejectprivate is true: + * - prepend "reject private:*" to the policy. + * - prepend entries that reject publicly routable addresses on this exit + * relay by calling policies_parse_exit_policy_reject_private + * + * If cfg doesn't end in an absolute accept or reject and if + * add_default_policy is true, add the default exit + * policy afterwards. + * + * Return -1 if we can't parse cfg, else return 0. + * + * This function is used to parse the exit policy from our torrc. For + * the functions used to parse the exit policy from a router descriptor, + * see router_add_exit_policy. + */ +static int +policies_parse_exit_policy_internal(config_line_t *cfg, + smartlist_t **dest, + int ipv6_exit, + int rejectprivate, + const smartlist_t *configured_addresses, + int reject_interface_addresses, + int reject_configured_port_addresses, + int add_default_policy) +{ + if (!ipv6_exit) { + append_exit_policy_string(dest, "reject *6:*"); + } + if (rejectprivate) { + /* Reject IPv4 and IPv6 reserved private netblocks */ + append_exit_policy_string(dest, "reject private:*"); + /* Reject IPv4 and IPv6 publicly routable addresses on this exit relay */ + policies_parse_exit_policy_reject_private( + dest, ipv6_exit, + configured_addresses, + reject_interface_addresses, + reject_configured_port_addresses); + } + if (parse_addr_policy(cfg, dest, -1)) + return -1; + + /* Before we add the default policy and final rejects, check to see if + * there are any lines after accept *:* or reject *:*. These lines have no + * effect, and are most likely an error. */ + policies_log_first_redundant_entry(dest); if (add_default_policy) { append_exit_policy_string(dest, DEFAULT_EXIT_POLICY); From bca4095b93615d5f77554b440b7935daff6773de Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Sun, 6 Dec 2015 21:30:52 +1100 Subject: [PATCH 2/4] Make policies_log_first_redundant_entry take a const smartlist_t * Also fixup code style. --- src/or/policies.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/or/policies.c b/src/or/policies.c index 275bab2708..692eaa1628 100644 --- a/src/or/policies.c +++ b/src/or/policies.c @@ -1119,25 +1119,22 @@ policies_parse_exit_policy_reject_private( } } -#define DEFAULT_EXIT_POLICY \ - "reject *:25,reject *:119,reject *:135-139,reject *:445," \ - "reject *:563,reject *:1214,reject *:4661-4666," \ - "reject *:6346-6429,reject *:6699,reject *:6881-6999,accept *:*" - /** - * Iterates through *dest and logs a warning with first + * Iterates through dest and logs a warning with first * redundant entry if found */ -static void -policies_log_first_redundant_entry(smartlist_t** dest) { +static void +policies_log_first_redundant_entry(const smartlist_t* dest) +{ int found_final_effective_entry = 0; int first_redundant_entry = 0; - for (int i = 0; i < smartlist_len(*dest); ++i) { + tor_assert(dest); + for (int i = 0; i < smartlist_len(dest); ++i) { sa_family_t family; addr_policy_t *p; int found_ipv4_wildcard = 0, found_ipv6_wildcard = 0; - p = smartlist_get(*dest, i); + p = smartlist_get(dest, i); /* Look for accept/reject *[4|6|]:* entires */ if (p->prt_min <= 1 && p->prt_max == 65535 && p->maskbits == 0) { @@ -1160,7 +1157,7 @@ policies_log_first_redundant_entry(smartlist_t** dest) { if (found_ipv4_wildcard && found_ipv6_wildcard) { found_final_effective_entry = 1; /* if we're not on the final entry in the list */ - if (i < smartlist_len(*dest) - 1) { + if (i < smartlist_len(dest) - 1) { first_redundant_entry = i + 1; } break; @@ -1174,8 +1171,8 @@ policies_log_first_redundant_entry(smartlist_t** dest) { * which contains a max-length IPv6 address, plus 24 characters. */ char line[TOR_ADDR_BUF_LEN + 32]; - tor_assert(first_redundant_entry < smartlist_len(*dest)); - p = smartlist_get(*dest, first_redundant_entry); + tor_assert(first_redundant_entry < smartlist_len(dest)); + p = smartlist_get(dest, first_redundant_entry); /* since we've already parsed the policy into an addr_policy_t struct, * we might not log exactly what the user typed in */ policy_write_item(line, TOR_ADDR_BUF_LEN + 32, p, 0); @@ -1187,6 +1184,11 @@ policies_log_first_redundant_entry(smartlist_t** dest) { } } +#define DEFAULT_EXIT_POLICY \ + "reject *:25,reject *:119,reject *:135-139,reject *:445," \ + "reject *:563,reject *:1214,reject *:4661-4666," \ + "reject *:6346-6429,reject *:6699,reject *:6881-6999,accept *:*" + /** Parse the exit policy cfg into the linked list *dest. * * If ipv6_exit is false, prepend "reject *6:*" to the policy. @@ -1235,7 +1237,7 @@ policies_parse_exit_policy_internal(config_line_t *cfg, /* Before we add the default policy and final rejects, check to see if * there are any lines after accept *:* or reject *:*. These lines have no * effect, and are most likely an error. */ - policies_log_first_redundant_entry(dest); + policies_log_first_redundant_entry(*dest); if (add_default_policy) { append_exit_policy_string(dest, DEFAULT_EXIT_POLICY); From db433b8dc3c8684e6a86365e12336a708f67edaa Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 9 Dec 2015 11:02:10 -0500 Subject: [PATCH 3/4] Tweak policies_log_first_redundant_entry more. * Since the variable is no longer modified, it should be called 'policy' instead of 'dest'. ("Dest" is short for "destination".) * Fixed the space issue that dgoulet found on the ticket. * Fixed the comment a little. (We use the imperative for function documentation.) --- src/or/policies.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/or/policies.c b/src/or/policies.c index 692eaa1628..ec29b23c3e 100644 --- a/src/or/policies.c +++ b/src/or/policies.c @@ -1120,21 +1120,21 @@ policies_parse_exit_policy_reject_private( } /** - * Iterates through dest and logs a warning with first - * redundant entry if found + * Iterate through policy looking for redundant entries. Log a + * warning message with the first redundant entry, if any is found. */ static void -policies_log_first_redundant_entry(const smartlist_t* dest) +policies_log_first_redundant_entry(const smartlist_t *policy) { int found_final_effective_entry = 0; int first_redundant_entry = 0; - tor_assert(dest); - for (int i = 0; i < smartlist_len(dest); ++i) { + tor_assert(policy); + for (int i = 0; i < smartlist_len(policy); ++i) { sa_family_t family; addr_policy_t *p; int found_ipv4_wildcard = 0, found_ipv6_wildcard = 0; - p = smartlist_get(dest, i); + p = smartlist_get(policy, i); /* Look for accept/reject *[4|6|]:* entires */ if (p->prt_min <= 1 && p->prt_max == 65535 && p->maskbits == 0) { @@ -1157,7 +1157,7 @@ policies_log_first_redundant_entry(const smartlist_t* dest) if (found_ipv4_wildcard && found_ipv6_wildcard) { found_final_effective_entry = 1; /* if we're not on the final entry in the list */ - if (i < smartlist_len(dest) - 1) { + if (i < smartlist_len(policy) - 1) { first_redundant_entry = i + 1; } break; @@ -1171,8 +1171,8 @@ policies_log_first_redundant_entry(const smartlist_t* dest) * which contains a max-length IPv6 address, plus 24 characters. */ char line[TOR_ADDR_BUF_LEN + 32]; - tor_assert(first_redundant_entry < smartlist_len(dest)); - p = smartlist_get(dest, first_redundant_entry); + tor_assert(first_redundant_entry < smartlist_len(policy)); + p = smartlist_get(policy, first_redundant_entry); /* since we've already parsed the policy into an addr_policy_t struct, * we might not log exactly what the user typed in */ policy_write_item(line, TOR_ADDR_BUF_LEN + 32, p, 0); @@ -1237,7 +1237,7 @@ policies_parse_exit_policy_internal(config_line_t *cfg, /* Before we add the default policy and final rejects, check to see if * there are any lines after accept *:* or reject *:*. These lines have no * effect, and are most likely an error. */ - policies_log_first_redundant_entry(*dest); + policies_log_first_redundant_entry(*dest); if (add_default_policy) { append_exit_policy_string(dest, DEFAULT_EXIT_POLICY); From 580d788b3f85ee04f8893325b902bf28727a451b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 9 Dec 2015 11:04:56 -0500 Subject: [PATCH 4/4] Tweak policies_log_first_redundant_entry even more * Use smartlist_foreach_begin/end instead of a plain for loop. * constify the pointers. --- src/or/policies.c | 13 ++++++------- src/or/policies.h | 2 +- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/or/policies.c b/src/or/policies.c index ec29b23c3e..07f8cd7c40 100644 --- a/src/or/policies.c +++ b/src/or/policies.c @@ -1129,12 +1129,10 @@ policies_log_first_redundant_entry(const smartlist_t *policy) int found_final_effective_entry = 0; int first_redundant_entry = 0; tor_assert(policy); - for (int i = 0; i < smartlist_len(policy); ++i) { + SMARTLIST_FOREACH_BEGIN(policy, const addr_policy_t *, p) { sa_family_t family; - addr_policy_t *p; int found_ipv4_wildcard = 0, found_ipv6_wildcard = 0; - - p = smartlist_get(policy, i); + const int i = p_sl_idx; /* Look for accept/reject *[4|6|]:* entires */ if (p->prt_min <= 1 && p->prt_max == 65535 && p->maskbits == 0) { @@ -1162,10 +1160,11 @@ policies_log_first_redundant_entry(const smartlist_t *policy) } break; } - } + } SMARTLIST_FOREACH_END(p); + /* Work out if there are redundant trailing entries in the policy list */ if (found_final_effective_entry && first_redundant_entry > 0) { - addr_policy_t *p; + const addr_policy_t *p; /* Longest possible policy is * "accept6 ffff:ffff:..255/128:10000-65535", * which contains a max-length IPv6 address, plus 24 characters. */ @@ -1504,7 +1503,7 @@ policy_is_reject_star(const smartlist_t *policy, sa_family_t family) /** Write a single address policy to the buf_len byte buffer at buf. Return * the number of characters written, or -1 on failure. */ int -policy_write_item(char *buf, size_t buflen, addr_policy_t *policy, +policy_write_item(char *buf, size_t buflen, const addr_policy_t *policy, int format_for_desc) { size_t written = 0; diff --git a/src/or/policies.h b/src/or/policies.h index bb56bf42b8..007f494482 100644 --- a/src/or/policies.h +++ b/src/or/policies.h @@ -75,7 +75,7 @@ char * policy_dump_to_string(const smartlist_t *policy_list, int getinfo_helper_policies(control_connection_t *conn, const char *question, char **answer, const char **errmsg); -int policy_write_item(char *buf, size_t buflen, addr_policy_t *item, +int policy_write_item(char *buf, size_t buflen, const addr_policy_t *item, int format_for_desc); void addr_policy_list_free(smartlist_t *p);