[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
[tor-commits] [tor/master] Fix parse_short_policy (bug 7192.)
commit 85659d3964669f9f419123c648e517f4ba539462
Author: Nick Mathewson <nickm@xxxxxxxxxxxxxx>
Date: Mon Oct 22 17:34:05 2012 -0400
Fix parse_short_policy (bug 7192.)
Our implementation of parse_short_policy was screwed up: it would
ignore the last character of every short policy. Obviously, that's
broken.
This patch fixes the busted behavior, and adds a bunch of unit tests
to make sure the rest of that function is okay.
Fixes bug 7192; fix on 0.2.3.1-alpha.
---
changes/bug7192 | 10 ++++++++
src/or/policies.c | 18 +++++++++-----
src/or/policies.h | 2 +-
src/test/test.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
4 files changed, 85 insertions(+), 9 deletions(-)
diff --git a/changes/bug7192 b/changes/bug7192
new file mode 100644
index 0000000..10cbc24
--- /dev/null
+++ b/changes/bug7192
@@ -0,0 +1,10 @@
+ o Major bugfixes:
+ - When parsing exit policy summaries from microdescriptors, we had
+ previously been ignoring the last character in each one, so that
+ "accept 80,443,8080" would be treated by clients as indicating a
+ node that allows access to ports 80, 443, and 808. That would lead
+ to clients attempting connections that could never work, and
+ ignoring exit nodes that would support their connections. Now clients
+ parse these exit policy summaries correctly. Fixes bug 7192;
+ bugfix on 0.2.3.1-alpha.
+
diff --git a/src/or/policies.c b/src/or/policies.c
index 486c264..bbd6816 100644
--- a/src/or/policies.c
+++ b/src/or/policies.c
@@ -1348,8 +1348,10 @@ parse_short_policy(const char *summary)
unsigned low, high;
char dummy;
char ent_buf[32];
+ size_t len;
next = comma ? comma+1 : strchr(summary, '\0');
+ len = comma ? (size_t)(comma - summary) : strlen(summary);
if (n_entries == MAX_EXITPOLICY_SUMMARY_LEN) {
log_fn(LOG_PROTOCOL_WARN, LD_DIR, "Impossibly long policy summary %s",
@@ -1357,20 +1359,22 @@ parse_short_policy(const char *summary)
return NULL;
}
- if (! TOR_ISDIGIT(*summary) || next-summary > (int)(sizeof(ent_buf)-1)) {
+ if (! TOR_ISDIGIT(*summary) || len > (sizeof(ent_buf)-1)) {
/* unrecognized entry format. skip it. */
continue;
}
- if (next-summary < 2) {
+ if (len < 1) {
/* empty; skip it. */
+ /* XXX This happens to be unreachable, since if len==0, then *summary is
+ * ',' or '\0', and the TOR_ISDIGIT test above would have failed. */
continue;
}
- memcpy(ent_buf, summary, next-summary-1);
- ent_buf[next-summary-1] = '\0';
+ memcpy(ent_buf, summary, len);
+ ent_buf[len] = '\0';
if (tor_sscanf(ent_buf, "%u-%u%c", &low, &high, &dummy) == 2) {
- if (low<1 || low>65535 || high<1 || high>65535) {
+ if (low<1 || low>65535 || high<1 || high>65535 || low>high) {
log_fn(LOG_PROTOCOL_WARN, LD_DIR,
"Found bad entry in policy summary %s", escaped(orig_summary));
return NULL;
@@ -1415,7 +1419,7 @@ parse_short_policy(const char *summary)
/** Write <b>policy</b> back out into a string. Used only for unit tests
* currently. */
-const char *
+char *
write_short_policy(const short_policy_t *policy)
{
int i;
@@ -1424,7 +1428,7 @@ write_short_policy(const short_policy_t *policy)
smartlist_add_asprintf(sl, "%s", policy->is_accept ? "accept " : "reject ");
- for(i=0; i < policy->n_entries; i++) {
+ for (i=0; i < policy->n_entries; i++) {
const short_policy_entry_t *e = &policy->entries[i];
if (e->min_port == e->max_port) {
smartlist_add_asprintf(sl, "%d", e->min_port);
diff --git a/src/or/policies.h b/src/or/policies.h
index b385d8e..f00d829 100644
--- a/src/or/policies.h
+++ b/src/or/policies.h
@@ -61,7 +61,7 @@ void policies_free_all(void);
char *policy_summarize(smartlist_t *policy);
short_policy_t *parse_short_policy(const char *summary);
-const char *write_short_policy(const short_policy_t *policy);
+char *write_short_policy(const short_policy_t *policy);
void short_policy_free(short_policy_t *policy);
int short_policy_is_reject_star(const short_policy_t *policy);
addr_policy_result_t compare_tor_addr_to_short_policy(
diff --git a/src/test/test.c b/src/test/test.c
index 9b510d2..ddfd633 100644
--- a/src/test/test.c
+++ b/src/test/test.c
@@ -1004,6 +1004,28 @@ test_circuit_timeout(void)
return;
}
+/* Helper: assert that short_policy parses and writes back out as itself,
+ or as <b>expected</b> if that's provided. */
+static void
+test_short_policy_parse(const char *input,
+ const char *expected)
+{
+ short_policy_t *short_policy = NULL;
+ char *out = NULL;
+
+ if (expected == NULL)
+ expected = input;
+
+ short_policy = parse_short_policy(input);
+ tt_assert(short_policy);
+ out = write_short_policy(short_policy);
+ tt_str_op(out, ==, expected);
+
+ done:
+ tor_free(out);
+ short_policy_free(short_policy);
+}
+
/** Helper: Parse the exit policy string in <b>policy_str</b>, and make sure
* that policies_summarize() produces the string <b>expected_summary</b> from
* it. */
@@ -1014,7 +1036,7 @@ test_policy_summary_helper(const char *policy_str,
config_line_t line;
smartlist_t *policy = smartlist_new();
char *summary = NULL;
- const char *summary_after;
+ char *summary_after = NULL;
int r;
short_policy_t *short_policy = NULL;
@@ -1231,6 +1253,46 @@ test_policies(void)
"accept *:*",
"reject 1,3,5,7");
+ /* Short policies with unrecognized formats should get accepted. */
+ test_short_policy_parse("accept fred,2,3-5", "accept 2,3-5");
+ test_short_policy_parse("accept 2,fred,3", "accept 2,3");
+ test_short_policy_parse("accept 2,fred,3,bob", "accept 2,3");
+ test_short_policy_parse("accept 2,-3,500-600", "accept 2,500-600");
+ /* Short policies with nil entries are accepted too. */
+ test_short_policy_parse("accept 1,,3", "accept 1,3");
+ test_short_policy_parse("accept 100-200,,", "accept 100-200");
+ test_short_policy_parse("reject ,1-10,,,,30-40", "reject 1-10,30-40");
+
+ /* Try parsing various broken short policies */
+ tt_ptr_op(NULL, ==, parse_short_policy("accept 200-199"));
+ tt_ptr_op(NULL, ==, parse_short_policy(""));
+ tt_ptr_op(NULL, ==, parse_short_policy("rejekt 1,2,3"));
+ tt_ptr_op(NULL, ==, parse_short_policy("reject "));
+ tt_ptr_op(NULL, ==, parse_short_policy("reject"));
+ tt_ptr_op(NULL, ==, parse_short_policy("rej"));
+ tt_ptr_op(NULL, ==, parse_short_policy("accept 2,3,100000"));
+ tt_ptr_op(NULL, ==, parse_short_policy("accept 2,3x,4"));
+ tt_ptr_op(NULL, ==, parse_short_policy("accept 2,3x,4"));
+ tt_ptr_op(NULL, ==, parse_short_policy("accept 2-"));
+ tt_ptr_op(NULL, ==, parse_short_policy("accept 2-x"));
+ tt_ptr_op(NULL, ==, parse_short_policy("accept 1-,3"));
+ tt_ptr_op(NULL, ==, parse_short_policy("accept 1-,3"));
+ /* Test a too-long policy. */
+ {
+ int i;
+ char *policy = NULL;
+ smartlist_t *chunks = smartlist_new();
+ smartlist_add(chunks, tor_strdup("accept "));
+ for (i=1; i<10000; ++i)
+ smartlist_add_asprintf(chunks, "%d,", i);
+ smartlist_add(chunks, tor_strdup("20000"));
+ policy = smartlist_join_strings(chunks, "", 0, NULL);
+ SMARTLIST_FOREACH(chunks, char *, ch, tor_free(ch));
+ smartlist_free(chunks);
+ tt_ptr_op(NULL, ==, parse_short_policy(policy));/* shouldn't be accepted */
+ tor_free(policy); /* could leak. */
+ }
+
/* truncation ports */
sm = smartlist_new();
for (i=1; i<2000; i+=2) {
_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits