#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@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs