Opened 3 years ago

Closed 2 years ago

#24587 closed defect (fixed)

Reset bootstrapping state on shutdown

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-mobile, s8-api
Cc: darkk, brade, mcs, ahf, nickm, mike@…, arthuredelstein Actual Points:
Parent ID: #23847 Points:
Reviewer: asn Sponsor:


Child Tickets

Change History (8)

comment:1 Changed 3 years ago by nickm

I've started some work here as restart_reset_bootstrap, but I would like to get some of its sibling-tickets merged first.

comment:2 Changed 3 years ago by nickm

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

comment:3 Changed 3 years ago by nickm

Keywords: review-group-31 added

comment:4 Changed 3 years ago by asn

Status: needs_reviewmerge_ready

Hey, did some review here as part of r-g-31.

Commit 6033538 looks very good to me.

Commit 3809036 also looks OK, but not very happy with it. I feel like resetting all the global statics on tor_free_all() makes sense but it's all a very brittle logic. The moment someone adds a new global static in that file and doesn't know about this convention of wiping at tor_free_all(), it will introduce a bug IIUC. Furthermore, the fact that some of those vars get reset to 0 and others to 1 is kinda arbitrary (and you need to look at their definitions to see if it's correct).

I wonder what we could do about 3809036 to make it better. Perhaps we should de-global those variables, put them in a struct, and initialize them in a function, then call that function from some entry-point and tor_free_all()? That seems like a better approach but probably not so trivial. Maybe subject for a future ticket on this area?

I'll mark this as merge_ready because I think it's correct, but we should think about the above concerns.

comment:5 Changed 3 years ago by asn

Reviewer: asn

comment:6 Changed 3 years ago by nickm

Keywords: review-group-31 removed

Remove merge_ready items that are in 0.3.4 or for backport consideration into 0.3.2 from review-group-31

comment:7 Changed 2 years ago by nickm

I've made #25493 to track looking for a better pattern here, since I think asn is right that the approach I've been using is fragile. But for now, I'll merge, since the current behavior can break restart on mobile.

comment:8 Changed 2 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged in master.

Note: See TracTickets for help on using tickets.