Re: [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:  closed
 Priority:  Medium  |  Milestone:  Tor: 0.3.0.x-final
Component:  Core Tor/Tor|Version:  Tor: 0.2.9.5-alpha
 Severity:  Normal  | Resolution:  fixed
 Keywords:  easy refactor intro hs  |  Actual Points:
Parent ID:  | Points:  0.1
 Reviewer:  |Sponsor:
+
Changes (by nickm):

 * status:  merge_ready => closed
 * resolution:   => fixed


Comment:

 merging, per teor's review.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  merge_ready
 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 => merge_ready


Comment:

 Okay, I wasn't sure, so was just being cautious.  I updated the patch to
 remove the extra log.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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_revision
 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 teor):

 * status:  needs_review => needs_revision


Comment:

 Looks good, but we don't really need the extra `log_info(LD_REND,
 "Ephemeral HS started in non-anonymous mode.");`, because the BUG() macro
 logs a message anyway.

 But it's harmless, so it's up to you whether you want to revise it.
 Otherwise, please flip it to "ready to merge".

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
+

Comment (by jryans):

 Further updates to the patch at
 https://github.com/jryans/tor/commits/service_is_ephemeral after
 discussing on IRC:

 * Cleaned up format of the conditionals to read more clearly
 * Use `return -1` with poison checks
 * Add checks to `rend_service_poison_new_single_onion_dir` as well
   * I pulled up the log message for ephemeral services from
 `rend_service_poison_new_single_onion_dir_impl` since checking for this
 case in `rend_service_poison_new_single_onion_dir` would omit the message

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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 X
 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: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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_revision
 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 teor):

 * status:  needs_review => needs_revision


Comment:

 Replying to [comment:8 jryans]:
 > Thanks for the quick review!  Updated patch at:
 >
 > https://github.com/jryans/tor/commits/service_is_ephemeral
 >
 > Replying to [comment:6 teor]:
 > > 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.
 >
 > 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.

 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.

 > > > 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 X
 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)'.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:

 Thanks for the quick review!  Updated patch at:

 https://github.com/jryans/tor/commits/service_is_ephemeral

 Replying to [comment:6 teor]:
 > 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.

 Okay, I added explicit `BUG` checks for the directory in addition to the
 ephemeral check for both of these cases.

 > > 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 X
 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.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
+

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 X
 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: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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_revision
 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 teor):

 * status:  needs_review => needs_revision


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  assigned => needs_review


Comment:

 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

 Not sure if a changes file was needed for this, but I added one anyway
 just to get familiar with the process.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:  assigned
 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):

 * owner:   => jryans
 * status:  new => assigned


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 Type:  defect  | Status:  new
 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):

 I agree, I tend to avoid backports, I've been burnt before by backporting
 too many patches.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 Type:  defect  | Status:  new
 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 nickm):

 Yes, I'd call it a bug.

 I'd suggest no backport, though

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
 Type:  defect  | Status:  new
 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 teor):

 * version:  Tor: 0.3.0.0-alpha-dev => Tor: 0.2.9.5-alpha


Comment:

 I think this made it into 0.2.9.5, and it was #20526.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs