[tor-bugs] #18322 [Core Tor/Tor]: Log unparseable votes so they can be analysed

[Tor Bug Tracker & Wiki]

#18322: Log unparseable votes so they can be analysed
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:  andrea
     Type:  defect                               |         Status:
 Priority:  Medium                               |  needs_revision
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.9.x-final
 Keywords:  tor-dos, TorCoreTeam201606, review-  |        Version:
  group-4                                        |     Resolution:
Parent ID:  #17293                               |  Actual Points:  3
 Reviewer:                                       |         Points:  2
                                                 |        Sponsor:
                                                 |  SponsorU-can
-------------------------------------------------+-------------------------

Comment (by andrea):

 > The torrc option is called DetailedLogForUnparseableDescriptors but it
 actually requires a size input to work. With such a name, I would expect
 it to be a boolean option that enables or disables the feature. If we
 enable this feature by default as suggested above, maybe we can repurpose
 the torrc option to something that simply controls the maximum size?

 Yeah, it can be renamed to MaxUnparseableDescSizeToLog or something like
 that.

 > I think this feature is disabled by default. Why is that? Is there any
 way the old behavior is better than this feature with appropriate
 defaults?
 >
 > Also, when this feature is disabled, I think we fallback to the old
 behavior. Why would we ever want to do that? Why not enable this by
 default, with a small size allowance (e.g. 10MB) and remove all the old
 behavior code?

 Okay, sure.

 > I wonder if there is something simpler we could do instead of FIFO. Like
 just refuse to log more descriptors if the maximum size has been reached,
 and log this to the operator. Or maybe enforce limits based on the number
 of descriptors instead of the exact size.

 The former option would be acceptable; the latter would be a bad move -
 one of the descriptor types that dump_desc() can be called on is a new
 router descriptor uploaded to a dirauth.  I believe counting number rather
 than size lets anyone force a dirauth to consume as much disk space as it
 can allocate memory by uploading a huge, unparseable descriptor.  We
 really do want to watch how much we log here.

 > {{{if (emit_prefix) filelen = 50 + strlen(type) + len;}}} What is 50
 here?

 It's the maximum length of the file header naming the type of descriptor;
 it's ugly, but it's also been there since 2009:
 https://gitweb.torproject.org/tor.git/commit/?id=889c07f1fc54b5bd60c7919f8a5fc784eed2d57a

 That'll go if we get rid of the old code, of course.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18322#comment:16>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online