Controller: Stream event source field is "<unknown address type>:1" for DNS requests from the controller

My patch #8203 (moved) tried to be clever and copied the IP and port of the control connection into the SOURCE field of STREAM_EVENT_NEW_RESOLVE events. As it turns out, the control connection address can be a unix domain socket, which tor_dup_addr helpfully converts to "".

From the control Spec

  Address = ip4-address / ip6-address / hostname   (XXXX Define these)
(...)
"650" SP "STREAM" SP StreamID SP StreamStatus SP CircuitID SP Target
          [SP "REASON=" Reason [ SP "REMOTE_REASON=" Reason ]]
          [SP "SOURCE=" Source] [ SP "SOURCE_ADDR=" Address ":" Port ]
          [SP "PURPOSE=" Purpose]
          CRLF

Previously "(Tor_internal):0" was used, even though it also did not follow the control spec, but the controllers seemed to handle it fine.

My suggestion: If the control connection is via a unix domain socket, use "(Tor_internal)" again. And while we are at it, add (Tor_internal) to the control spec as well.

Trac:
Username: Desoxy

changed milestone to %Tor: 0.2.4.x-final

added component::core tor/tor milestone::Tor: 0.2.4.x-final owner::andrea priority::medium reporter::Desoxy resolution::fixed status::closed tor-client tor-spec type::defect version::tor 0.2.4.11-alpha labels

Tor patch: https://github.com/desoxy-tor/tor/compare/maint-0.2.4...bug-8639 Torspec patch: https://github.com/desoxy-tor/torspec/compare/master...bug-8639

Trac:
Username: Desoxy
Status: new to needs_review

Hi Desoxy. "(Tor_internal)" is certainly far better than "" (spaces would be a problem), but what about simply omitting the SOURCE field when it's a unix domain socket? It is an optional field, after all.

Either approach is okay with me, assuming that nothing (erroneously) requires a SOURCE_ADDR field.

Removing the SOURCE_ADDR field should work too. We would obviously have to adapt the control spec to mark the field as optional. Do we want to omit the SOURCE_ADDR field for all controller-generated stream events or just when the control connection is via an unix domain socket?

Trac:
Username: Desoxy

Removing the SOURCE_ADDR field should work too. We would obviously have to adapt the control spec to mark the field as optional.

Actually, it already is (that's what the '[]' means). At least I'm pretty sure that's the case, I don't recall where in the spec it says that - might be good to wait for Nick to confirm. :)

Trac:
Keywords: N/A deleted, tor-spec added

Trac:
Keywords: tor-spec deleted, tor-spec tor-client added

Let's do the one where we omit the SOURCE_ADDR when we don't have one, assuming that won't break anything.

Trac:
Status: needs_review to needs_revision

Trac:
Status: needs_revision to assigned
Owner: N/A to andrea

Observation: when we have an AF_UNIX control connection, the address field is the path to the socket before dnsserv_launch_request() does anything to it; do we still want to make this change to "(Tor_internal)" or leave it as the socket path?

Ah, wait, this is about the new entry_conn; never mind previous comment.

See my bug8639 branch for a proposed fix.

Trac:
Status: assigned to needs_review

I think above we said that the one we wanted to do was the version where we omit the SOURCE address when we don't have one, rather than having it be "(Tor_internal)". desoxy above already had a patch that did that.

Trac:
Status: needs_review to needs_revision

See my bug8639_v2 branch.

Trac:
Status: needs_revision to needs_review

Oh hang on; I'd worry that stuff will crash if there are connections with address == NULL flying around the code. Look for ->address in connection.c for example, and consider all the places we use it without checking for whether it's NULL.

What if we took the first version of your patch, and added the control.c changes from the second version, but checked for !strcmp(..,"(Tor_internal)") instead of NULL?

Replying to nickm:

Oh hang on; I'd worry that stuff will crash if there are connections with address == NULL flying around the code. Look for ->address in connection.c for example, and consider all the places we use it without checking for whether it's NULL.

What if we took the first version of your patch, and added the control.c changes from the second version, but checked for !strcmp(..,"(Tor_internal)") instead of NULL?

Ouch, the string form is that widely used? Okay, that would work; how would you feel about an empty string instead for the sake of memory savings?

Ick, it can't be just a string constant because if it isn't NULL something will surely try to free it. Allocating a string per connection just to flag "this is from an AF_UNIX control connection" seems really gross. :(

Either approach is fine with me, though I doubt it's much memory in total. There aren't many AF_UNIX connections in total, and everythin else does have the connection_t.address field set.

If you like, we can open up another ticket with the name "remove connection_t.address" in 0.2.5.x; that would save a few bytes per connection or so.

Replying to nickm:

If you like, we can open up another ticket with the name "remove connection_t.address" in 0.2.5.x; that would save a few bytes per connection or so.

It turns out we already have that ticket as #5528 (moved).

Replying to nickm:

Replying to nickm:

If you like, we can open up another ticket with the name "remove connection_t.address" in 0.2.5.x; that would save a few bytes per connection or so.

It turns out we already have that ticket as #5528 (moved).

Turns out ticket #5528 (moved) refers to routerinfo_t->address, not connection_t. Doing this for connection_t would be a bit more effort, I think; for routerinfo_t it's problematic for reasons I will describe on #5528 (moved).

Replying to nickm:

Either approach is fine with me, though I doubt it's much memory in total. There aren't many AF_UNIX connections in total, and everythin else does have the connection_t.address field set.

If you like, we can open up another ticket with the name "remove connection_t.address" in 0.2.5.x; that would save a few bytes per connection or so.

I have a new version that keeps "(Tor_internal)" and tests for it in control.c up in my bug8639_v3 branch.

Looks good; merging!

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

closed

moved to tpo/core/tor#8639 (closed)