Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4875 closed defect (fixed)

router_new_address_suggestion is not IPv6 aware

Reported by: dcf Owned by: ln5
Priority: Low Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.10-alpha
Severity: Keywords: ipv6 tor-client
Cc: ln5 Actual Points:
Parent ID: #3563 Points:
Reviewer: Sponsor:

Description

I see this in the debug log when using an IPv6 bridge. I censored the addresses but they are well-formed IPv6 addresses in the log.

Jan 08 12:49:16.000 [debug] connection_dir_client_reached_eof(): Received response from directory server '2600:3c01::...:9001': 200 "
OK" (purpose: 6)
Jan 08 12:49:16.000 [debug] router_new_address_suggestion(): Malformed X-Your-Address-Is header "2001:470::...". Ignoring.

Child Tickets

Change History (14)

comment:1 Changed 8 years ago by arma

Component: - Select a componentTor Client
Priority: minornormal

comment:2 Changed 8 years ago by ln5

Status: newaccepted

Yes, this needs fixing. I have a note about this being harmless for the current use case (but no explanation why). Thanks for creating a ticket.

comment:3 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-final
Priority: normalminor

It would be nice to have this fixed in 0.2.3.x, but it is not essetial.

comment:4 Changed 8 years ago by ln5

Parent ID: #3563

comment:5 Changed 8 years ago by ln5

Status: acceptedneeds_review

Branch bug4875 in my public repo fixes this. Tested on a client and a bridge relay.

comment:6 Changed 8 years ago by ln5

Note that bug4875 is based on bug4561_4.

comment:7 Changed 8 years ago by nickm

Since #4875 depends on #4561, either #4561 needs to go into 0.2.3.x, or this needs to go into 0.2.4.x, or the two branches need to be decoupled.

Which commit is the 4875 fix? I see a couple whose commit messages are just "first take" -- are they the ones I should be looking at?

comment:8 in reply to:  7 Changed 8 years ago by ln5

Replying to nickm:

Which commit is the 4875 fix? I see a couple whose commit messages are just "first take" -- are they the ones I should be looking at?

Please see 1ff25a3e.
Sorry about the mess here.

Let me know if you think that I should backport it to pre-#4561.

comment:9 Changed 8 years ago by nickm

re the patch contents:

  • Let's just always use strlcpy, not strcpy. OpenBSD spews warnings when you use strcpy, and the performance/convenience advantage just isn't worth it.
  • I don't like {AF_UNSPEC,{0}} as a literal initializer. If we want to have something like that, it ought to be a macro in address.h , so we use it everywhere and change it in one place only if the representation of tor_addr_t changes.
  • It needs a real commit message and a changes file.
  • I think that cherry-picking it back onto something pre-#4561 makes sense : when I tried "git cherry-pick 1ff25a3e", there was only one (trivial) conflict.
  • Can you tell me a little about the "XXX ipv6" instances? It looks like part of this patch makes parts of the code work with IPv6, but other parts of the patch assume that everything is still IPv4? Or do I understand wrong?

comment:10 Changed 8 years ago by ln5

Please see branch bug4875_2 based on master.

  • Using strlcpy now.
  • Created TOR_ADDR_NULL.
  • changes file added and commit message improved.
  • Now based on master.
  • Re "XXXX ipv6", yes this patch doesn't fix everything "all the way down". Functions or parts of functions involved in this patch which still don't handle IPv6 are marked with "XXXX ipv6". For example, router_new_address_suggestion() uses resolve_my_address() which still doesn't know about IPv6.

comment:11 Changed 8 years ago by nickm

Looks good. will review again in the morning, then probably merge.

comment:12 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Still looks good; merged and closing.

comment:13 Changed 7 years ago by nickm

Keywords: tor-client added

comment:14 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.