Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#6226 closed defect (fixed)

pt unit tests do dns lookup on "abcd"

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: asn Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Unplug your ethernet (but leave the interface up), and then run tor's 'make test'. It will stall at "pt/parsing" for a minute or so.

It has to do with these two tests in src/test/test_pt.c:

  /* wrong addrport */
  strcpy(line,"CMETHOD trebuchet socks4 abcd");
  test_assert(parse_cmethod_line(line, mp) < 0);
[...]
  /* wrong addr type */
  strcpy(line,"SMETHOD trebuchet abcd");
  test_assert(parse_smethod_line(line, mp) < 0);

parse_cmethod_line() calls tor_addr_port_lookup() which calls tor_addr_lookup() which triggers the dns lookup.

I haven't explored whether having my resolver *answer* the dns query can change the behavior of the unit tests.

Child Tickets

Change History (8)

comment:1 Changed 7 years ago by nickm

This is also a name convention violation: functions with "_parse" in their name should really only do parsing. Functions with "_resolve" in their name should be the ones that do name lookup. Previously I only enforced this behavior for address-parsing/lookup functions, but there's no reason we can't propagate it further.

comment:2 Changed 7 years ago by asn

Status: newneeds_review

Please see branch bug6226 in https://git.gitorious.org/mytor/mytor.git.

comment:3 Changed 7 years ago by nickm

It looks like this patch changes the behavior of the parse_*_line functions so they just parsing, with no lookup. What will eventually do the lookup?

The changes file seems iffy; the change doesn't make it so we don't do the parsing when doing corrupted addresses: It makes it so we don't do the parsing in the parse_* functions at all.

comment:4 in reply to:  3 Changed 7 years ago by asn

Replying to nickm:

It looks like this patch changes the behavior of the parse_*_line functions so they just parsing, with no lookup. What will eventually do the lookup?

Hm, I'm confused. Why do you think that parse_{c,s}method_line() should do DNS lookups? Those functions are for parsing the {C,S}METHOD lines from the managed proxy, so that they know in which TCP ports its transports managed to bind (the addrport in the {C,S}METHOD line is later used in connection_or_connect() when using the transport proxy, IIRC).

I'm not sure if there are any cases where a pluggable transport proxy would return a domain name in a {C,S}METHOD.

The changes file seems iffy; the change doesn't make it so we don't do the parsing when doing corrupted addresses: It makes it so we don't do the parsing in the parse_* functions at all.

Hm, I'm confused again. We do the addrport parsing in parse_{c,s}method_line(), we just don't use a *_lookup function anymore.

comment:5 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Ah, so you're saying it was never supposed to do name resolution in the first place? In that case, this patch makes sense. I'll cherry-pick it onto 0.2.3.x.

comment:6 in reply to:  5 Changed 7 years ago by asn

Replying to nickm:

Ah, so you're saying it was never supposed to do name resolution in the first place? In that case, this patch makes sense. I'll cherry-pick it onto 0.2.3.x.

Correct. I think that when I first coded parse_{c,s}method_line(), #3512 was not merged and I was not aware that that addrport parsing function was also doing DNS lookups.

comment:7 Changed 7 years ago by nickm

Keywords: tor-client added

comment:8 Changed 7 years ago by nickm

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