#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 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)'. -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/20853#comment:9> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online _______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs