[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

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

#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:
------------------------------------+------------------------------------

Comment (by teor):

 Replying to [comment:5 jryans]:
 > Okay, this seems like a pretty straightforward refactoring.  I believe
 I've made the requested changes:
 >
 > https://github.com/jryans/tor/commits/service_is_ephemeral

 Thanks for this patch!

 rend_service_verify_single_onion_poison() needs an explicit check for the
 directory, because rend_service_private_key_exists() requires there to be
 a directory, or tor asserts. And we want to be able to vary how we check
 for ephemeral services in future. (But you're right that we should check
 for ephemeral services.)

 So I think we can fix this by adding a check for s->directory as well as
 the ephemeral check.

 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.

 For bonus points, these extra checks should probably be BUG() checks,
 which log a bug warning. (If they ever get triggered, it's a coder's
 error.)

 A normal condition looks like:
 {{{
 if (boolean_condition) {
   action();
 }
 }}}


 A BUG condition looks like:
 {{{
 if (BUG(boolean_condition)) {
   action();
 }
 }}}

 > 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

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/20853#comment:6>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs