[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
-------------------------------------------------+-------------------------
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 Replying to [comment:12 andrea]:
 > Proposed fix in my bug18322 branch; this introduces a new configuration
 option for the maximum size of dumped descriptors.  When this option is
 non-zero, the filename for dumped descriptors includes their hex-encoded
 SHA-256, which is referenced in the log message at the time of the dump.

 Hello. An initial review follows. I actually expected this feature to
 require less code, but it seems like fears of DoS made it more
 complicated. My current main concerns are the UX of this change, and the
 lack of tests.

 - 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?

 - 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?

 - I feel that this FIFO functionality is quite complex (lots of code
 added, asserts and length calculations) without any tests whatsoever. The
 general logic is also undocumented.

   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.

   Alternatively, if we like the FIFO approach, maybe some tests would help
 to bring more confidence here.

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

 - {{{if (debugfile_base != NULL) tor_free(debugfile_base);}}}

   `tor_free()` should be able to handle NULL as input, so the if check is
 not necessary. Also, maybe initializing `debugfile` and `debugfile_base`
 to `NULL` could be more defensive.

 Setting this to `needs_revision` for now. I'd like to do some more review
 and testing here eventually.

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