Opened 10 years ago

Closed 10 years ago

Last modified 8 years ago

#2195 closed defect (fixed)

Handle the case when we can't generate descriptors

Reported by: karsten Owned by:
Priority: Medium Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version: Tor: 0.2.2.17-alpha
Severity: Keywords: tor-relay
Cc: rransom Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Relays that fail to generate a server descriptor or extra-info descriptor also fail to generate a server descriptor. They just print out a warning and retry a couple of milliseconds later.

rransom wrote a patch in his mitigate2183 branch that at least generates a server descriptor in case we cannot generate an extra-info descriptor.

Sebastian mentioned that failing may also be an option if we can't generate an extra-info descriptor, or we'll never learn about this bug.

We should decide what the best behavior in this case is and write a patch.

Child Tickets

Change History (18)

comment:1 Changed 10 years ago by nickm

Cc: rransom added
Milestone: Tor: 0.2.3.x-finalTor: 0.2.2.x-final
Status: newneeds_review

comment:2 Changed 10 years ago by nickm

Reviewing mitigate2183 :

  • You should use tor_digest_is_zero() when setting has_extra_info_digest.

Other than that, looks okay to me. Is it tested? It should be trivial enough to insert a "return -1" to the extrainfo_dump_to_string function to make sure the the routerinfo gets generated okay.

comment:3 Changed 10 years ago by karsten

See also my comments and rransom's reply in https://trac.torproject.org/projects/tor/ticket/2183#comment:18 .

Also, did we decide that not generating an extra-info descriptor is the desired behavior? See Sebastian's idea in the ticket description. I don't feel strongly here, but want to avoid that this gets lost.

comment:4 in reply to:  3 Changed 10 years ago by rransom

Replying to karsten:

Also, did we decide that not generating an extra-info descriptor is the desired behavior? See Sebastian's idea in the ticket description. I don't feel strongly here, but want to avoid that this gets lost.

Crashing a relay (thereby ending all circuits passing through it, losing its Stable and Guard flags, etc.) merely to draw attention to the fact that it could not generate an extra-info descriptor is a bad idea. Uploading a router descriptor without an extra-info-digest line will still be quite noticeable, and will be far less disruptive to the network.

comment:5 Changed 10 years ago by nickm

Agreed that crashing is not right.

comment:6 in reply to:  2 Changed 10 years ago by rransom

Replying to nickm:

Reviewing mitigate2183 :

  • You should use tor_digest_is_zero() when setting has_extra_info_digest.

Thanks! I didn't know there was a utility function for that.

Other than that, looks okay to me. Is it tested? It should be trivial enough to insert a "return -1" to the extrainfo_dump_to_string function to make sure the the routerinfo gets generated okay.

It's working for me now as a bridge (sanitized fingerprint 07DC 31AB 1422 1482 77C7 3BB3 7C2C 5A3B 7FF5 DBC9).

comment:7 Changed 10 years ago by rransom

My revised branch is pushed as fix2195-v2 ( git://repo.or.cz/tor/rransom.git fix2195-v2 ).

comment:8 Changed 10 years ago by nickm

+  has_extra_info_digest =
+    tor_digest_is_zero(router->cache_info.extra_info_digest);

Shouldn't there be a ! in there?

comment:9 in reply to:  8 Changed 10 years ago by rransom

Replying to nickm:

+  has_extra_info_digest =
+    tor_digest_is_zero(router->cache_info.extra_info_digest);

Shouldn't there be a ! in there?

Thanks! Fix pushed in 69472ca4210183ec11a632abd952b6060a086258.

comment:10 Changed 10 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

thanks for the fix on that; merging.

comment:11 Changed 10 years ago by cypherpunks

You left alone ei->cache_info.send_unencrypted = 1 with a possible ei == NULL.

comment:12 in reply to:  11 Changed 10 years ago by rransom

Replying to cypherpunks:

You left alone ei->cache_info.send_unencrypted = 1 with a possible ei == NULL.

Oops! Thanks for reporting that!

comment:13 Changed 10 years ago by rransom

Resolution: fixed
Status: closedreopened

comment:14 Changed 10 years ago by rransom

Status: reopenedneeds_review

See fix2195-fix ( ssh://mob@repo.or.cz/srv/git/tor/rransom.git fix2195-fix ).

comment:15 Changed 10 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged; thanks to cypherpunks for the review and rransom for the quick fix.

comment:16 Changed 10 years ago by cypherpunks

Do not dereference NULL if a bridge fails to build
It was about a public relay not a bridge:
!options->BridgeRelay
Bridge wasn't affect.

comment:17 Changed 8 years ago by nickm

Keywords: tor-relay added

comment:18 Changed 8 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.