Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#3512 closed defect (implemented)

Functions that perform hostname lookup should be clearly labeled

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

Description

Quick quiz: which of the following might do a DNS lookup, and which don't?

  1. tor_addr_port_parse()
  2. parse_addr_port()
  3. tor_addr_lookup()
  4. tor_addr_parse_mask_ports()
  5. tor_addr_from_str()
  6. parse_addr_and_port_range()

If remembered that A, B, and C can do DNS lookups, but that D, E, and F don't do DNS lookups, then you have a better memory than I.

We should rename the functions here so that everything that does a resolve has "resolve" or "lookup" in its name, and everything that only has "parse" in its name is parsing-only. We should also audit our use of the resolving functions to make sure that we're not calling any of them any place that we shouldn't.

The renaming should hold off till after we've merged the IPv6 conversion, since that touches almost all of the code we'd want to alter here.

Child Tickets

Change History (9)

comment:1 Changed 8 years ago by nickm

So here's what I'm planning. "lookup" should indicate that name resolution can happen; "parse" indicates that it doesn't.

tor_addr_port_parse becomes tor_addr_port_lookup
parse_addr_port becomes addr_port_lookup
tor_addr_lookup stays unchanged

tor_addr_parse_mask_ports stays unchanged
tor_addr_from_str becomes tor_addr_parse
parse_addr_and_port_range stays unchanged

comment:2 Changed 8 years ago by arma

We have a competing convention that "address" is a char *, and addr is a tor_addr_t (used to be a uint32_t).

Are you still planning to wait until after the ipv6 conversion?

ALl of that said, yes, the current situation is not good, and your suggested fix sounds good.

comment:3 in reply to:  2 Changed 8 years ago by nickm

Replying to arma:

We have a competing convention that "address" is a char *, and addr is a tor_addr_t (used to be a uint32_t).

I don't think that's competing; none of my renamings changes address <-> addr, and you can say "lookup" or "parse" whatever.

Are you still planning to wait until after the ipv6 conversion?

No; we can't wait forever.

ALl of that said, yes, the current situation is not good, and your suggested fix sounds good.

great.

comment:4 Changed 8 years ago by nickm

Status: newneeds_review

Have a quick look at branch bug3512 in my public repository

comment:5 Changed 8 years ago by nickm

Grepping for the string "lookup" to see what might be problematic:

  • I should rename tor_addr_parse_reverse_lookup_name and tor_addr_to_reverse_lookup_name; they are too close to violating the new convention.
  • addr_port_lookup is a dangerous function; it sometimes does a lookup, but you can also use it just to parse a name if set its "addr" argument to NULL. We should refactor so that everything that uses it, uses it for a lookup. Stuff that uses it for simple parsing includes: connection_exit_begin_conn, authority_cert_parse_from_string,

comment:6 Changed 8 years ago by nickm

Addressed those in branch bug3512

comment:7 Changed 8 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merging it now.

comment:8 Changed 7 years ago by nickm

Keywords: tor-client added

comment:9 Changed 7 years ago by nickm

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