Opened 20 months ago

Closed 5 weeks ago

#23790 closed defect (fixed)

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

Reported by: jl Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: 029-backport, 031-unreached-backport, consider-backport-after-0404
Cc: asn, nickm Actual Points: 0.1
Parent ID: Points: 0.1
Reviewer: asn Sponsor: Sponsor27-can

Description

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.

Child Tickets

Change History (19)

comment:1 Changed 20 months ago by nickm

Cc: asn dgoulet added
Keywords: 031-backport added
Milestone: Tor: 0.3.2.x-final

comment:2 Changed 19 months ago by dgoulet

Cc: dgoulet removed
Owner: set to dgoulet
Status: newaccepted

comment:3 Changed 19 months ago by dgoulet

Keywords: 029-backport added
Status: acceptedneeds_review

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!

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

comment:4 Changed 19 months ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.1.x-final
Status: needs_reviewmerge_ready

Merged to 0.3.2 and forward; marking for possible backport.

comment:5 Changed 15 months ago by nickm

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

comment:6 Changed 11 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.2.9.x-final

comment:7 Changed 11 months ago by teor

Keywords: 031-unreached-backport added; 031-backport removed

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

comment:8 Changed 3 months ago by teor

Status: merge_readyneeds_revision

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.

comment:9 in reply to:  8 Changed 2 months ago by dgoulet

Status: needs_revisionneeds_review

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.

comment:10 Changed 2 months ago by asn

Reviewer: mikeperry

comment:11 Changed 7 weeks ago by teor

Reviewer: mikeperry

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

comment:12 Changed 7 weeks ago by teor

Actual Points: 0.1
Points: 0.1
Sponsor: Sponsor27-can

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.

comment:13 Changed 6 weeks ago by asn

Reviewer: asn

comment:14 Changed 6 weeks ago by asn

Status: needs_reviewmerge_ready

LGTM!

comment:15 Changed 6 weeks ago by teor

Keywords: consider-backport-after-0404-alpha added

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.

comment:16 in reply to:  15 Changed 6 weeks ago by teor

Cc: nickm 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?

comment:17 Changed 6 weeks ago by nickm

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

comment:18 Changed 5 weeks ago by teor

Keywords: consider-backport-after-0404 added; consider-backport-after-0404-alpha removed

Drop the -alpha from backport tags

comment:19 Changed 5 weeks ago by teor

Resolution: fixed
Status: merge_readyclosed
Version: Tor: 0.3.1.7Tor: unspecified

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, #29665, #29017, #27199, #29144, #13221, #28698.

Note: See TracTickets for help on using tickets.