Opened 7 years ago

Closed 5 years ago

#5528 closed enhancement (implemented)

router->address is redundant with router->addr

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client easy
Cc: ln5 Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

It used to be more, but these days address is always an IP address, and I believe it always matches router->addr.

So it's only around to save us the trouble of making a string out of ri->addr when we want one. Maybe that's not worth it anymore.

Here are the places where it's set:

In router_parse_entry_from_string():

  router->address = tor_strdup(tok->args[1]);
  if (!tor_inet_aton(router->address, &in)) {
    log_warn(LD_DIR,"Router address is not an IP address.");
    goto err;
  }
  router->addr = ntohl(in.s_addr);

In rewrite_node_address_for_bridge():

        ri->addr = tor_addr_to_ipv4h(&bridge->addr);
        tor_free(ri->address);
        ri->address = tor_dup_ip(ri->addr);

In router_rebuild_descriptor():

  ri->address = tor_dup_ip(addr);
  ri->nickname = tor_strdup(options->Nickname);
  ri->addr = addr;

I think these three might be all the places.

Child Tickets

Change History (13)

comment:1 Changed 7 years ago by ln5

Corresponds well with my notes from the "private bridges on IPv6" work:

  • rewrite_node_address_for_bridge() <-- learned_bridge_descriptor() <-- routerlist_descriptors_added()
  • router_rebuild_descriptor() using addr from router_pick_published_address() using
    • resolve_my_address() -- not IPv6 ready
    • router_guess_address_from_dir_headers() -- not IPv6 ready
  • So if we go for storing the IPv6 address this is a problem for clients if router_rebuild_descriptor() is ever called for bridges in bridge list, which it seems like it's not the case
  • router_parse_entry_from_string() uses arg[1] from a K_ROUTER ("router") descriptor, i.e. the IPv4 address
  • routerinfo_free -- freeing it

comment:2 Changed 7 years ago by arma

Somebody (maybe me later) should investigate how feasible it would be to open a similar ticket for conn->address.

comment:3 Changed 7 years ago by ln5

Cc: ln5 added

comment:4 Changed 7 years ago by rransom

How will this affect Tor's ability to fetch directory information from the DirPort of a relay like dannenberg which has a ‘real web server’ in front of Tor?

comment:5 in reply to:  4 Changed 7 years ago by arma

Replying to rransom:

How will this affect Tor's ability to fetch directory information from the DirPort of a relay like dannenberg which has a ‘real web server’ in front of Tor?

Should be unrelated. Basically right now we preemptively malloc a string version of router->addr, and then carry it around ever after for when we need it. We could instead generate it on demand and then not keep it in between uses. Should have no impact on external-visible behavior.

comment:6 Changed 7 years ago by nickm

Keywords: tor-client added

comment:7 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:8 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

comment:9 Changed 6 years ago by arma

Keywords: easy added

comment:10 Changed 6 years ago by arma

Status: newneeds_review

See my ticket5528 branch.

comment:11 Changed 6 years ago by andrea

Hmm; the change in the unit test seems to hint at a problem with this: sometimes router->address is set from the public address rather than the address we're listening on, and thus does not always match router->addr. Are we okay with changing every instance of anything using router->address to use the actual listener address?

comment:12 in reply to:  11 Changed 6 years ago by arma

Replying to andrea:

Hmm; the change in the unit test seems to hint at a problem with this: sometimes router->address is set from the public address rather than the address we're listening on, and thus does not always match router->addr.

I believe this only happens in this one case in the unit tests. Basically, our test assumed that it could set address and addr to different things. Nothing else in the code thinks this.

Are we okay with changing every instance of anything using router->address to use the actual listener address?

Don't be fooled about 'actual listener address' or any of that. router->addr is always set from the IP address in the router descriptor, and router->address was always just the string representation of router->addr (except as you note in the unit tests, where it just fabricates some values and assigns them).

I assume you're looking at

-  strlcpy(buf2, "router Magri 18.244.0.1 9000 0 9003\n"
+  strlcpy(buf2, "router Magri 192.168.0.1 9000 0 9003\n"

The reason I made this change was because of

-  r1->address = tor_strdup("18.244.0.1");
   r1->addr = 0xc0a80001u; /* 192.168.0.1 */

We could have left it at "router Magri 18.244.0.1 9000 0 9003\n" if somebody wants to compute what the hex of 18.244.0.1 is (and then we assign that to r1->addr).

comment:13 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

This is surprisingly easy to merge, considering the code drift we've had since it was written.

After another review, I think it looks okay. Merged to master.

Note: See TracTickets for help on using tickets.