Opened 3 years ago

Closed 3 years ago

#22281 closed defect (implemented)

Prevent pattern of bugs caused by calling get_options() within options_validate() etc

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-23
Cc: catalyst Actual Points: .1
Parent ID: Points:
Reviewer: dgoulet Sponsor:


In #22252 and elsewhere, we hit a problem with calling a function that called get_options() -- both directly and via networkstatus_get_latest_consensus() -- before the options were fully assigned.

We ought to do something to eliminate this pattern of bugs.

One possibility might be to 'fill in' the new options before they're completely validated. But we'd want to be able to roll back to the old options as needed if the new options _didn't_ validate.

Child Tickets

Change History (13)

comment:1 Changed 3 years ago by nickm

To note: this class of bugs affects us not only when first starting Tor, but also when we're moving from one configuration to another: instead of causing crashes in that case, bugs like this cause us to look at facets configuration when we should be validating the new configuration, and therefore to report inaccurate information.

comment:2 Changed 3 years ago by catalyst

Cc: catalyst added

comment:3 Changed 3 years ago by arma

I had thought, as a short term hack, to assign the current options we're testing, if the global options is null, and also to add a boolean "in_validation" to or_options_t, and then have get_options() warn when it returns an options set that has in_validation true. That would turn the current asserts into warns.

I'm not sure if that hack fixes enough to be worthwhile though.

Last edited 3 years ago by arma (previous) (diff)

comment:4 Changed 3 years ago by nickm

Actual Points: .1
Owner: set to nickm
Status: newaccepted

I think that approach could be reasonable. I wrote it up as a branch ticket22281. Needs review?

comment:5 Changed 3 years ago by nickm

Status: acceptedneeds_review

comment:6 Changed 3 years ago by nickm

Keywords: review-group-23 added

Put 0.3.2 needs_review and merge_ready tickets into review-group-23.

comment:7 Changed 3 years ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision

Should we also apply this to options_init_from_string() that calls options_validate() ?

comment:8 Changed 3 years ago by nickm

I think you're right. I added a fixup commit for this. Better now?

(I also ran it through the chutney tests to see if they would complain.)

comment:9 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

comment:10 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

Not sure here...

It seems that the "option validation" is broader then just calling options_validate(). Like in options_trial_assign(), it seems it is from validation up to when we set the option in set_options().

Shouldn't we follow that as well in options_init_from_string() that is reset in_option_validation after the set options. Or fix the above and not include set_options()? (I'm saying that because set_options() calls options_act() for which the very first thing it does is to call get_mutable_options() meaning triggering the assert...?)

comment:11 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

Seems reasonable. I've updated the ticket22281 branch to try to be consistent across all the paths. I find that it has to exclude set_options() generally.

comment:12 Changed 3 years ago by dgoulet

Status: needs_reviewmerge_ready


comment:13 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed


Note: See TracTickets for help on using tickets.