[tor-bugs] #20853 [Core Tor/Tor]: rend_config_services should use service_is_ephemeral rather than old/new->directory

[Tor Bug Tracker & Wiki]

#20853: rend_config_services should use service_is_ephemeral rather than
old/new->directory
------------------------------------+------------------------------------
 Reporter:  teor                    |          Owner:  jryans
     Type:  defect                  |         Status:  needs_review
 Priority:  Medium                  |      Milestone:  Tor: 0.3.0.x-final
Component:  Core Tor/Tor            |        Version:  Tor: 0.2.9.5-alpha
 Severity:  Normal                  |     Resolution:
 Keywords:  easy refactor intro hs  |  Actual Points:
Parent ID:                          |         Points:  0.1
 Reviewer:                          |        Sponsor:
------------------------------------+------------------------------------
Changes (by jryans):

 * status:  needs_revision => needs_review


Comment:

 Updated patch at
 https://github.com/jryans/tor/commits/service_is_ephemeral.

 Replying to [comment:9 teor]:
 > Replying to [comment:8 jryans]:
 > > Replying to [comment:6 teor]:
 > > > Similarly, we can't change this part of rend_config_services:
 > > > {{{
 > > >         if (new->directory && old->directory &&
 > > >             !strcmp(old->directory, new->directory)) {
 > > > }}}
 > > > without adding explicit checks for the directories being valid
 before doing a strcmp on them.
 > >
 > > Okay, I added explicit `BUG` checks for the directory in addition to
 the ephemeral check for both of these cases.
 >
 > Thanks for this change.
 > The `BUG(new->directory)` and the old check are inverted, they will log
 a message when the directory exists, but we want to log a message when it
 doesn't.

 Ah, good catch!  I inverted the part inside `BUG` to log in the correct
 case, but I also have to invert outside `BUG` to chain with the rest of
 the conditional.  Not sure if `&& !BUG(!...) &&` is okay style.

 > Oh, and can we do them in the same order in
 rend_service_verify_single_onion_poison?
 > (That is, the ephemeral check first, then the directory check?)
 > Let's not log a BUG unless we really have to.

 Makes sense, updated the order.

 > > > > Not sure if a changes file was needed for this, but I added one
 anyway just to get familiar with the process.
 > > >
 > > > Thanks! Minor comment changes don't get a changes file, everything
 else does.
 > > > (I've submitted a number of code changes that were "Refactoring, no
 behaviour change", and then ended up being "unexpected bug in ticket XXXXX
 because behaviour changed during refactor".)
 > > >
 > > > One nitpick:
 > > >
 > > > the changes file format starts with
 > > > `Minor bugfix (optional categories):`
 > > >
 https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n56
 > >
 > > I wasn't sure if you wanted me to add an optional category or change
 to "Minor bugfix".  The current file (using the group "Code simplification
 and refactoring") passes the `lintChanges.py` script.  Let me know if
 there's still a change needed.
 >
 > Yes, please change it to 'Minor bugfix (hidden services)'.

 Okay, updated.  Added `bugfix on ...` as requested by lint tool as well.
 Comment 1 suspected that #20526 made it into 0.2.9.5, but `git describe
 --contains` on the commit from #20526 didn't find any tags, so I listed it
 as not in any released version.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/20853#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online