Opened 3 years ago

Closed 3 years ago

Last modified 13 months ago

#13066 closed defect (fixed)

smartlist_choose_node_by_bandwidth() does not consider bad exit

Reported by: asn Owned by: rl1987
Priority: Low Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client, 025-backport, 026-triaged-1, 2016-bug-retrospective
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

smartlist_choose_node_by_bandwidth() does not check for the bad exit flag, and can return badexited relays. The sister function smartlist_choose_node_by_bandwidth_weights correctly checks for the bad exit flag.

The good thing is that node_sl_choose_by_bandwidth() will only use the faulty function if the bandwidth weight system fails. This should not happen to the current network though. Hence, this does not seem to be a big issue.

Child Tickets

Change History (9)

comment:1 follow-up: Changed 3 years ago by nickm

  • Keywords 025-backport 026-triaged-1 added

Yes, this should get fixed. It doesn't sound hard to do; are there any hidden obstacles to fixing this?

comment:2 in reply to: ↑ 1 Changed 3 years ago by asn

Replying to nickm:

Yes, this should get fixed. It doesn't sound hard to do; are there any hidden obstacles to fixing this?

I don't think there are any hidden obstacles. I think it might be enough to replace

    is_exit = node->is_exit;

with

    is_exit = node->is_exit && ! node->is_bad_exit;

like smartlist_choose_node_by_bandwidth_weights() does.

FWIW, maybe a better long-term plan would be to replace those manual checks for is_exit/is_bad_exit with a node_is_good_exit() method, and only use that when checking whether a node is exit. It's unintuitive for new programmers to always check for those two flags instead of just is_exit.

comment:3 Changed 3 years ago by rl1987

  • Owner set to rl1987
  • Status changed from new to accepted

comment:4 Changed 3 years ago by nickm

See also #13126 , which is about removing smartlist_choose_node_by_bandwidth entirely. If that can be done, we don't need to do this ticket.

comment:5 Changed 3 years ago by rl1987

  • Status changed from accepted to needs_review

comment:6 Changed 3 years ago by nickm

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

lgtm; merged!

(And it's still IMO okay to remove this function entirely per #13126)

comment:7 Changed 13 months ago by nickm

  • Keywords 2016-bug-retrospective added

Mark bugs for 2016 bug retrospective based on hand-examination of changelogs for 0.2.5 onwards.

comment:8 Changed 13 months ago by nickm

Mark bugs for 2016 bug retrospective based on hand-examination of changelogs for 0.2.5 onwards.

comment:9 Changed 13 months ago by nickm

Mark bugs for 2016 bug retrospective based on hand-examination of changelogs for 0.2.5 onwards.

Note: See TracTickets for help on using tickets.