Opened 5 weeks ago

Closed 4 weeks ago

#27236 closed defect (fixed)

When checking usable exit descriptors, always check exit policies

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.6.3-alpha
Severity: Normal Keywords: 034-must, 035-must, regression, tor-bridge 034-backport
Cc: dgoulet, intrigeri Actual Points:
Parent ID: #27080 Points:
Reviewer: nickm Sponsor:

Description

We need to check exit policies whenever we're counting the usable exit descriptors. At the moment, we only check exit policies for exits in ExitNodes.

    SMARTLIST_FOREACH_BEGIN(myexits_unflagged, const node_t *, node) {
      if (node_has_preferred_descriptor(node, 0) &&
          node_exit_policy_rejects_all(node)) {
        SMARTLIST_DEL_CURRENT(myexits_unflagged, node);
        /* this node is not actually an exit */
        np--;
        /* this node is unusable as an exit */
        nu--;
      }
    } SMARTLIST_FOREACH_END(node);

Child Tickets

TicketStatusOwnerSummaryComponent
#27230closedteorStop setting PathsNeededToBuildCircuits to a low valueCore Tor/Chutney

Change History (16)

comment:1 Changed 5 weeks ago by teor

Summary: Check allWhen checking usable exit descriptors, always check exit policies

comment:2 Changed 5 weeks ago by teor

Milestone: Tor: 0.3.5.x-finalTor: 0.3.4.x-final
Status: assignedneeds_review

See my branch bug27236-034 on https://github.com/teor2345/tor.git

Pull request: https://github.com/torproject/tor/pull/290

The new code always checks exit policies when checking the exit flag. There was a partial fix in 0.2.6.3-alpha, so I moved it into the function that checks all the nodes.

comment:3 Changed 5 weeks ago by teor

Oops, I didn't "make check-changes". I force-pushed a fix.

comment:4 Changed 5 weeks ago by nickm

LGTM; merging to 0.3.4 and forward.

comment:5 Changed 5 weeks ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:6 Changed 5 weeks ago by nickm

Resolution: fixed
Status: closedreopened

But actually, before we close this, please have a look at the merge commit in 2ae92ab973019cd88c5b610462a6be99a2ac4c6f -- there was a conflict in nodelist.c in the master branch.

comment:7 Changed 5 weeks ago by nickm

Status: reopenedmerge_ready

comment:8 in reply to:  6 Changed 5 weeks ago by teor

Resolution: fixed
Status: merge_readyclosed

Replying to nickm:

But actually, before we close this, please have a look at the merge commit in 2ae92ab973019cd88c5b610462a6be99a2ac4c6f -- there was a conflict in nodelist.c in the master branch.

Here's the conflict I see:

-     f_myexit= frac_nodes_with_descriptors(myexits, WEIGHT_FOR_EXIT, 0);
++    f_myexit= frac_nodes_with_descriptors(myexits,WEIGHT_FOR_EXIT, 0);

It's a whitespace change from #25886, and the merge restores the 0.3.4 version.

comment:9 Changed 5 weeks ago by teor

(Oh, and as a bonus, make test-network-all is fast again.)

comment:10 Changed 5 weeks ago by teor

Resolution: fixed
Reviewer: nickm
Status: closedreopened

I put the exit policy check in the wrong place, which made all exits require descriptors to be usable. Requiring descriptors for all exits skips the check that makes sure directories aren't trickling exits.

Unfortunately, this improved fix does not fix #27080. I think we might need to fix more of the #27080 children.

See my branch bug27236-034-fixup, which merges cleanly to master.

CI:

comment:11 in reply to:  10 Changed 5 weeks ago by teor

Status: reopenedneeds_review

Replying to teor:

Unfortunately, this improved fix does not fix #27080. I think we might need to fix more of the #27080 children.

It does fix #27080, I just checked twice on both master and 0.3.4.

Either I forgot to run the build, or bridges-min fails occasionally rather than all the time.

comment:12 Changed 5 weeks ago by nickm

Resolution: fixed
Status: needs_reviewclosed

okay, merged that to 0.3.4 and forward.

comment:13 Changed 4 weeks ago by teor

Resolution: fixed
Status: closedreopened

The latest patch broke all of the onion service networks in chutney.

comment:14 Changed 4 weeks ago by teor

Status: reopenedneeds_review

Please see my branch bug27236-034-hs, which fixes the onion service networks.
It needs the chutney fix in #27230 for bridges to bootstrap.

comment:16 Changed 4 weeks ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Cherry-picked to 0.3.4, merged forward.

Note: See TracTickets for help on using tickets.