Opened 7 months ago

Closed 7 months ago

#24975 closed defect (fixed)

sched: scheduler_notify_networkstatus_changed() calls select_scheduler() without the new consensus

Reported by: dgoulet Owned by: dgoulet
Priority: High Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 032-backport, tor-sched
Cc: pastly Actual Points:
Parent ID: Points:
Reviewer: armadev Sponsor:

Description

select_scheduler() is called when a new consensus arrives with scheduler_notify_networkstatus_changed() but never takes the new consensus for which at that point it is not yet set as the global consensus.

It needs to use new_c.

This is not good because of that, we can't change anything with the consensus at runtime.........

Thanks to Roger for finding this out here: https://oniongit.eu/dgoulet/tor/merge_requests/17#note_2153

Child Tickets

Change History (10)

comment:1 Changed 7 months ago by arma

I recommend if you want a function that gets called after the new consensus is in place (i.e. you don't care about what the old consensus used to say), move your call to the bottom of networkstatus_set_current_consensus.

comment:2 Changed 7 months ago by arma

Quoting the oniongit comment here for ease of history:
"""
When this function is called, the consensus *has not yet changed*. It is still old_c. It will be new_c soon. This is your chance to take new actions.

So if you ignore new_c and just call stuff that looks at "the consensus", you're going to be looking at the current consensus, i.e. old_c.

I think this is a bug in 0.3.2 with scheduler_notify_networkstatus_changed() too, in that set_scheduler() can call select_scheduler() which calls scheduler_can_use_kist() which calls kist_scheduler_run_interval(NULL) -- so even though kist_scheduler_run_interval knows how to receive an ns as an argument, it doesn't get new_c here.
"""

comment:3 Changed 7 months ago by dgoulet

Status: assignedneeds_review

See branch: bug24975_032_01

Important bug that affects any values set by the consensus for the KIST scheduler so we need to backport it.

comment:4 Changed 7 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

merged to 0.3.2 and forward.

comment:5 Changed 7 months ago by dgoulet

Resolution: fixed
Reviewer: armadev
Status: closedreopened

Ok sorry, I fucked up and arma noticed so good!

I've pushed a revert and then a commit that does the right thing.

The problem was that the function was moved _after_ but was still using get_latest_consensus() for the parameter of the "old consensus". After we set globally the new consensus, that doesn't work so well...

Solution is to go with a before and after function. To achieve such, the scheduler is a bit changed but only parameters that are actually never used are removed.

Branch: bug24975_032_01

comment:6 Changed 7 months ago by dgoulet

Status: reopenedneeds_review

comment:7 Changed 7 months ago by arma

New branch looks ok.

The changelog says "This lead [sic] to the scheduler failing to notice any consensus parameters that might have changed between consensuses" but I think it isn't quite that bad -- the bug meant that you picked up the new consensus params on the following consensus fetch.

So it could count as a minor bugfix and go only on master if we wanted to be more conservative.

Speaking of conflicts, when this gets merged there will be a conflict with the new dos_consensus_has_changed() call, which can be put in either the notify_before or notify_after function, so long as it's done right.

comment:8 Changed 7 months ago by arma

Oh, and eventually all of those other "things we do with a new consensus" should work their way out of networkstatus_set_current_consensus() into one of these new functions.

comment:9 Changed 7 months ago by nickm

I've merged to 0.3.2 and forward (on the theory that having only one version of this to maintain will make us happier). I put the dos_consensus_has_changed() call in notify_before but please let's change it as appropriate?

Leaving this open till somebody confirms the dos_consensus_has_changed() issue.

comment:10 in reply to:  9 Changed 7 months ago by dgoulet

Resolution: fixed
Status: needs_reviewclosed

Replying to nickm:

I've merged to 0.3.2 and forward (on the theory that having only one version of this to maintain will make us happier). I put the dos_consensus_has_changed() call in notify_before but please let's change it as appropriate?

Leaving this open till somebody confirms the dos_consensus_has_changed() issue.

Yes I do confirm that it is properly placed. Using the new consensus it was needs to be done so good!

Note: See TracTickets for help on using tickets.