Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#13483 closed defect (fixed)

rend_consider_services_upload() sets initial next_upload_time which is clobbered when first intro point established?

Reported by: arma Owned by: dgoulet
Priority: Very Low Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: tor-hs, 027-triaged-1-in
Cc: Actual Points:
Parent ID: Points: small
Reviewer: Sponsor: SponsorR

Description

In rend_consider_services_upload(), if (!service->next_upload_time), it sets

      /* The fixed lower bound of 30 seconds ensures that the descriptor
       * is stable before being published. See comment below. */
      service->next_upload_time =
        now + 30 + crypto_rand_int(2*rendpostperiod);

which sure seems to a casual reader like Tor initializes its first upload for the hidden service descriptor to be 30 seconds + a random fraction of twice the RendPostPeriod. So, an hour give or take.

But then right below that we check

        (service->desc_is_dirty &&
         service->desc_is_dirty < now-30)) {

and the service gets dirty whenever an intro point is established, which presumably happens soon after you start up your Tor.

We should verify that this behavior is what I think it is, and then we should at the least fix all the comments to be more accurate, and probably we should get rid of the "up to 2 hours" part of the code since it isn't working anyway.

Child Tickets

Change History (13)

comment:1 Changed 5 years ago by dgoulet

Status: newneeds_review

See branch bug13483_026_v1 with a fix for this.

Comment and code has been cleaned up. I've also did a very minor refactoring of using a SMARLIST_FOREACH_BEGIN and changed the rendpostinterval to time_t. The behaviour is unchanged, tested in chutney.

Please careful review, it's an important function of tor! :)

comment:2 Changed 5 years ago by nickm

How much testing have you done on it so far?

Is there any hope of a unit test for this?

comment:3 in reply to:  2 ; Changed 5 years ago by dgoulet

Replying to nickm:

How much testing have you done on it so far?

Few unfortunately. I ran a chutney network and HS functionnalities are working (that is service is reachable and client can reach it). The behaviour shouldn't change here but better be careful than sorry so I'll deploy this on my test HS I run with reachability test. Couple of days should tell us if stable or not :)

Is there any hope of a unit test for this?

There are none :( ... could be a good time indeed to make one finally :)

comment:4 in reply to:  3 Changed 5 years ago by dgoulet

Replying to dgoulet:

Replying to nickm:

How much testing have you done on it so far?

Few unfortunately. I ran a chutney network and HS functionnalities are working (that is service is reachable and client can reach it). The behaviour shouldn't change here but better be careful than sorry so I'll deploy this on my test HS I run with reachability test. Couple of days should tell us if stable or not :)

This patch has been running for 18+ hours now on an HS service with a reachability test at each minutes. I can still access it successfully from 3 different locations.

comment:5 Changed 5 years ago by nickm

Keywords: sebastian-review added

comment:6 Changed 5 years ago by dgoulet

Milestone: Tor: 0.2.6.x-finalTor: 0.2.7.x-final
Owner: set to dgoulet
Status: needs_reviewaccepted

Postponing this one to 0.2.7 because adding a test would means some change to the code to have MOCK_DECL for some functions in rendservice.c which would add more changes to the branch. We are too near the release and I prefer going safe here.

Again, the current patch does NOT change the behavior of the function, just cleans it up be removing code that doesn't work, clarify comments and very minor refactor.

comment:7 Changed 5 years ago by Sebastian

Keywords: sebastian-review removed

ok, will gladly review once you're happy with the patch.

comment:8 Changed 4 years ago by dgoulet

After some thinking and research, thanks to special's help, here is what we think we should do.

The HS descriptor should be uploaded when *all* introduction points have been established. Else, the HS ends up uploading a descriptor that may contain intro points that are not yet "valid" meaning not yet established or proven to work. It could also trigger three uploads for the *same* descriptor if every intro points takes more than 30 seconds to establish because of desc_is_dirty being set at each intro established.

The HS will recover from that and upload a new one soon after when everything is established but that could be few minutes after.

Nothing critical but the point is to avoid jungling around and doing unnecessary uploads. With this, MIN_REND_INITIAL_POST_DELAY will be useless (and also especially with #12500 with something like endInitialPostPeriod)

If that makes sense, I'll propose a patch for it.

comment:9 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking more tickets as triaged-in for 0.2.7

comment:10 Changed 4 years ago by isabela

Points: small
Priority: normaltrivial
Version: Tor: 0.2.7

comment:11 Changed 4 years ago by dgoulet

Branch under review in #4862 fixes this ticket.
(bug4862_027_02 - commit: 542724470e0f9c3e935ea37178830a2d179a2b71)

comment:12 Changed 4 years ago by nickm

Resolution: fixed
Status: acceptedclosed

Merged #4862 branch. (But the commit message says that this was only "partially" fixed? Please reopen if there is more to do on this)

comment:13 Changed 4 years ago by dgoulet

Keywords: SponsorR removed
Sponsor: SponsorR
Note: See TracTickets for help on using tickets.