Re: [tor-bugs] #13339 [Tor]: Merge GSoC project - Consensus Diffs

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#13339: Merge GSoC project - Consensus Diffs
-----------------------------+-------------------------------------------
     Reporter:  mvdan        |      Owner:
         Type:  enhancement  |     Status:  needs_revision
     Priority:  major        |  Milestone:  Tor: 0.2.6.x-final
    Component:  Tor          |    Version:
   Resolution:               |   Keywords:  gsoc merge tor-client prop140
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+-------------------------------------------

Comment (by nickm):

 I gave it a look-over too.  Here's what I found:

 '''util.c'''
    * Maybe we should make sure that the rm_rf function won't follow
 symlinks.  Just in case.

 '''consdiff.c'''
    * ''I need to review this later''

 '''consdiff.h'''
    * Does anything outside of consdiff.c use the structure here?

 '''directory.c''':
    * `{get,set}_has_sent_bad_diff`: I would be more comfortable if these
 assertions were just log_ratelim(LOG_WARN, LD_BUG) calls instead.
    * `resolve_fetch_consensus`:
       * It's stupid, but I'd prefer to see a check vs INT_MAX before
 casting body_len to int.
       * You need to pass an mmap to tor_munmap, not tor_free
    * `connection_dir_client_reached_eof`: We should probably log when we
 get a bad diff, and who sent it to us.
    * `directory_handle_command_get`:
       * tor_memdup() is better than malloc-plus-copy.
       * dir_fps might need a better name now.

 '''dirserv.c'''
    * `dirserv_lookup_cached_consensus_diff_by_digest` should probably be
 renamed to `...by_hexdigest256`.
    * The documentation for `old_cached_consensus_by_digest` should mention
 the types of the keys and the values.
    * `new_cached_dir_comp` looks to me like it's going to cause a memory
 leak or a double-free somehow.
    * `dirserv_refresh_stored_consensuses` should probably do something to
 check that the digest format is right, the date format is right, and that
 the consensuses are not corrupted.
    * Use `char buf[constant]; tor_snprintf(buf, sizeof(buf)...)`, not
 `char buf[constant]; tor_snprintf(buf, constant...)`
    * Use the %ld printf format, not %li.  (Almost nobody uses %i)
    * I think we have an alternative to atol that checks for errors better.
 Is it `tor_parse_ulong`?
    * In dirserv_remove_old_consensuses, you can get the current index into
 the smartlist iteration by looking at `c_sl_idx`.  You don't need to have
 a separate `i`.
    * `dirserv_update_consensus_diffs`: ''I need to review this more
 later''.
    * `connection_dirserv_add_cons_diff_bytes_to_outbuf`: does this decref
 the cached object correctly?  Is this duplicate code?

 '''networkstatus.c'''
    * `networkstatus_get_old_consensuses_to_keep` -- this can just use int,
 instead of int32.
    * `current_{ns,md}_consensus_mmap` needs to get passed to tor_munmap if
 it's set before we mmap a new one.

 '''configuration'''
    * Instead of a boolean, maybe `options->SaveConsensuses` should be a
 number to save, or a maximum number of bytes to use when saving them?

 '''Later''':
    * I'd like to see fewer copies of strings done here. There's an easy
 way to do that, I think.
    * How expensive is dirserv_update_consensus_diffs?  It seems kind of
 pricey.  Maybe it needs to happen in the background?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/13339#comment:8>
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