commit 266f8cddd87f8cf507e094725b3f6028bb8d803b Author: George Kadianakis desnacked@riseup.net Date: Mon Feb 11 13:43:20 2013 +0000
Refactoring to make parse_bridge_line() unittestable.
- Make parse_bridge_line() return a struct. - Make bridge_add_from_config() accept a struct. - Make string_is_key_value() less hysterical. --- src/common/util.c | 13 +++-- src/common/util.h | 2 +- src/or/config.c | 133 ++++++++++++++++++++++++-------------------------- src/or/config.h | 14 +++++ src/or/entrynodes.c | 57 +++++++++++++-------- src/or/entrynodes.h | 7 +-- src/test/test_util.c | 18 +++--- 7 files changed, 133 insertions(+), 111 deletions(-)
diff --git a/src/common/util.c b/src/common/util.c index 9aba7d6..bcb69f2 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -866,9 +866,10 @@ tor_digest_is_zero(const char *digest) }
/** Return true if <b>string</b> is a valid '<key>=[<value>]' string. - * <value> is optional, to indicate the empty string. */ + * <value> is optional, to indicate the empty string. Log at logging + * <b>severity</b> if something ugly happens. */ int -string_is_key_value(const char *string) +string_is_key_value(int severity, const char *string) { /* position of equal sign in string */ const char *equal_sign_pos = NULL; @@ -876,19 +877,21 @@ string_is_key_value(const char *string) tor_assert(string);
if (strlen(string) < 2) { /* "x=" is shortest args string */ - log_warn(LD_GENERAL, "'%s' is too short to be a k=v value.", escaped(string)); + tor_log(severity, LD_GENERAL, "'%s' is too short to be a k=v value.", + escaped(string)); return 0; }
equal_sign_pos = strchr(string, '='); if (!equal_sign_pos) { - log_warn(LD_GENERAL, "'%s' is not a k=v value.", escaped(string)); + tor_log(severity, LD_GENERAL, "'%s' is not a k=v value.", escaped(string)); return 0; }
/* validate that the '=' is not in the beginning of the string. */ if (equal_sign_pos == string) { - log_warn(LD_GENERAL, "'%s' is not a valid k=v value.", escaped(string)); + tor_log(severity, LD_GENERAL, "'%s' is not a valid k=v value.", + escaped(string)); return 0; }
diff --git a/src/common/util.h b/src/common/util.h index e3cd721..624202c 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -209,7 +209,7 @@ const char *find_whitespace_eos(const char *s, const char *eos); const char *find_str_at_start_of_line(const char *haystack, const char *needle); int string_is_C_identifier(const char *string); -int string_is_key_value(const char *string); +int string_is_key_value(int severity, const char *string);
int tor_mem_is_zero(const char *mem, size_t len); int tor_digest_is_zero(const char *digest); diff --git a/src/or/config.c b/src/or/config.c index a09dda9..9d0d564 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -481,7 +481,6 @@ static int options_transition_affects_descriptor( const or_options_t *old_options, const or_options_t *new_options); static int check_nickname_list(const char *lst, const char *name, char **msg);
-static int parse_bridge_line(const char *line, int validate_only); static int parse_client_transport_line(const char *line, int validate_only);
static int parse_server_transport_line(const char *line, int validate_only); @@ -1293,11 +1292,13 @@ options_act(const or_options_t *old_options) if (options->Bridges) { mark_bridge_list(); for (cl = options->Bridges; cl; cl = cl->next) { - if (parse_bridge_line(cl->value, 0)<0) { + bridge_line_t *bridge_line = parse_bridge_line(cl->value); + if (!bridge_line) { log_warn(LD_BUG, "Previously validated Bridge line could not be added!"); return -1; } + bridge_add_from_config(bridge_line); } sweep_bridge_list(); } @@ -2966,8 +2967,10 @@ options_validate(or_options_t *old_options, or_options_t *options, REJECT("If you set UseBridges, you must set TunnelDirConns.");
for (cl = options->Bridges; cl; cl = cl->next) { - if (parse_bridge_line(cl->value, 1)<0) - REJECT("Bridge line did not parse. See logs for details."); + bridge_line_t *bridge_line = parse_bridge_line(cl->value); + if (!bridge_line) + REJECT("Bridge line did not parse. See logs for details."); + bridge_line_free(bridge_line); }
for (cl = options->ClientTransportPlugin; cl; cl = cl->next) { @@ -4038,8 +4041,10 @@ validate_transport_socks_arguments(const smartlist_t *args) tor_assert(smartlist_len(args) > 0);
SMARTLIST_FOREACH_BEGIN(args, const char *, s) { - if (!string_is_key_value(s)) /* arguments should be k=v items */ + if (!string_is_key_value(LOG_WARN, s)) { /* items should be k=v items */ + log_warn(LD_CONFIG, "'%s' is not a k=v item.", s); return -1; + } } SMARTLIST_FOREACH_END(s);
socks_string = pt_stringify_socks_args(args); @@ -4059,22 +4064,36 @@ validate_transport_socks_arguments(const smartlist_t *args) return 0; }
+/** Deallocate a bridge_line_t structure. */ +/* private */ void +bridge_line_free(bridge_line_t *bridge_line) +{ + if (!bridge_line) + return; + + if (bridge_line->socks_args) { + SMARTLIST_FOREACH(bridge_line->socks_args, char*, s, tor_free(s)); + smartlist_free(bridge_line->socks_args); + } + tor_free(bridge_line->transport_name); + tor_free(bridge_line); +} + /** Read the contents of a Bridge line from <b>line</b>. Return 0 * if the line is well-formed, and -1 if it isn't. If * <b>validate_only</b> is 0, and the line is well-formed, then add - * the bridge described in the line to our internal bridge list. */ -static int -parse_bridge_line(const char *line, int validate_only) + * the bridge described in the line to our internal bridge list. + * + * Bridge line format: + * Bridge [transport] IP:PORT [id-fingerprint] [k=v] [k=v] ... + */ +/* private */ bridge_line_t * +parse_bridge_line(const char *line) { smartlist_t *items = NULL; - int r; char *addrport=NULL, *fingerprint=NULL; - char *transport_name=NULL; char *field=NULL; - tor_addr_t addr; - uint16_t port = 0; - char digest[DIGEST_LEN]; - smartlist_t *socks_args = NULL; + bridge_line_t *bridge_line = tor_malloc_zero(sizeof(bridge_line_t));
items = smartlist_new(); smartlist_split_string(items, line, NULL, @@ -4084,47 +4103,49 @@ parse_bridge_line(const char *line, int validate_only) goto err; }
- /* field is either a transport name or addrport */ + /* first field is either a transport name or addrport */ field = smartlist_get(items, 0); smartlist_del_keeporder(items, 0);
- if (!(strstr(field, ".") || strstr(field, ":"))) { - /* new-style bridge line */ - transport_name = field; + if (string_is_C_identifier(field)) { + /* It's a transport name. */ + bridge_line->transport_name = field; if (smartlist_len(items) < 1) { log_warn(LD_CONFIG, "Too few items to Bridge line."); goto err; } - addrport = smartlist_get(items, 0); + addrport = smartlist_get(items, 0); /* Next field is addrport then. */ smartlist_del_keeporder(items, 0); } else { addrport = field; }
- if (tor_addr_port_lookup(addrport, &addr, &port)<0) { + /* Parse addrport. */ + if (tor_addr_port_lookup(addrport, + &bridge_line->addr, &bridge_line->port)<0) { log_warn(LD_CONFIG, "Error parsing Bridge address '%s'", addrport); goto err; } - if (!port) { + if (!bridge_line->port) { log_info(LD_CONFIG, "Bridge address '%s' has no port; using default port 443.", addrport); - port = 443; + bridge_line->port = 443; }
/* If transports are enabled, next field could be a fingerprint or a - socks argument. If transports are disabled, next field should be + socks argument. If transports are disabled, next field must be a fingerprint. */ if (smartlist_len(items)) { - if (transport_name) { /* transports enabled: */ + if (bridge_line->transport_name) { /* transports enabled: */ field = smartlist_get(items, 0); smartlist_del_keeporder(items, 0);
/* If it's a key=value pair, then it's a SOCKS argument for the transport proxy... */ - if (string_is_key_value(field)) { - socks_args = smartlist_new(); - smartlist_add(socks_args, field); + if (string_is_key_value(LOG_DEBUG, field)) { + bridge_line->socks_args = smartlist_new(); + smartlist_add(bridge_line->socks_args, field); } else { /* ...otherwise, it's the bridge fingerprint. */ fingerprint = field; } @@ -4134,78 +4155,50 @@ parse_bridge_line(const char *line, int validate_only) } }
+ /* Handle fingerprint, if it was provided. */ if (fingerprint) { if (strlen(fingerprint) != HEX_DIGEST_LEN) { log_warn(LD_CONFIG, "Key digest for Bridge is wrong length."); goto err; } - if (base16_decode(digest, DIGEST_LEN, fingerprint, HEX_DIGEST_LEN)<0) { + if (base16_decode(bridge_line->digest, DIGEST_LEN, + fingerprint, HEX_DIGEST_LEN)<0) { log_warn(LD_CONFIG, "Unable to decode Bridge key digest."); goto err; } }
/* If we are using transports, any remaining items in the smartlist - must be k=v values. */ - if (transport_name && smartlist_len(items)) { - if (!socks_args) - socks_args = smartlist_new(); + should be k=v values. */ + if (bridge_line->transport_name && smartlist_len(items)) { + if (!bridge_line->socks_args) + bridge_line->socks_args = smartlist_new();
/* append remaining items of 'items' to 'socks_args' */ - smartlist_add_all(socks_args, items); + smartlist_add_all(bridge_line->socks_args, items); smartlist_clear(items);
- tor_assert(smartlist_len(socks_args) > 0); + tor_assert(smartlist_len(bridge_line->socks_args) > 0); }
- if (!validate_only) { - log_debug(LD_DIR, "Bridge at %s (transport: %s) (%s)", - fmt_addrport(&addr, port), - transport_name ? transport_name : "no transport", - fingerprint ? fingerprint : "no key listed"); - - if (socks_args) { /* print socks arguments */ - int i = 0; - - tor_assert(smartlist_len(socks_args) > 0); - - log_debug(LD_DIR, "Bridge uses %d SOCKS arguments:", - smartlist_len(socks_args)); - SMARTLIST_FOREACH(socks_args, const char *, arg, - log_debug(LD_CONFIG, "%d: %s", ++i, arg)); - } - - bridge_add_from_config(&addr, port, - fingerprint ? digest : NULL, - transport_name, socks_args); - } else { - if (socks_args) { - if (validate_transport_socks_arguments(socks_args) < 0) - goto err; - } + if (bridge_line->socks_args) { + if (validate_transport_socks_arguments(bridge_line->socks_args) < 0) + goto err; }
- r = 0; goto done;
err: - r = -1; + bridge_line_free(bridge_line); + bridge_line = NULL;
done: SMARTLIST_FOREACH(items, char*, s, tor_free(s)); smartlist_free(items); tor_free(addrport); - tor_free(transport_name); tor_free(fingerprint);
- /* We only have to free socks_args if we are validating, since - otherwise bridge_add_from_config() steals its reference. */ - if (socks_args && validate_only) { - SMARTLIST_FOREACH(socks_args, char*, s, tor_free(s)); - smartlist_free(socks_args); - } - - return r; + return bridge_line; }
/** Read the contents of a ClientTransportPlugin line from diff --git a/src/or/config.h b/src/or/config.h index 8e34655..b5c0c73 100644 --- a/src/or/config.h +++ b/src/or/config.h @@ -96,5 +96,19 @@ int addressmap_register_auto(const char *from, const char *to, addressmap_entry_source_t addrmap_source, const char **msg);
+/** Represents the information stored in a torrc Bridge line. */ +typedef struct bridge_line_t { + tor_addr_t addr; /* The IP address of the bridge. */ + uint16_t port; /* The TCP port of the bridge. */ + char *transport_name; /* The name of the pluggable transport that + should be used to connect to the bridge. */ + char digest[DIGEST_LEN]; /* The bridge's identity key digest. */ + smartlist_t *socks_args;; /* SOCKS arguments for the pluggable + transport proxy. */ +} bridge_line_t; + +void bridge_line_free(bridge_line_t *bridge_line); +bridge_line_t *parse_bridge_line(const char *line); + #endif
diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index a07670b..44041d3 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1633,37 +1633,52 @@ bridge_resolve_conflicts(const tor_addr_t *addr, uint16_t port, } SMARTLIST_FOREACH_END(bridge); }
-/** Remember a new bridge at <b>addr</b>:<b>port</b>. If <b>digest</b> - * is set, it tells us the identity key too. If we already had the - * bridge in our list, unmark it, and don't actually add anything new. - * If <b>transport_name</b> is non-NULL - the bridge is associated with a - * pluggable transport - we assign the transport to the bridge. - * If <b>transport_name</b> is non-NULL - the bridge is associated - * with a pluggable transport - we assign the transport to the bridge. - * If <b>socks_args</b> is non-NULL, it's a smartlist carrying - * key=value pairs to be passed to the pluggable transports - * proxy. This function steals reference of the smartlist. */ +/** Register the bridge information in <b>bridge_line</b> to the + * bridge subsystem. Steals reference of <b>bridge_line</b>. */ void -bridge_add_from_config(const tor_addr_t *addr, uint16_t port, - const char *digest, const char *transport_name, - smartlist_t *socks_args) +bridge_add_from_config(bridge_line_t *bridge_line) { bridge_info_t *b;
- bridge_resolve_conflicts(addr, port, digest, transport_name); + { /* Log the bridge we are about to register: */ + log_debug(LD_GENERAL, "Registering bridge at %s (transport: %s) (%s)", + fmt_addrport(&bridge_line->addr, bridge_line->port), + bridge_line->transport_name ? + bridge_line->transport_name : "no transport", + tor_digest_is_zero(bridge_line->digest) ? + "no key listed" : hex_str(bridge_line->digest, DIGEST_LEN)); + + if (bridge_line->socks_args) { /* print socks arguments */ + int i = 0; + + tor_assert(smartlist_len(bridge_line->socks_args) > 0); + + log_debug(LD_GENERAL, "Bridge uses %d SOCKS arguments:", + smartlist_len(bridge_line->socks_args)); + SMARTLIST_FOREACH(bridge_line->socks_args, const char *, arg, + log_debug(LD_CONFIG, "%d: %s", ++i, arg)); + } + } + + bridge_resolve_conflicts(&bridge_line->addr, + bridge_line->port, + bridge_line->digest, + bridge_line->transport_name);
b = tor_malloc_zero(sizeof(bridge_info_t)); - tor_addr_copy(&b->addr, addr); - b->port = port; - if (digest) - memcpy(b->identity, digest, DIGEST_LEN); - if (transport_name) - b->transport_name = tor_strdup(transport_name); + tor_addr_copy(&b->addr, &bridge_line->addr); + b->port = bridge_line->port; + if (bridge_line->digest) + memcpy(b->identity, bridge_line->digest, DIGEST_LEN); + if (bridge_line->transport_name) + b->transport_name = bridge_line->transport_name; b->fetch_status.schedule = DL_SCHED_BRIDGE; - b->socks_args = socks_args; + b->socks_args = bridge_line->socks_args; if (!bridge_list) bridge_list = smartlist_new();
+ tor_free(bridge_line); /* Deallocate bridge_line now. */ + smartlist_add(bridge_list, b); }
diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index 48f678a..6a4bcea 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -97,11 +97,8 @@ int routerinfo_is_a_configured_bridge(const routerinfo_t *ri); int node_is_a_configured_bridge(const node_t *node); void learned_router_identity(const tor_addr_t *addr, uint16_t port, const char *digest); -struct smartlist_t; -void bridge_add_from_config(const tor_addr_t *addr, uint16_t port, - const char *digest, - const char *transport_name, - struct smartlist_t *socks_args); +struct bridge_line_t; +void bridge_add_from_config(struct bridge_line_t *bridge_line); void retry_bridge_descriptor_fetch_directly(const char *digest); void fetch_bridge_descriptors(const or_options_t *options, time_t now); void learned_bridge_descriptor(routerinfo_t *ri, int from_cache); diff --git a/src/test/test_util.c b/src/test/test_util.c index a307a79..606f831 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -838,17 +838,17 @@ static void test_util_string_is_key_value(void *ptr) { (void)ptr; - test_assert(string_is_key_value("key=value")); - test_assert(string_is_key_value("k=v")); - test_assert(string_is_key_value("key=")); - test_assert(string_is_key_value("x=")); - test_assert(string_is_key_value("xx=")); - test_assert(!string_is_key_value("=value")); - test_assert(!string_is_key_value("=x")); - test_assert(!string_is_key_value("=")); + test_assert(string_is_key_value(LOG_WARN, "key=value")); + test_assert(string_is_key_value(LOG_WARN, "k=v")); + test_assert(string_is_key_value(LOG_WARN, "key=")); + test_assert(string_is_key_value(LOG_WARN, "x=")); + test_assert(string_is_key_value(LOG_WARN, "xx=")); + test_assert(!string_is_key_value(LOG_WARN, "=value")); + test_assert(!string_is_key_value(LOG_WARN, "=x")); + test_assert(!string_is_key_value(LOG_WARN, "="));
/* ??? */ - /* test_assert(!string_is_key_value("===")); */ + /* test_assert(!string_is_key_value(LOG_WARN, "===")); */ done: ; }