Ticket25573 v2 #270

mikeperry-tor · 2018-08-09T00:02:45Z

Broken into functional commits.

We allow their CONNECTEDs, RESOLVEDs, ENDs, SENDMEs, and DATA cells to not count as dropped until the windows are empty, or we get an END. This commit does not change behavior. It only changes CIRC_BW event field values.

This commit does not change behavior. It only changes CIRC_BW field values.

Avoid data corrupton by avoiding mixing up old stream ids with new ones. This commit changes client behavior.

TRUNCATED cells were ignored while in path bias. Now they are obeyed, and cause us to tear down the circuit. The actual impact is minimal, since we would just wait around for a probe that would never arrive before. This commit changes client behavior.

coveralls · 2018-08-09T00:27:06Z

Coverage decreased (-0.1%) to 59.82% when pulling b8ae08c on mikeperry-tor:ticket25573-v2 into 0f0dac0 on torproject:maint-0.3.4.

teor2345

This is my review of "Ticket #25573: Track half-closed stream ids". (There is a lot of code here, so I'll do a review for each group of similar commits.)

In general, I think this code would be simpler and more performant if it used a hash table, rather than a smartlist. Using a hash table would make it easier to check correctness, too.

teor2345 · 2018-08-16T04:12:39Z

src/or/connection_edge.c

+  half_conn->connected_pending = conn->base_.state ==
+      AP_CONN_STATE_CONNECT_WAIT;
+
+  /* Data should only arrive if we're not waiting on a resolved.


Typo: "resolved cell"?

Fixed in 38dde96

teor2345 · 2018-08-16T04:12:39Z

src/or/connection_edge.c

+  if (!half_conns)
+    return 0;
+
+  SMARTLIST_FOREACH_BEGIN(half_conns, half_edge_t *, half_conn) {


How large do we expect half_conns to get?
Would it be better implemented as a hash table?

The half_conns list should be quite small, and is checked infrequently. Connections only get added when a client hangs up on a TCP connection early, before getting the END cell from the other side. This typically only happens in cases where the user interrupts client action by a cancel/stop/kill at the application layer.

It would take a very broken client to fill the half_closed list, and even if the half_closed connection table manages to get full, it will be at most 65535 uint16_t equality comparisons to go through it. And it will only be checked when using that same circuit for a new stream, or when getting stray data back with stale stream ids on that circuit.

I actually think that the additional memory allocation, indirection, indirect calls, and hashing overhead involved in a hash table will make it considerably more expensive to use for these small sizes, especially since we're only comparing int16_t values in the search.

All of this made me opt for the simple list for this (which is also what we use for our opened streams). The simple list also felt easier to check for lifetime and correctness issues. (Though the refactoring you suggested does make this even easier).

teor2345 · 2018-08-16T04:12:39Z

src/or/connection_edge.c

+/**
+ * Check if this stream_id is in a half-closed state. If so,
+ * check if it still has data cells pending, and decrement that
+ * window if so.


Conceptually, these seem like separate functions:

find the half-closed state for this stream

check if it still has data cells pending

decrement that window

You perform step 1 in all the connection_half_edge_is_valid_*() functions, and duplicate all that code. Consider using a macro or function pointer?

Maybe it's ok to duplicate the code as an optimisation. But using a hash table, the search code would be simpler.

Also, this function should be named after an action, not a question. Here's one possible name:

connection_half_edge_reduce_data_window

I like the idea of factoring out the search into a separate function. I did this in 8257aab.

It seems safer to me for callers to use explicit named functions for each of the window checks, rather than making them pass in a pointer to an int or something. This way, caller misuse is more easily spotted, and we don't have to worry about dangling pointers.

In my mind, it makes sense to have these calls be questions rather than action statements. This is consistent with the philosophy I used for the patch. None of these functions perform any actions that alter behavior. They only tell the caller if the cell counts as valid data for the control port statistics, so that the caller can count it (or not).

(e08e36f has an additional fixup to make the search function take a const list).

Thanks, the refactored code looks much better.

mikeperry-tor · 2018-08-20T20:04:25Z

src/or/connection_edge.c

+ */
+static void
+connection_half_edge_add(edge_connection_t *conn,
+                         origin_circuit_t *circ)


I added this in 8257aab.

teor2345 · 2018-08-16T04:12:39Z

src/or/connection_edge.c

+ */
+int
+connection_half_edge_is_valid_data(smartlist_t *half_conns,
+                                   streamid_t stream_id)


I don't know if you'll want to const smartlist_t *half_conns.

Technically, we don't modify it, but we do modify one of the items' attributes.

I guess this is consistent with the "these are verfication functions that don't alter behavior" notion. I made the containers const in b75fcb5

teor2345 · 2018-08-16T04:12:39Z

src/or/relay.c

+          if (connection_half_edge_is_valid_data(ocirc->half_streams,
+                                                 rh.stream_id)) {
+            circuit_read_valid_data(TO_ORIGIN_CIRCUIT(circ), rh.length);
+            log_info(domain,"data cell valid on half-closed stream.");
