Opened 10 months ago

Closed 7 months ago

#23524 closed defect (fixed)

Avoid crashing when we ask for running bridges, but UseBridges is 0

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: 0.3.0.1-alpha
Severity: Normal Keywords: dos-resistance, 030-backport, 031-backport
Cc: Actual Points: 0.2
Parent ID: #24392 Points: 0.1
Reviewer: Sponsor:

Description

Instead, log a bug, and return the obvious answer: "there are no running bridges".

Child Tickets

Change History (20)

comment:1 Changed 10 months ago by teor

Keywords: dos-resistance added
Status: newneeds_review

Please see my branch bug23524.

This bug can't be triggered in existing code, so there's no need to backport it unless we think we'll backport extra calls to this function.

comment:2 Changed 10 months ago by teor

I added an additional commit to make sure the bug warning isn't triggered by the code I added in #23347.

comment:3 Changed 10 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Nice; thank you!

Merging now.

comment:4 Changed 8 months ago by teor

Keywords: 030-backport 031-backport removed
Parent ID: #24367
Resolution: fixed
Status: closedreopened

This was a bug on #23347, but now we're replacing uses of any_bridge_descriptors_known() iwith num_bridges_usable() > 0, we need to make this change there instead.

comment:5 Changed 8 months ago by teor

Status: reopenedneeds_review

This change is in my bug24367_032 branch.

comment:6 Changed 8 months ago by nickm

Keywords: review-group-26 added

Creating review-group-26.

comment:7 Changed 8 months ago by nickm

Owner: set to teor
Status: needs_reviewassigned

setting owner

comment:8 Changed 8 months ago by nickm

Status: assignedneeds_review

comment:9 Changed 8 months ago by nickm

Teor, did you mean "bug24367"? I don't see "bug24367_032".

comment:10 Changed 8 months ago by teor

Oops, I might have forgotten to push it. They are the same.

comment:11 Changed 8 months ago by teor

Now I've pushed bug24367_032, the relevant commit for this ticket is d7833c9d27.

comment:12 Changed 8 months ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.1.x-final
Status: needs_reviewmerge_ready

Yes, this seems okay; merging to 0.3.2. Should part of this be a backport candidate? If not, please close.

comment:13 Changed 8 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final
Status: merge_readyneeds_revision

I spoke too soon -- the unit tests break when I merge this branch:

  FAIL src/test/test_entrynodes.c:618: assert(gs_br OP_EQ NULL): 0x5585741f34b0 vs (nil)entrynodes/parse_from_state_full: 
  [parse_from_state_full FAILED]
entrynodes/get_guard_selection_by_name: 
  FAIL src/test/test_entrynodes.c:802: assert(gs3 == get_guard_selection_info())
  [get_guard_selection_by_name FAILED]

  FAIL src/test/test_entrynodes.c:2258: assert(r OP_EQ 0): -1 vs 0entrynodes/drop_guards: 
  [drop_guards FAILED]
entrynodes/outdated_dirserver_exclusion: 
  FAIL src/test/test_entrynodes.c:2777: expected log to contain "No primary or confirmed guards available."  Captured logs:
    1. info: "Not setting md restriction: only 0 usable guards.\n"
    2. info: "Trying to sample a reachable guard: We know of 0 in the USABLE_FILTERED set.\n"
    3. info: "  (That isn\'t enough. Trying to expand the sample.)\n"
    4. info: "Expanding the sample guard set. We have 0 guards in the sample, and 0 eligible guards to extend it with.\n"
    5. info: "Not expanding the guard sample any further; just ran out of eligible guards\n"
    6. info: "  (After filters [b], we have 0 guards to consider.)\n"
    7. info: "Trying to sample a reachable guard: We know of 0 in the USABLE_FILTERED set.\n"
    8. info: "  (That isn\'t enough. Trying to expand the sample.)\n"
    9. info: "Expanding the sample guard set. We have 0 guards in the sample, and 0 eligible guards to extend it with.\n"
   10. info: "Not expanding the guard sample any further; just ran out of eligible guards\n"
   11. info: "  (After filters [7], we have 0 guards to consider.)\n"
   12. info: "Absolutely no sampled guards were available. Marking all guards for retry and starting from top again.\n"
   13. info: "No router found for microdescriptor fetch; falling back to dirserver list.\n"
   14. notice: "While fetching directory info, no running dirservers known. Will try again later. (purpose 19)\n"

  [outdated_dirserver_exclusion FAILED]
4/1031 TESTS FAILED. (31 skipped)

comment:14 Changed 8 months ago by teor

Parent ID: #24367#24392

Yes, #24392 is assigned to you (nickm) to fix the unit tests, because I don't understand the guard code well enough.
I split it off from #24367 because that ticket deals with a lot of different issues.

comment:15 Changed 8 months ago by nickm

Status: needs_revisionmerge_ready

Okay, I found the problem: the entrynode tests were depending on some global-state that some of the earlier tests were munging. I haven't tracked down what global state exactly, though -- only enough to fix this.

See my branch bug24367_032, based on yours.

comment:16 Changed 8 months ago by teor

Ok, I think we can do the reviews of this in #24367.

comment:17 Changed 8 months ago by nickm

On this commit, I'm mainly waiting to know whether Roger thinks that the merge should be blocked because of #24486

comment:18 Changed 8 months ago by nickm

Keywords: review-group-27 added

comment:19 Changed 7 months ago by teor

Keywords: 030-backport 031-backport added; review-group-26 review-group-27 removed
Milestone: Tor: 0.3.2.x-finalTor: 0.3.0.x-final
Status: merge_readyneeds_revision

This was fixed in #24392 in master and 0.3.2.
We might also need to backport this to 0.3.0 and 0.3.1.

comment:20 Changed 7 months ago by teor

Actual Points: 0.10.2
Resolution: fixed
Status: needs_revisionclosed

I backported this in bug24367_030. I am happy for us to handle this in #24392.

Note: See TracTickets for help on using tickets.