#20853 closed defect (fixed)

rend_config_services should use service_is_ephemeral rather than old/new->directory

Reported by: teor Owned by: jryans
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: 0.2.9.5-alpha
Severity: Normal Keywords: easy refactor intro hs
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor:

Description

We missed this in the earlier refactor.
(Does that make it a bug? I think so.)

Child Tickets

Change History (14)

comment:1 Changed 18 months ago by teor

Version: Tor: 0.3.0.0-alpha-devTor: 0.2.9.5-alpha

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

comment:2 Changed 18 months ago by nickm

Yes, I'd call it a bug.

I'd suggest no backport, though

comment:3 Changed 18 months ago by teor

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

comment:4 Changed 18 months ago by jryans

Owner: set to jryans
Status: newassigned

comment:5 Changed 18 months ago by jryans

Status: assignedneeds_review

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.

comment:6 in reply to:  5 ; Changed 18 months ago by teor

Replying to 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

comment:7 Changed 18 months ago by teor

Status: needs_reviewneeds_revision

comment:8 in reply to:  6 ; Changed 18 months ago by jryans

Status: needs_revisionneeds_review

Thanks for the quick review! Updated patch at:

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

Replying to 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 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.

comment:9 in reply to:  8 ; Changed 18 months ago by teor

Status: needs_reviewneeds_revision

Replying to jryans:

Thanks for the quick review! Updated patch at:

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

Replying to 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)'.

comment:10 in reply to:  9 Changed 18 months ago by jryans

Status: needs_revisionneeds_review

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

Replying to teor:

Replying to jryans:

Replying to 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.

comment:11 Changed 18 months ago 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

comment:12 Changed 18 months ago by teor

Status: needs_reviewneeds_revision

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

comment:13 Changed 18 months ago by jryans

Status: needs_revisionmerge_ready

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

comment:14 Changed 18 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

merging, per teor's review.

Note: See TracTickets for help on using tickets.