[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #18322 [Core Tor/Tor]: Log unparseable votes so they can be analysed
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs