Opened 18 months ago

Last modified 8 days ago

#23790 needs_review defect

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:
Severity: Normal Keywords: 029-backport, 031-unreached-backport
Cc: asn Actual Points:
Parent ID: Points:
Reviewer: mikeperry Sponsor:


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 (10)

comment:1 Changed 18 months ago by nickm

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

comment:2 Changed 17 months ago by dgoulet

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

comment:3 Changed 17 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 17 months ago by dgoulet (previous) (diff)

comment:4 Changed 17 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 13 months ago by nickm

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

comment:6 Changed 9 months ago by nickm

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

comment:7 Changed 9 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 12 days 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 11 days 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:

Branch: ticket23790_029_01.

comment:10 Changed 8 days ago by asn

Reviewer: mikeperry
Note: See TracTickets for help on using tickets.