Description

When tor receives more than one request for connecting to an onion service within a short amount of time, and these requests are circuit-isolated (by sockauth, or something else), tor will launch multiple connections to an intro point (one for each isolated request). When the first introduction succeeds (intro acks), tor closes any circuits it considers redundant (rend_client_close_other_intros). Tor should only close circuits if the socks request isolation bits match.

The request: Could somebody replace the "bugfix on 0.3.5.1-alpha" in the changelog with the actual version that first contained this bug? (If a bug first appears in A, and we fix it in B, the changelog should say "bugfix on A".)

The question: Does this apply to v3 onion services too? Should it?

The concern: I don't know if checking the "mixed" flags for equality is what we actually want here. (The flag "ISO_XYZ" is set in isolation_flags_mixed when the circuit have been used to connect streams with two different values for the "XYZ" parameter. Why are we looking at that?)

The request: Could somebody replace the "bugfix on 0.3.5.1-alpha" in the changelog with the actual version that first contained this bug? (If a bug first appears in A, and we fix it in B, the changelog should say "bugfix on A".)

I opened #27761 to catch bug fix changes files on future releases. But #27761 might not have caught this changes file, because bugfixes *can* happen on the most recent release.

I have pushed an updated version which checks the individual isolation flags. However, it requires nested for loops and a goto statement. It is committed already in the same PR (​https://github.com/torproject/tor/pull/311).

I don't believe I can write a unit test because of circuit_mark_for_close(). This function is mocked in other unit tests, but the mocked function is a dummy function that basically eliminates the functionality which needed by rend_client_close_other_intros() (which is closing circuits without isolation flags).

A mocked function for circuit_mark_for_close() looks something like this (code snippet from src/test):

I discussed this with Nick a bit on IRC and here is the reality with isolation flags. Here is the gist quoting him:

<+nickm> so, the isolation flags aren't really meaningful on their own...
<+nickm> An isolation flag ISO_FOO on a stream means "this stream must be isolated from all other streams with a different FOO"
<+nickm> but if stream 1 has ISO_FOO set, and stream 2 has a different value for "foo", then they can't share a circuit
<+nickm> If stream 1 has ISO_DESTPORT set, it can still share a circuit with stream 2 if stream 2 has the same value for its destport

That being said, Nick also pointed out to me this wonderful function: connection_edge_compatible_with_circuit() that should tell you if you can use the circuit or not for the given connection (in this case the SOCKS conn).

Sorry for all this confusion... it wasn't clear to me either how to proceed.

About the unit tests, you can do your own MOCKed function instead of using the one already in the unit test. Just add a new one with a meaningful name and do whatever you want in there.