Opened 8 months ago

Closed 7 months ago

#25761 closed defect (fixed)

hs: Reload signal (HUP) doesn't remove a disabled service

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 032-backport, 033-backport, tor-hs, regression
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

Reported by a user on tor-onions@:

https://lists.torproject.org/pipermail/tor-onions/2018-April/000265.html

I can confirm that on 0.3.2, commenting the hidden service options in the torrc file and then HUP does NOT disable it for which it should!

Both v2 and v3 are affected. So somehow, we broke that feature in 032. I'm putting this in 034 milestone and mark it for possible backport.

Child Tickets

Change History (8)

comment:1 Changed 8 months ago by dgoulet

Owner: set to dgoulet
Status: newaccepted

comment:2 Changed 7 months ago by asn

I spent an hour today doing some debugging here. The main issue that causes the bug here is that when we enter rend_service_prune_list() after the HUP, to shutdown old services, we encounter rend_service_staging_list being NULL and we stop the pruning by hitting:

  /* Don't try to prune anything if we have no staging list. */
  if (!rend_service_staging_list) {
    return;
  }

Seems like rend_service_staging_list gets NULLed during the initial service configuration (before the HUP) in rend_service_prune_list_impl_() with:

  /* Finally, nullify the staging list pointer as we don't need it anymore
   * and it needs to be NULL before the next reload. */
  rend_service_staging_list = NULL;

and then nothing actually sets up the staging list after the HUP because we are not adding any services, so we skip the post-HUP pruning...

I guess what we need to do is to setup the staging list even when we have 0 services to configure? Or alternatively, don't check for !rend_service_prune_list when pruning? Need to dig more into this...

Also, the second case of test_prune_services_on_reload() was meant to test this particular case, but it passes without detecting the regression. :(

Last edited 7 months ago by asn (previous) (diff)

comment:3 Changed 7 months ago by asn

I think the fix here is to initialize rend_service_staging_list in rend_service_prune_list() instead of exiting the function:

  if (!rend_service_staging_list) {
-     return;
+     rend_service_staging_list = smartlist_new();
  }

dgoulet what you think? does this fit the whole staging/pruning logic?

comment:4 Changed 7 months ago by asn

Status: acceptedneeds_review

Please check out branch bug25761 for a v2 and v3 fix here.

comment:5 Changed 7 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision

lgtm;

But the branch needs to be based on maint-0.3.2 for backport. Thanks!

Last edited 7 months ago by dgoulet (previous) (diff)

comment:6 Changed 7 months ago by asn

Status: needs_revisionneeds_review

Pushed bug25761_032 and bug25761_033 which are based on maint-0.3.2 and maint-0.3.3 respectively.

comment:7 Changed 7 months ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

comment:8 Changed 7 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged to 0.3.2 and forward!

Note: See TracTickets for help on using tickets.