Opened 7 years ago

Closed 7 years ago

#8639 closed defect (fixed)

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

Reported by: Desoxy Owned by: andrea
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.11-alpha
Severity: Keywords: tor-spec tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

My patch #8203 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 "<unknown address type>".

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.

Child Tickets

Change History (21)

comment:2 Changed 7 years ago by atagar

Hi Desoxy. "(Tor_internal)" is certainly far better than "<unknown address type>" (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.

comment:3 Changed 7 years ago by nickm

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

comment:4 Changed 7 years ago by Desoxy

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?

comment:5 Changed 7 years ago by atagar

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. :)

comment:6 Changed 7 years ago by nickm

Keywords: tor-spec added

comment:7 Changed 7 years ago by nickm

Keywords: tor-client added

comment:8 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

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

comment:9 Changed 7 years ago by andrea

Owner: set to andrea
Status: needs_revisionassigned

comment:10 Changed 7 years ago by 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?

comment:11 Changed 7 years ago by andrea

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

comment:12 Changed 7 years ago by andrea

Status: assignedneeds_review

See my bug8639 branch for a proposed fix.

comment:13 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

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.

comment:14 Changed 7 years ago by andrea

Status: needs_revisionneeds_review

See my bug8639_v2 branch.

comment:15 Changed 7 years ago by 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?

comment:16 in reply to:  15 Changed 7 years ago by andrea

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. :(

comment:17 Changed 7 years ago by 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.

comment:18 in reply to:  17 ; Changed 7 years ago by 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.

comment:19 in reply to:  18 Changed 7 years ago by andrea

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.

Turns out ticket #5528 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.

comment:20 in reply to:  17 Changed 7 years ago by andrea

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.

comment:21 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks good; merging!

Note: See TracTickets for help on using tickets.