tor-commits March 2018

tor-commits@lists.torproject.org

15 participants
1430 discussions

[torspec/master] Fix an ambiguity in the grammar for Document.
by nickm＠torproject.org 19 Mar '18

19 Mar '18

commit 3e533e974b15d0ad71dca9466efef81f977e6859 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Mar 19 09:06:19 2018 -0400 Fix an ambiguity in the grammar for Document. Previously, a BEGIN/END block might be interpreted as starting with the keyword "-----BEGIN". This change specifically disallows that, to match the behavior of the existing Tor code. Fixes bug 25188. Bugfix from "witchof0x20". --- dir-spec.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dir-spec.txt b/dir-spec.txt index 186c4a8..bc17913 100644 --- a/dir-spec.txt +++ b/dir-spec.txt @@ -222,6 +222,8 @@ BeginLine ::= "-----BEGIN " Keyword "-----" NL EndLine ::= "-----END " Keyword "-----" NL + A Keyword may not be "-----BEGIN". + The BeginLine and EndLine of an Object must use the same keyword. When interpreting a Document, software MUST ignore any KeywordLine that

1 0

[metrics-lib/master] Override logLines method from LogDescriptor.
by karsten＠torproject.org 19 Mar '18

19 Mar '18

commit c54b264ad865d522bceb135d0fd0ab6b116e5f81 Author: iwakeh <iwakeh(a)torproject.org> Date: Mon Feb 26 15:24:26 2018 +0000 Override logLines method from LogDescriptor. --- .../java/org/torproject/descriptor/WebServerAccessLog.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/main/java/org/torproject/descriptor/WebServerAccessLog.java b/src/main/java/org/torproject/descriptor/WebServerAccessLog.java index 5f3ad73..a24b9cd 100644 --- a/src/main/java/org/torproject/descriptor/WebServerAccessLog.java +++ b/src/main/java/org/torproject/descriptor/WebServerAccessLog.java @@ -6,6 +6,7 @@ package org.torproject.descriptor; import java.time.LocalDate; import java.util.List; import java.util.Optional; +import java.util.stream.Stream; /** * Contains a sanitized web server access log file from a {@code torproject.org} @@ -63,6 +64,15 @@ public interface WebServerAccessLog extends LogDescriptor { public List<String> getUnrecognizedLines(); /** + * Returns a stream of all valid log lines. + * + * @since 2.3.0 + */ + @Override + public Stream<WebServerAccessLog.Line> logLines() + throws DescriptorParseException; + + /** * Facilitates access to all log line fields that don't only contain * default values post sanitization. *

1 0

[metrics-lib/master] Add change log entry for #25526.
by karsten＠torproject.org 19 Mar '18

19 Mar '18

commit 2e235ef8f38aac679817fa8e8b9171aadb3d5b9d Author: Karsten Loesing <karsten.loesing(a)gmx.net> Date: Mon Mar 19 11:58:37 2018 +0100 Add change log entry for #25526. --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d87ba4..d8576e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +# Changes in version 2.3.0 - 2018-0?-?? + + * Minor changes. + - Override logLines() method from LogDescriptor in + WebServerAccessLog. + + # Changes in version 2.2.0 - 2018-02-26 * Major changes

1 0

[tor/master] Remove duplicate code between parse_{c, s}method in transport.c
by nickm＠torproject.org 19 Mar '18

19 Mar '18

commit 130e2ffad74c04dbb2acaf94eb2be3879f6a898d Author: Deepesh Pathak <deepshpathak(a)gmail.com> Date: Sat Feb 24 20:27:08 2018 +0530 Remove duplicate code between parse_{c,s}method in transport.c - Merged common code in function parse_{c,s}method to a single function - Removed duplicate code in transport.c - Fixes #6236 --- changes/ticket6236 | 4 ++ src/or/transports.c | 182 ++++++++++++++++++++-------------------------------- 2 files changed, 75 insertions(+), 111 deletions(-) diff --git a/changes/ticket6236 b/changes/ticket6236 new file mode 100644 index 000000000..9dea07e69 --- /dev/null +++ b/changes/ticket6236 @@ -0,0 +1,4 @@ + o Minor bugfixes (Duplicate code): + - Remove duplicate code in parse_{c,s}method_line and bootstrap + their functionalities into a single function. Fixes + bug 6236; bugfix on 0.2.3.6-alpha. diff --git a/src/or/transports.c b/src/or/transports.c index b08dcd161..5cbf1ddbc 100644 --- a/src/or/transports.c +++ b/src/or/transports.c @@ -1025,48 +1025,71 @@ parse_method_error(const char *line, int is_server) line+strlen(error)+1); } -/** Parses an SMETHOD line and if well-formed it registers the - * new transport in mp. */ -STATIC int -parse_smethod_line(const char *line, managed_proxy_t *mp) +/** A helper for parse_{c,s}method_line(), bootstraps its + * functionalities. If is_smethod is true then the + * the line to parse is a SMETHOD line otherwise it is a + * CMETHOD line*/ +static int +parse_method_line_helper(const char *line, + managed_proxy_t *mp, + int is_smethod) { + int item_index = 0; int r; - smartlist_t *items = NULL; - char *method_name=NULL; + char *transport_name=NULL; char *args_string=NULL; char *addrport=NULL; - tor_addr_t tor_addr; + int socks_ver=PROXY_NONE; char *address=NULL; uint16_t port = 0; + const char *method_str = is_smethod ? PROTO_SMETHOD : PROTO_CMETHOD; + const int min_args_count = is_smethod ? 3 : 4; + + tor_addr_t tor_addr; transport_t *transport=NULL; + smartlist_t *items= smartlist_new(); - items = smartlist_new(); smartlist_split_string(items, line, NULL, SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, -1); - if (smartlist_len(items) < 3) { - log_warn(LD_CONFIG, "Server managed proxy sent us a SMETHOD line " - "with too few arguments."); + if (smartlist_len(items) < min_args_count) { + log_warn(LD_CONFIG, "Managed proxy sent us a %s line " + "with too few arguments.", method_str); goto err; } - /* Example of legit SMETHOD line: - SMETHOD obfs2 0.0.0.0:25612 ARGS:secret=supersekrit,key=superkey */ - - tor_assert(!strcmp(smartlist_get(items,0),PROTO_SMETHOD)); + tor_assert(!strcmp(smartlist_get(items, item_index),method_str)); + ++item_index; - method_name = smartlist_get(items,1); - if (!string_is_C_identifier(method_name)) { + transport_name = smartlist_get(items,item_index); + ++item_index; + if (!string_is_C_identifier(transport_name)) { log_warn(LD_CONFIG, "Transport name is not a C identifier (%s).", - method_name); + transport_name); goto err; } - addrport = smartlist_get(items, 2); + /** Check for the proxy method sent to us in CMETHOD line. */ + if (!is_smethod) { + const char *socks_ver_str = smartlist_get(items,item_index); + ++item_index; + + if (!strcmp(socks_ver_str,"socks4")) { + socks_ver = PROXY_SOCKS4; + } else if (!strcmp(socks_ver_str,"socks5")) { + socks_ver = PROXY_SOCKS5; + } else { + log_warn(LD_CONFIG, "Client managed proxy sent us a proxy protocol " + "we don't recognize. (%s)", socks_ver_str); + goto err; + } + } + + addrport = smartlist_get(items, item_index); + ++item_index; if (tor_addr_port_split(LOG_WARN, addrport, &address, &port)<0) { - log_warn(LD_CONFIG, "Error parsing transport " - "address '%s'", addrport); + log_warn(LD_CONFIG, "Error parsing transport address '%s'", addrport); goto err; } @@ -1081,10 +1104,11 @@ parse_smethod_line(const char *line, managed_proxy_t *mp) goto err; } - if (smartlist_len(items) > 3) { + /** Check for options in the SMETHOD line. */ + if (is_smethod && smartlist_len(items) > min_args_count) { /* Seems like there are also some [options] in the SMETHOD line. Let's see if we can parse them. */ - char *options_string = smartlist_get(items, 3); + char *options_string = smartlist_get(items, item_index); log_debug(LD_CONFIG, "Got options_string: %s", options_string); if (!strcmpstart(options_string, "ARGS:")) { args_string = options_string+strlen("ARGS:"); @@ -1092,15 +1116,20 @@ parse_smethod_line(const char *line, managed_proxy_t *mp) } } - transport = transport_new(&tor_addr, port, method_name, - PROXY_NONE, args_string); + transport = transport_new(&tor_addr, port, transport_name, + socks_ver, args_string); smartlist_add(mp->transports, transport); - /* For now, notify the user so that they know where the server - transport is listening. */ - log_info(LD_CONFIG, "Server transport %s at %s:%d.", - method_name, address, (int)port); + /** Logs info about line parsing success for client or server */ + if (is_smethod) { + log_info(LD_CONFIG, "Server transport %s at %s:%d.", + transport_name, address, (int)port); + } else { + log_info(LD_CONFIG, "Transport %s at %s:%d with SOCKS %d. " + "Attached to managed proxy.", + transport_name, address, (int)port, socks_ver); + } r=0; goto done; @@ -1115,93 +1144,24 @@ parse_smethod_line(const char *line, managed_proxy_t *mp) return r; } +/** Parses an SMETHOD line and if well-formed it registers the + * new transport in mp. */ +STATIC int +parse_smethod_line(const char *line, managed_proxy_t *mp) +{ + /* Example of legit SMETHOD line: + SMETHOD obfs2 0.0.0.0:25612 ARGS:secret=supersekrit,key=superkey */ + return parse_method_line_helper(line, mp, 1); +} + /** Parses a CMETHOD line, and if well-formed it registers * the new transport in mp. */ STATIC int parse_cmethod_line(const char *line, managed_proxy_t *mp) { - int r; - smartlist_t *items = NULL; - - char *method_name=NULL; - - char *socks_ver_str=NULL; - int socks_ver=PROXY_NONE; - - char *addrport=NULL; - tor_addr_t tor_addr; - char *address=NULL; - uint16_t port = 0; - - transport_t *transport=NULL; - - items = smartlist_new(); - smartlist_split_string(items, line, NULL, - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, -1); - if (smartlist_len(items) < 4) { - log_warn(LD_CONFIG, "Client managed proxy sent us a CMETHOD line " - "with too few arguments."); - goto err; - } - - tor_assert(!strcmp(smartlist_get(items,0),PROTO_CMETHOD)); - - method_name = smartlist_get(items,1); - if (!string_is_C_identifier(method_name)) { - log_warn(LD_CONFIG, "Transport name is not a C identifier (%s).", - method_name); - goto err; - } - - socks_ver_str = smartlist_get(items,2); - - if (!strcmp(socks_ver_str,"socks4")) { - socks_ver = PROXY_SOCKS4; - } else if (!strcmp(socks_ver_str,"socks5")) { - socks_ver = PROXY_SOCKS5; - } else { - log_warn(LD_CONFIG, "Client managed proxy sent us a proxy protocol " - "we don't recognize. (%s)", socks_ver_str); - goto err; - } - - addrport = smartlist_get(items, 3); - if (tor_addr_port_split(LOG_WARN, addrport, &address, &port)<0) { - log_warn(LD_CONFIG, "Error parsing transport " - "address '%s'", addrport); - goto err; - } - - if (!port) { - log_warn(LD_CONFIG, - "Transport address '%s' has no port.", addrport); - goto err; - } - - if (tor_addr_parse(&tor_addr, address) < 0) { - log_warn(LD_CONFIG, "Error parsing transport address '%s'", address); - goto err; - } - - transport = transport_new(&tor_addr, port, method_name, socks_ver, NULL); - - smartlist_add(mp->transports, transport); - - log_info(LD_CONFIG, "Transport %s at %s:%d with SOCKS %d. " - "Attached to managed proxy.", - method_name, address, (int)port, socks_ver); - - r=0; - goto done; - - err: - r = -1; - - done: - SMARTLIST_FOREACH(items, char*, s, tor_free(s)); - smartlist_free(items); - tor_free(address); - return r; + /* Example of legit CMETHOD line: + CMETHOD obfs2 socks5 127.0.0.1:35713 */ + return parse_method_line_helper(line, mp, 0); } /** Parses an PROXY-ERROR line and warns the user accordingly. */

1 0

[tor/master] Merge remote-tracking branch 'fristonio/ticket-6236'
by nickm＠torproject.org 19 Mar '18

19 Mar '18

commit b0f0c0f5505b3cf4cfbd4a3c1908ace4740f343b Merge: a324cd902 130e2ffad Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Mar 19 06:42:10 2018 -0400 Merge remote-tracking branch 'fristonio/ticket-6236' changes/ticket6236 | 4 ++ src/or/transports.c | 182 ++++++++++++++++++++-------------------------------- 2 files changed, 75 insertions(+), 111 deletions(-)

1 0

[collector/master] Add lines according to their count.
by karsten＠torproject.org 19 Mar '18

19 Mar '18

commit 2c00f28ab7eaba0310d2d96eda7f81f2a301a4a5 Author: iwakeh <iwakeh(a)torproject.org> Date: Wed Mar 7 13:46:54 2018 +0000 Add lines according to their count. Making the test from the previous commit pass and fixing task-25522. --- src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java b/src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java index 8ef9dde..df178b4 100644 --- a/src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java +++ b/src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java @@ -47,6 +47,7 @@ import java.util.SortedSet; import java.util.StringJoiner; import java.util.TreeMap; import java.util.TreeSet; +import java.util.function.UnaryOperator; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -188,7 +189,7 @@ public class SanitizeWeblogs extends CollecTorMain { } public static byte[] bytesFor(String line, long times) { - return Stream.of(line).limit(times) + return Stream.iterate(line, UnaryOperator.identity()).limit(times) .collect(Collectors.joining("\n", "", "\n")).getBytes(); }

1 0

[collector/master] Add a failing test.
by karsten＠torproject.org 19 Mar '18

19 Mar '18

commit 2e6fa506b3c9634837e6da4c8ad3dbf477936232 Author: iwakeh <iwakeh(a)torproject.org> Date: Wed Mar 7 13:46:51 2018 +0000 Add a failing test. Making a static method easier accessible for tests. --- .../collector/webstats/SanitizeWeblogs.java | 2 +- .../collector/webstats/SanitizeWeblogsTest.java | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java b/src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java index 5a270dd..8ef9dde 100644 --- a/src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java +++ b/src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java @@ -187,7 +187,7 @@ public class SanitizeWeblogs extends CollecTorMain { } } - private static byte[] bytesFor(String line, long times) { + public static byte[] bytesFor(String line, long times) { return Stream.of(line).limit(times) .collect(Collectors.joining("\n", "", "\n")).getBytes(); } diff --git a/src/test/java/org/torproject/collector/webstats/SanitizeWeblogsTest.java b/src/test/java/org/torproject/collector/webstats/SanitizeWeblogsTest.java new file mode 100644 index 0000000..9ad4996 --- /dev/null +++ b/src/test/java/org/torproject/collector/webstats/SanitizeWeblogsTest.java @@ -0,0 +1,20 @@ +/* Copyright 2017--2018 The Tor Project + * See LICENSE for licensing information */ + +package org.torproject.collector.webstats; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; + +public class SanitizeWeblogsTest { + + @Test + public void bytesForTest() { + String lines = "line\nline\nline\nline\nline\n" + + "line\nline\nline\nline\nline\n"; + assertEquals(lines, new String(SanitizeWeblogs.bytesFor("line", 10))); + } + +} +

1 0

[collector/master] Avoid calling bytesFor() unnecessarily.
by karsten＠torproject.org 19 Mar '18

19 Mar '18

commit 8c1bdd9a8403a0a19149b6388bbc940ee632eaae Author: Karsten Loesing <karsten.loesing(a)gmx.net> Date: Sat Mar 17 10:45:55 2018 +0100 Avoid calling bytesFor() unnecessarily. Found while looking into #25522. --- src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java b/src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java index df178b4..fd301b0 100644 --- a/src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java +++ b/src/main/java/org/torproject/collector/webstats/SanitizeWeblogs.java @@ -169,9 +169,12 @@ public class SanitizeWeblogs extends CollecTorMain { OutputStream os = FileType.XZ.outputStream(baos)) { for (Map.Entry<String, Long> entry : lines.entrySet()) { long count = entry.getValue(); - byte[] batch = bytesFor(entry.getKey(), BATCH); + byte[] batch = null; while (count > 0) { if (count > BATCH) { + if (null == batch) { + batch = bytesFor(entry.getKey(), BATCH); + } os.write(batch); count -= BATCH; } else {

1 0

[tor/master] cmux: Rename cell_ewma_set_scale_factor()
by nickm＠torproject.org 19 Mar '18

19 Mar '18

commit 9af5b625e8be64ed63ad5b19ae6c4b450e977ce0 Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Feb 15 13:51:34 2018 -0500 cmux: Rename cell_ewma_set_scale_factor() It is rename to something more meaningful that explains what it does exactly which is sets the EWMA options (currently only one exists). The new name is cmux_ewma_set_options(). Also, remove a public function from circuitmux_ewma.h that is only used in the C file. Make it static inline as well. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/circuitmux_ewma.c | 18 +++++++++--------- src/or/circuitmux_ewma.h | 6 +++--- src/or/config.c | 2 +- src/or/networkstatus.c | 2 +- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/or/circuitmux_ewma.c b/src/or/circuitmux_ewma.c index d9ee8d3ef..b2ace8a9f 100644 --- a/src/or/circuitmux_ewma.c +++ b/src/or/circuitmux_ewma.c @@ -241,6 +241,13 @@ circuitmux_policy_t ewma_policy = { /*** EWMA method implementations using the below EWMA helper functions ***/ +/** Compute and return the current cell_ewma tick. */ +static inline unsigned int +cell_ewma_get_tick(void) +{ + return ((unsigned)approx_time() / EWMA_TICK_LEN); +} + /** * Allocate an ewma_policy_data_t and upcast it to a circuitmux_policy_data_t; * this is called when setting the policy on a circuitmux_t to ewma_policy. @@ -610,13 +617,6 @@ cell_ewma_tick_from_timeval(const struct timeval *now, return res; } -/** Compute and return the current cell_ewma tick. */ -unsigned int -cell_ewma_get_tick(void) -{ - return ((unsigned)approx_time() / EWMA_TICK_LEN); -} - /* Default value for the CircuitPriorityHalflifeMsec consensus parameter in * msec. */ #define CMUX_PRIORITY_HALFLIFE_MSEC_DEFAULT 30000 @@ -672,8 +672,8 @@ get_circuit_priority_halflife(const or_options_t *options, /** Adjust the global cell scale factor based on options */ void -cell_ewma_set_scale_factor(const or_options_t *options, - const networkstatus_t *consensus) +cmux_ewma_set_options(const or_options_t *options, + const networkstatus_t *consensus) { double halflife; const char *source; diff --git a/src/or/circuitmux_ewma.h b/src/or/circuitmux_ewma.h index 4e9e512df..2ef8c2586 100644 --- a/src/or/circuitmux_ewma.h +++ b/src/or/circuitmux_ewma.h @@ -12,12 +12,12 @@ #include "or.h" #include "circuitmux.h" +/* The public EWMA policy callbacks object. */ extern circuitmux_policy_t ewma_policy; /* Externally visible EWMA functions */ -unsigned int cell_ewma_get_tick(void); -void cell_ewma_set_scale_factor(const or_options_t *options, - const networkstatus_t *consensus); +void cmux_ewma_set_options(const or_options_t *options, + const networkstatus_t *consensus); #endif /* !defined(TOR_CIRCUITMUX_EWMA_H) */ diff --git a/src/or/config.c b/src/or/config.c index 0ac777921..107f9ac5f 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -2073,7 +2073,7 @@ options_act(const or_options_t *old_options) configure_accounting(time(NULL)); /* Change the cell EWMA settings */ - cell_ewma_set_scale_factor(options, networkstatus_get_latest_consensus()); + cmux_ewma_set_options(options, networkstatus_get_latest_consensus()); /* Update the BridgePassword's hashed version as needed. We store this as a * digest so that we can do side-channel-proof comparisons on it. diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 80cdc9e5b..3455a7bb6 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -2003,7 +2003,7 @@ networkstatus_set_current_consensus(const char *consensus, update_consensus_networkstatus_fetch_time(now); /* Change the cell EWMA settings */ - cell_ewma_set_scale_factor(options, c); + cmux_ewma_set_options(options, c); /* XXXX this call might be unnecessary here: can changing the * current consensus really alter our view of any OR's rate limits? */

1 0

[tor/master] cmux: Remove round-robin circuit policy
by nickm＠torproject.org 19 Mar '18

19 Mar '18

commit c235c32bbcd33351d0d720fe2fb3dbac6369d12c Author: David Goulet <dgoulet(a)torproject.org> Date: Thu Feb 15 14:23:36 2018 -0500 cmux: Remove round-robin circuit policy Since 0.2.4, tor uses EWMA circuit policy to prioritize. The previous algorithm, round-robin, hasn't been used since then but was still used as a fallback. Now that EWMA is mandatory, remove that code entirely and enforce a cmux policy to be set. This is part of a circuitmux cleanup to improve performance and reduce complexity in the code. We'll be able to address future optimization with this work. Closes #25268 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/or/circuitlist.c | 5 - src/or/circuitmux.c | 338 +++------------------------------------------ src/or/or.h | 17 --- src/test/test_channel.c | 3 +- src/test/test_circuitmux.c | 2 + 5 files changed, 26 insertions(+), 339 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 29ad9a8ee..00726ca98 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -403,9 +403,6 @@ circuit_set_p_circid_chan(or_circuit_t *or_circ, circid_t id, circuit_set_circid_chan_helper(circ, CELL_DIRECTION_IN, id, chan); if (chan) { - tor_assert(bool_eq(or_circ->p_chan_cells.n, - or_circ->next_active_on_p_chan)); - chan->timestamp_last_had_circuits = approx_time(); } @@ -428,8 +425,6 @@ circuit_set_n_circid_chan(circuit_t *circ, circid_t id, circuit_set_circid_chan_helper(circ, CELL_DIRECTION_OUT, id, chan); if (chan) { - tor_assert(bool_eq(circ->n_chan_cells.n, circ->next_active_on_n_chan)); - chan->timestamp_last_had_circuits = approx_time(); } diff --git a/src/or/circuitmux.c b/src/or/circuitmux.c index 6ff03ab79..e601ffa55 100644 --- a/src/or/circuitmux.c +++ b/src/or/circuitmux.c @@ -114,13 +114,6 @@ struct circuitmux_s { */ chanid_circid_muxinfo_map_t *chanid_circid_map; - /* - * Double-linked ring of circuits with queued cells waiting for room to - * free up on this connection's outbuf. Every time we pull cells from - * a circuit, we advance this pointer to the next circuit in the ring. - */ - struct circuit_t *active_circuits_head, *active_circuits_tail; - /** List of queued destroy cells */ destroy_cell_queue_t destroy_cell_queue; /** Boolean: True iff the last cell to circuitmux_get_first_active_circuit @@ -188,18 +181,9 @@ chanid_circid_entry_hash(chanid_circid_muxinfo_t *a); static chanid_circid_muxinfo_t * circuitmux_find_map_entry(circuitmux_t *cmux, circuit_t *circ); static void -circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ, - cell_direction_t direction); +circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ); static void -circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ, - cell_direction_t direction); -static inline void -circuitmux_move_active_circ_to_tail(circuitmux_t *cmux, circuit_t *circ, - cell_direction_t direction); -static inline circuit_t ** -circuitmux_next_active_circ_p(circuitmux_t *cmux, circuit_t *circ); -static inline circuit_t ** -circuitmux_prev_active_circ_p(circuitmux_t *cmux, circuit_t *circ); +circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ); /* Static global variables */ @@ -209,115 +193,6 @@ static int64_t global_destroy_ctr = 0; /* Function definitions */ /** - * Linked list helpers - */ - -/** - * Move an active circuit to the tail of the cmux's active circuits list; - * used by circuitmux_notify_xmit_cells(). - */ - -static inline void -circuitmux_move_active_circ_to_tail(circuitmux_t *cmux, circuit_t *circ, - cell_direction_t direction) -{ - circuit_t **next_p = NULL, **prev_p = NULL; - circuit_t **next_prev = NULL, **prev_next = NULL; - circuit_t **tail_next = NULL; - or_circuit_t *or_circ = NULL; - - tor_assert(cmux); - tor_assert(circ); - - /* Figure out our next_p and prev_p for this cmux/direction */ - if (direction) { - if (direction == CELL_DIRECTION_OUT) { - tor_assert(circ->n_mux == cmux); - next_p = &(circ->next_active_on_n_chan); - prev_p = &(circ->prev_active_on_n_chan); - } else { - or_circ = TO_OR_CIRCUIT(circ); - tor_assert(or_circ->p_mux == cmux); - next_p = &(or_circ->next_active_on_p_chan); - prev_p = &(or_circ->prev_active_on_p_chan); - } - } else { - if (circ->n_mux == cmux) { - next_p = &(circ->next_active_on_n_chan); - prev_p = &(circ->prev_active_on_n_chan); - } else { - or_circ = TO_OR_CIRCUIT(circ); - tor_assert(or_circ->p_mux == cmux); - next_p = &(or_circ->next_active_on_p_chan); - prev_p = &(or_circ->prev_active_on_p_chan); - } - } - tor_assert(next_p); - tor_assert(prev_p); - - /* Check if this really is an active circuit */ - if ((*next_p == NULL && *prev_p == NULL) && - !(circ == cmux->active_circuits_head || - circ == cmux->active_circuits_tail)) { - /* Not active, no-op */ - return; - } - - /* Check if this is already the tail */ - if (circ == cmux->active_circuits_tail) return; - - /* Okay, we have to move it; figure out next_prev and prev_next */ - if (*next_p) next_prev = circuitmux_prev_active_circ_p(cmux, *next_p); - if (*prev_p) prev_next = circuitmux_next_active_circ_p(cmux, *prev_p); - /* Adjust the previous node's next pointer, if any */ - if (prev_next) *prev_next = *next_p; - /* Otherwise, we were the head */ - else cmux->active_circuits_head = *next_p; - /* Adjust the next node's previous pointer, if any */ - if (next_prev) *next_prev = *prev_p; - /* We're out of the list; now re-attach at the tail */ - /* Adjust our next and prev pointers */ - *next_p = NULL; - *prev_p = cmux->active_circuits_tail; - /* Set the next pointer of the tail, or the head if none */ - if (cmux->active_circuits_tail) { - tail_next = circuitmux_next_active_circ_p(cmux, - cmux->active_circuits_tail); - *tail_next = circ; - } else { - cmux->active_circuits_head = circ; - } - /* Set the tail to this circuit */ - cmux->active_circuits_tail = circ; -} - -static inline circuit_t ** -circuitmux_next_active_circ_p(circuitmux_t *cmux, circuit_t *circ) -{ - tor_assert(cmux); - tor_assert(circ); - - if (circ->n_mux == cmux) return &(circ->next_active_on_n_chan); - else { - tor_assert(TO_OR_CIRCUIT(circ)->p_mux == cmux); - return &(TO_OR_CIRCUIT(circ)->next_active_on_p_chan); - } -} - -static inline circuit_t ** -circuitmux_prev_active_circ_p(circuitmux_t *cmux, circuit_t *circ) -{ - tor_assert(cmux); - tor_assert(circ); - - if (circ->n_mux == cmux) return &(circ->prev_active_on_n_chan); - else { - tor_assert(TO_OR_CIRCUIT(circ)->p_mux == cmux); - return &(TO_OR_CIRCUIT(circ)->prev_active_on_p_chan); - } -} - -/** * Helper for chanid_circid_cell_count_map_t hash table: compare the channel * ID and circuit ID for a and b, and return less than, equal to, or greater * than zero appropriately. @@ -412,7 +287,7 @@ circuitmux_detach_all_circuits(circuitmux_t *cmux, smartlist_t *detached_out) */ if (to_remove->muxinfo.cell_count > 0) { - circuitmux_make_circuit_inactive(cmux, circ, CELL_DIRECTION_OUT); + circuitmux_make_circuit_inactive(cmux, circ); } /* Clear n_mux */ @@ -427,7 +302,7 @@ circuitmux_detach_all_circuits(circuitmux_t *cmux, smartlist_t *detached_out) */ if (to_remove->muxinfo.cell_count > 0) { - circuitmux_make_circuit_inactive(cmux, circ, CELL_DIRECTION_IN); + circuitmux_make_circuit_inactive(cmux, circ); } /* @@ -978,10 +853,10 @@ circuitmux_attach_circuit,(circuitmux_t *cmux, circuit_t *circ, */ if (hashent->muxinfo.cell_count > 0 && cell_count == 0) { --(cmux->n_active_circuits); - circuitmux_make_circuit_inactive(cmux, circ, direction); + circuitmux_make_circuit_inactive(cmux, circ); } else if (hashent->muxinfo.cell_count == 0 && cell_count > 0) { ++(cmux->n_active_circuits); - circuitmux_make_circuit_active(cmux, circ, direction); + circuitmux_make_circuit_active(cmux, circ); } cmux->n_cells -= hashent->muxinfo.cell_count; cmux->n_cells += cell_count; @@ -1029,20 +904,11 @@ circuitmux_attach_circuit,(circuitmux_t *cmux, circuit_t *circ, if (direction == CELL_DIRECTION_OUT) circ->n_mux = cmux; else TO_OR_CIRCUIT(circ)->p_mux = cmux; - /* Make sure the next/prev pointers are NULL */ - if (direction == CELL_DIRECTION_OUT) { - circ->next_active_on_n_chan = NULL; - circ->prev_active_on_n_chan = NULL; - } else { - TO_OR_CIRCUIT(circ)->next_active_on_p_chan = NULL; - TO_OR_CIRCUIT(circ)->prev_active_on_p_chan = NULL; - } - /* Update counters */ ++(cmux->n_circuits); if (cell_count > 0) { ++(cmux->n_active_circuits); - circuitmux_make_circuit_active(cmux, circ, direction); + circuitmux_make_circuit_active(cmux, circ); } cmux->n_cells += cell_count; } @@ -1106,7 +972,7 @@ circuitmux_detach_circuit,(circuitmux_t *cmux, circuit_t *circ)) if (hashent->muxinfo.cell_count > 0) { --(cmux->n_active_circuits); /* This does policy notifies, so comes before freeing policy data */ - circuitmux_make_circuit_inactive(cmux, circ, last_searched_direction); + circuitmux_make_circuit_inactive(cmux, circ); } cmux->n_cells -= hashent->muxinfo.cell_count; @@ -1143,81 +1009,16 @@ circuitmux_detach_circuit,(circuitmux_t *cmux, circuit_t *circ)) */ static void -circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ, - cell_direction_t direction) +circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ) { - circuit_t **next_active = NULL, **prev_active = NULL, **next_prev = NULL; - circuitmux_t *circuit_cmux = NULL; - chanid_circid_muxinfo_t *hashent = NULL; - channel_t *chan = NULL; - circid_t circ_id; - int already_active; - tor_assert(cmux); + tor_assert(cmux->policy); tor_assert(circ); - tor_assert(direction == CELL_DIRECTION_OUT || - direction == CELL_DIRECTION_IN); - - /* Get the right set of active list links for this direction */ - if (direction == CELL_DIRECTION_OUT) { - next_active = &(circ->next_active_on_n_chan); - prev_active = &(circ->prev_active_on_n_chan); - circuit_cmux = circ->n_mux; - chan = circ->n_chan; - circ_id = circ->n_circ_id; - } else { - next_active = &(TO_OR_CIRCUIT(circ)->next_active_on_p_chan); - prev_active = &(TO_OR_CIRCUIT(circ)->prev_active_on_p_chan); - circuit_cmux = TO_OR_CIRCUIT(circ)->p_mux; - chan = TO_OR_CIRCUIT(circ)->p_chan; - circ_id = TO_OR_CIRCUIT(circ)->p_circ_id; - } - - /* Assert that it is attached to this mux and a channel */ - tor_assert(cmux == circuit_cmux); - tor_assert(chan != NULL); - - /* - * Check if the circuit really was inactive; if it's active, at least one - * of the next_active and prev_active pointers will not be NULL, or this - * circuit will be either the head or tail of the list for this cmux. - */ - already_active = (*prev_active != NULL || *next_active != NULL || - cmux->active_circuits_head == circ || - cmux->active_circuits_tail == circ); - - /* If we're already active, log a warning and finish */ - if (already_active) { - log_warn(LD_CIRC, - "Circuit %u on channel " U64_FORMAT " was already active", - (unsigned)circ_id, U64_PRINTF_ARG(chan->global_identifier)); - return; - } - - /* - * This is going at the head of the list; if the old head is not NULL, - * then its prev pointer should point to this. - */ - *next_active = cmux->active_circuits_head; /* Next is old head */ - *prev_active = NULL; /* Prev is NULL (this will be the head) */ - if (cmux->active_circuits_head) { - /* The list had an old head; update its prev pointer */ - next_prev = - circuitmux_prev_active_circ_p(cmux, cmux->active_circuits_head); - tor_assert(next_prev); - *next_prev = circ; - } else { - /* The list was empty; this becomes the tail as well */ - cmux->active_circuits_tail = circ; - } - /* This becomes the new head of the list */ - cmux->active_circuits_head = circ; /* Policy-specific notification */ - if (cmux->policy && - cmux->policy->notify_circ_active) { + if (cmux->policy->notify_circ_active) { /* Okay, we need to check the circuit for policy data now */ - hashent = circuitmux_find_map_entry(cmux, circ); + chanid_circid_muxinfo_t *hashent = circuitmux_find_map_entry(cmux, circ); /* We should have found something */ tor_assert(hashent); /* Notify */ @@ -1232,99 +1033,16 @@ circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ, */ static void -circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ, - cell_direction_t direction) +circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ) { - circuit_t **next_active = NULL, **prev_active = NULL; - circuit_t **next_prev = NULL, **prev_next = NULL; - circuitmux_t *circuit_cmux = NULL; - chanid_circid_muxinfo_t *hashent = NULL; - channel_t *chan = NULL; - circid_t circ_id; - int already_inactive; - tor_assert(cmux); + tor_assert(cmux->policy); tor_assert(circ); - tor_assert(direction == CELL_DIRECTION_OUT || - direction == CELL_DIRECTION_IN); - - /* Get the right set of active list links for this direction */ - if (direction == CELL_DIRECTION_OUT) { - next_active = &(circ->next_active_on_n_chan); - prev_active = &(circ->prev_active_on_n_chan); - circuit_cmux = circ->n_mux; - chan = circ->n_chan; - circ_id = circ->n_circ_id; - } else { - next_active = &(TO_OR_CIRCUIT(circ)->next_active_on_p_chan); - prev_active = &(TO_OR_CIRCUIT(circ)->prev_active_on_p_chan); - circuit_cmux = TO_OR_CIRCUIT(circ)->p_mux; - chan = TO_OR_CIRCUIT(circ)->p_chan; - circ_id = TO_OR_CIRCUIT(circ)->p_circ_id; - } - - /* Assert that it is attached to this mux and a channel */ - tor_assert(cmux == circuit_cmux); - tor_assert(chan != NULL); - - /* - * Check if the circuit really was active; if it's inactive, the - * next_active and prev_active pointers will be NULL and this circuit - * will not be the head or tail of the list for this cmux. - */ - already_inactive = (*prev_active == NULL && *next_active == NULL && - cmux->active_circuits_head != circ && - cmux->active_circuits_tail != circ); - - /* If we're already inactive, log a warning and finish */ - if (already_inactive) { - log_warn(LD_CIRC, - "Circuit %d on channel " U64_FORMAT " was already inactive", - (unsigned)circ_id, U64_PRINTF_ARG(chan->global_identifier)); - return; - } - - /* Remove from the list; first get next_prev and prev_next */ - if (*next_active) { - /* - * If there's a next circuit, its previous circuit becomes this - * circuit's previous circuit. - */ - next_prev = circuitmux_prev_active_circ_p(cmux, *next_active); - } else { - /* Else, the tail becomes this circuit's previous circuit */ - next_prev = &(cmux->active_circuits_tail); - } - - /* Got next_prev, now prev_next */ - if (*prev_active) { - /* - * If there's a previous circuit, its next circuit becomes this circuit's - * next circuit. - */ - prev_next = circuitmux_next_active_circ_p(cmux, *prev_active); - } else { - /* Else, the head becomes this circuit's next circuit */ - prev_next = &(cmux->active_circuits_head); - } - - /* Assert that we got sensible values for the next/prev pointers */ - tor_assert(next_prev != NULL); - tor_assert(prev_next != NULL); - - /* Update the next/prev pointers - this removes circ from the list */ - *next_prev = *prev_active; - *prev_next = *next_active; - - /* Now null out prev_active/next_active */ - *prev_active = NULL; - *next_active = NULL; /* Policy-specific notification */ - if (cmux->policy && - cmux->policy->notify_circ_inactive) { + if (cmux->policy->notify_circ_inactive) { /* Okay, we need to check the circuit for policy data now */ - hashent = circuitmux_find_map_entry(cmux, circ); + chanid_circid_muxinfo_t *hashent = circuitmux_find_map_entry(cmux, circ); /* We should have found something */ tor_assert(hashent); /* Notify */ @@ -1383,12 +1101,12 @@ circuitmux_set_num_cells(circuitmux_t *cmux, circuit_t *circ, if (hashent->muxinfo.cell_count > 0 && n_cells == 0) { --(cmux->n_active_circuits); hashent->muxinfo.cell_count = n_cells; - circuitmux_make_circuit_inactive(cmux, circ, hashent->muxinfo.direction); + circuitmux_make_circuit_inactive(cmux, circ); /* Is the old cell count == 0 and the new cell count > 0 ? */ } else if (hashent->muxinfo.cell_count == 0 && n_cells > 0) { ++(cmux->n_active_circuits); hashent->muxinfo.cell_count = n_cells; - circuitmux_make_circuit_active(cmux, circ, hashent->muxinfo.direction); + circuitmux_make_circuit_active(cmux, circ); } else { hashent->muxinfo.cell_count = n_cells; } @@ -1417,6 +1135,9 @@ circuitmux_get_first_active_circuit(circuitmux_t *cmux, circuit_t *circ = NULL; tor_assert(cmux); + tor_assert(cmux->policy); + /* This callback is mandatory. */ + tor_assert(cmux->policy->pick_active_circuit); tor_assert(destroy_queue_out); *destroy_queue_out = NULL; @@ -1435,14 +1156,7 @@ circuitmux_get_first_active_circuit(circuitmux_t *cmux, /* We also must have a cell available for this to be the case */ tor_assert(cmux->n_cells > 0); /* Do we have a policy-provided circuit selector? */ - if (cmux->policy && cmux->policy->pick_active_circuit) { - circ = cmux->policy->pick_active_circuit(cmux, cmux->policy_data); - } - /* Fall back on the head of the active circuits list */ - if (!circ) { - tor_assert(cmux->active_circuits_head); - circ = cmux->active_circuits_head; - } + circ = cmux->policy->pick_active_circuit(cmux, cmux->policy_data); cmux->last_cell_was_destroy = 0; } else { tor_assert(cmux->n_cells == 0); @@ -1492,12 +1206,6 @@ circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ, /* Adjust the mux cell counter */ cmux->n_cells -= n_cells; - /* If we aren't making it inactive later, move it to the tail of the list */ - if (!becomes_inactive) { - circuitmux_move_active_circ_to_tail(cmux, circ, - hashent->muxinfo.direction); - } - /* * We call notify_xmit_cells() before making the circuit inactive if needed, * so the policy can always count on this coming in on an active circuit. @@ -1514,7 +1222,7 @@ circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ, */ if (becomes_inactive) { --(cmux->n_active_circuits); - circuitmux_make_circuit_inactive(cmux, circ, hashent->muxinfo.direction); + circuitmux_make_circuit_inactive(cmux, circ); } } diff --git a/src/or/or.h b/src/or/or.h index 03efdd1d8..4d3a11849 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3179,15 +3179,6 @@ typedef struct circuit_t { /** Index in smartlist of all circuits (global_circuitlist). */ int global_circuitlist_idx; - /** Next circuit in the doubly-linked ring of circuits waiting to add - * cells to n_conn. NULL if we have no cells pending, or if we're not - * linked to an OR connection. */ - struct circuit_t *next_active_on_n_chan; - /** Previous circuit in the doubly-linked ring of circuits waiting to add - * cells to n_conn. NULL if we have no cells pending, or if we're not - * linked to an OR connection. */ - struct circuit_t *prev_active_on_n_chan; - /** Various statistics about cells being added to or removed from this * circuit's queues; used only if CELL_STATS events are enabled and * cleared after being sent to control port. */ @@ -3467,14 +3458,6 @@ struct onion_queue_t; typedef struct or_circuit_t { circuit_t base_; - /** Next circuit in the doubly-linked ring of circuits waiting to add - * cells to p_chan. NULL if we have no cells pending, or if we're not - * linked to an OR connection. */ - struct circuit_t *next_active_on_p_chan; - /** Previous circuit in the doubly-linked ring of circuits waiting to add - * cells to p_chan. NULL if we have no cells pending, or if we're not - * linked to an OR connection. */ - struct circuit_t *prev_active_on_p_chan; /** Pointer to an entry on the onion queue, if this circuit is waiting for a * chance to give an onionskin to a cpuworker. Used only in onion.c */ struct onion_queue_t *onionqueue_entry; diff --git a/src/test/test_channel.c b/src/test/test_channel.c index 392759768..812ec6c1a 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -281,6 +281,7 @@ new_fake_channel(void) chan->state = CHANNEL_STATE_OPEN; chan->cmux = circuitmux_alloc(); + circuitmux_set_policy(chan->cmux, &ewma_policy); return chan; } @@ -582,8 +583,6 @@ test_channel_outbound_cell(void *arg) * to the channel's cmux as well. */ circuit_set_n_circid_chan(TO_CIRCUIT(circ), 42, chan); tt_int_op(channel_num_circuits(chan), OP_EQ, 1); - tt_assert(!TO_CIRCUIT(circ)->next_active_on_n_chan); - tt_assert(!TO_CIRCUIT(circ)->prev_active_on_n_chan); /* Test the cmux state. */ tt_ptr_op(TO_CIRCUIT(circ)->n_mux, OP_EQ, chan->cmux); tt_int_op(circuitmux_is_circuit_attached(chan->cmux, TO_CIRCUIT(circ)), diff --git a/src/test/test_circuitmux.c b/src/test/test_circuitmux.c index 854f72505..75b7a0ea4 100644 --- a/src/test/test_circuitmux.c +++ b/src/test/test_circuitmux.c @@ -7,6 +7,7 @@ #include "or.h" #include "channel.h" #include "circuitmux.h" +#include "circuitmux_ewma.h" #include "relay.h" #include "scheduler.h" #include "test.h" @@ -45,6 +46,7 @@ test_cmux_destroy_cell_queue(void *arg) cmux = circuitmux_alloc(); tt_assert(cmux); ch = new_fake_channel(); + circuitmux_set_policy(cmux, &ewma_policy); ch->has_queued_writes = has_queued_writes; ch->wide_circ_ids = 1;

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

tor-commits March 2018