rend_service_prune_list_impl_() doesn't copy over desc_is_dirty when copying intro points

In rend_service_prune_list_impl_(void) (src/or/rendservice.c), the introduction points are copied over from the old to new rend_service_t:

smartlist_add_all(new->intro_nodes, old->intro_nodes);

but, the desc_is_dirty field is not copied over.

If a reload occurs between after a hidden service is added, but before its descriptor is published for the first time (triggered via desc_is_dirty), it will not publish its first descriptor until:

rendinitialpostdelay + crypto_rand_int(2*rendpostperiod)

It looks like it's simply missing new->desc_is_dirty = old->desc_is_dirty; prior to copying of introduction points.

Trac:
Username: jl

changed milestone to %Tor: 0.2.9.x-final

added 029-backport 031-unreached-backport actualpoints::0.1 component::core tor/tor consider-backport-after-0404 milestone::Tor: 0.2.9.x-final owner::dgoulet points::0.1 priority::medium reporter::jl resolution::fixed reviewer::asn severity::normal sponsor::27-can status::closed type::defect version::tor unspecified labels

Trac:
Cc: N/A to asn, dgoulet
Milestone: N/A to Tor: 0.3.2.x-final
Keywords: N/A deleted, 031-backport added

Trac:
Cc: asn, dgoulet to asn
Status: new to accepted
Owner: N/A to dgoulet

I've actually extended this to a bit more data that needed to be copied from the old service to the new service object. I'm also flagging this for to be considered maybe for 029-backport, if we do want 031 and 029 backport, I'll based the fix on those branch if we decide to.

Branch: bug23790_032_01

This is a nice find, it goes back up to 021. Thanks jl!

Trac:
Status: accepted to needs_review
Keywords: N/A deleted, 029-backport added

Merged to 0.3.2 and forward; marking for possible backport.

Trac:
Status: needs_review to merge_ready
Milestone: Tor: 0.3.2.x-final to Tor: 0.3.1.x-final

Is this bug bad enough to be worth backporting? The fix seems comparably safe.

Trac:
Milestone: Tor: 0.3.1.x-final to Tor: 0.2.9.x-final

0.3.1 is end of life, there are no more backports. Tagging with 031-unreached-backport instead.

Trac:
Keywords: 031-backport deleted, 031-unreached-backport added

This seems like a simple fix for a potential reachability issue on 0.2.9.

But I tried cherry-picking the commit back to 0.2.9, but it was non-trivial. Maybe we didn't backport some changes that this commit depends on?

dgoulet, if you think this is worth backporting, can you do a cherry-pick to 0.2.9, and make sure there is a clean merge to 0.3.4?

If you don't think we should backport, just close the ticket as fixed.

Trac:
Status: merge_ready to needs_revision

Replying to teor:

If you don't think we should backport, just close the ticket as fixed.

I do think we need to backport this. 029 has quite different code structure from 032. Here is the PR for 029:

PR: https://github.com/torproject/tor/pull/772 Branch: ticket23790_029_01.

Trac:
Status: needs_revision to needs_review

Trac:
Reviewer: N/A to mikeperry

Remove Mike as reviewer, because he's overloaded. We'll work out what to do with these tickets in the weekly meeting.

Trac:
Reviewer: mikeperry to N/A

I reviewed the 0.2.9 pull request: https://github.com/torproject/tor/pull/772

The new copy_service_on_prunning() function is the same as the 0.3.2 commit, but the call site is different: https://gitweb.torproject.org/user/dgoulet/tor.git/commit/?id=9592797cf3291fceb7715164e519e02a744c2a25

Once we get another review, I can backport this change.

Trac:
Points: N/A to 0.1
Sponsor: N/A to Sponsor27-can
Actualpoints: N/A to 0.1

Trac:
Reviewer: N/A to asn

LGTM!

Trac:
Status: needs_review to merge_ready

This fix has been in Tor 0.3.2 and later, so I could backport it straight away.

But I'll do it with the other post-0.4.0.4-alpha backports.

Trac:
Keywords: N/A deleted, consider-backport-after-0404-alpha added

Replying to teor:

This fix has been in Tor 0.3.2 and later

nickm, do we need to edit the changes file to say that this change is already in 0.3.2 and later?

Trac:
Cc: asn to asn, nickm

I don't think so -- I do that kind of thing (adding "backport from x.y.z") when I make a changelog.

Drop the -alpha from backport tags

Trac:
Keywords: consider-backport-after-0404-alpha deleted, consider-backport-after-0404 added

Merged PR 772 into 0.2.9, and merged forward to 0.3.4 using an "ours" merge to avoid carrying forward any changes. (We already merged a different fix into 0.3.2 and later.)

Merged #23790 (moved), #29665 (moved), #29017 (moved), #27199 (moved), #29144 (moved), #13221 (moved), #28698 (moved).

Trac:
Resolution: N/A to fixed
Version: Tor: 0.3.1.7 to Tor: unspecified
Status: merge_ready to closed

closed

changed time estimate to 48m

added 48m of time spent

mentioned in issue #27199 (moved)

mentioned in issue #28698 (moved)

mentioned in issue #29017 (moved)

mentioned in issue #29144 (moved)

mentioned in issue #29665 (moved)

moved to tpo/core/tor#23790 (closed)