Opened 6 months ago

Closed 5 months ago

#20862 closed defect (fixed)

Unittest fail on master: FAIL src/test/test_options.c:662: expected log to contain "Could not resolve local Address "

Reported by: asn Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points: .1
Parent ID: Points: 0.2
Reviewer: Sponsor:

Description

On latest master (e6facbfe7abc50de374300ecd83873ead65b4b04) the following test fail occurs:

options/validate__authdir: [forking] 
  FAIL src/test/test_options.c:662: expected log to contain "Could not resolve local Address " "'this.should.not_exist.example.org'. Failing.\n"  Captured logs:
    1. warn: "Address \'this.should.not_exist.example.org\' resolves to private IP address \'172.19.52.3\'. Tor servers that use the default DirAuthorities must have public IP addresses.\n"

  [validate__authdir FAILED]

Child Tickets

Change History (7)

comment:1 Changed 6 months ago by nickm

Of interest: This doesn't happen for me, but asn said that he was on a train when he was running the tests. This is probably something to do with the train's messed-up network...

comment:2 Changed 6 months ago by nickm

  • Milestone changed from Tor: 0.2.9.x-final to Tor: 0.3.0.x-final

comment:3 Changed 6 months ago by nickm

The test data here is:

  options_test_data_t *tdata = get_options_test_data(
                                 "AuthoritativeDirectory 1\n"
                                 "Address this.should.not_exist.example.org");

I guess we assumed that every decent DNS hijacker would decline to hijack an address with an underscrore in it.

comment:4 Changed 5 months ago by nickm

  • Actual Points set to .1
  • Owner set to nickm
  • Status changed from new to assigned

Reproduced while on the Amtrak 93 Northeast Regional. Thanks, Amtrak!

There's a fix in bug20862 for this bug, #20863, and a few other cases I found.

comment:5 Changed 5 months ago by nickm

  • Status changed from assigned to needs_review

comment:6 Changed 5 months ago by asn

  • Status changed from needs_review to merge_ready

Hello,

I'm not near a NE regional train right now, so I can't really test it with a real DNS hijacker.

That said, the fix looks good to me and tests pass fine on a normal network, so this is most probably an improvement!

FWIW, should we leave this XXX in? It's more of a general comment than something we should address:
+ // XXXX But it _can_ exist, if you're DNS-hijacked.

Turning this to merge_ready thanks!

comment:7 Changed 5 months ago by nickm

  • Resolution set to fixed
  • Status changed from merge_ready to closed

Merging, with comment fix. Thanks!

Note: See TracTickets for help on using tickets.