Opened 8 years ago

Closed 5 years ago

#4244 closed defect (fixed)

Tor changes default value of DirReqStatistics, then wants to SAVECONF the new default

Reported by: rransom Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay, 026-triaged-1
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

#4237 was caused by an underlying Tor bug.
We should fix it, and someday we should redesign the configuration-handling code to make this class of bugs go away.

Child Tickets

Change History (17)

comment:1 Changed 8 years ago by atagar

comment:2 Changed 8 years ago by rransom

Summary: Fix #4237Tor changes default value of DirReqStatistics, then wants to SAVECONF the new default

Sebastian found that when Tor's geoip file is not available, Tor changes its default value for DirReqStatistics from 1 to 0, but then the SAVECONF code doesn't know about the new default value, so it wants to save DirReqStatistics 0 in a regenerated torrc.

comment:3 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

comment:4 Changed 7 years ago by nickm

(moved this to 0.2.4.x because of the amount of mechanism I suspect will be involved.)

comment:5 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:6 Changed 7 years ago by nickm

Component: Tor RelayTor

comment:7 Changed 7 years ago by nickm

Keywords: 024-deferrable added

We might be able to do this by massaging global_default_options and global_options in sync.

comment:8 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final
Status: newneeds_review

We *can* do this with global_default_options, but it turns out that there are enough other places we do this that it would be a mistake. Instead, we should have *another* or_options_t object that tracks torrc+settings, without changes made internally by Tor.

That's doable, but it's too feature-y for 0.2.4 IMO. I have a TOTALLY UNTESTED fix in branch "bug4244" in my public repository.

comment:9 Changed 6 years ago by nickm

Keywords: 025-triaged added

comment:10 Changed 6 years ago by nickm

I can't see anything obviously wrong with this patch, but it seems a little complex for this stage in the game in 0.2.5 given the comparatively small size of the problem. Also, apparently the person who wrote it didn't write any tests for it. It might be best to let this alone until 0.2.6.... unless this is a much more serious problem than I think?

comment:11 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final
Status: needs_reviewneeds_revision

Deferring to 0.2.6. Needs testing to even be considered.

comment:12 Changed 5 years ago by nickm

Keywords: 026-triaged-1 added; 024-deferrable 025-triaged removed

comment:13 Changed 5 years ago by nickm

Keywords: nickm-patch added

Add nickm-patch keyword to needs_revision tickets in 0.2.6 where (I think) I wrote the patch, so I know which ones are my responsibility to revise.

comment:14 Changed 5 years ago by arma

Keywords: nickm-patch removed

My bug4244b branch has a much simpler, more targeted fix. I tested it and it works fine for me.

More generally, I think a trend of "thou shalt not modify the options to make it look like the user picked A when actually it said B" is something we should try to enforce.

I use NodeFamilySets, MaxMemInQueues, Socks4ProxyAddr, etc as my precedent here.

Note that this patch doesn't actually go through and fix every instance where we internally clobber what the torrc file said. But that makes it a reviewable and mergeable patch. :) Maybe it should even go into 0.2.5.x since TB users are seeing this one.

comment:15 Changed 5 years ago by arma

Status: needs_revisionneeds_review

comment:16 Changed 5 years ago by nickm

Almost right, but two problems:

  • I have no way to remember whether the _ version or the no-_ version is the one we should be looking at. Let's make the name that the code is supposed to use easier to remember, so that we're less likely to use the wrong one by mistake.
  • We no use identifiers that start with _, since those are reserved. Instead we could suffix with _.

Proposed fixup in "bug4244b" in my public repository. If you like, I'll squash and merge (or even cherry-pick to 0.2.5 is this is causing serious trouble)

comment:17 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Squashed as bug4244b_squashed and merged to master.

Note: See TracTickets for help on using tickets.