[tor-bugs] #28636 [Core Tor/Tor]: Address WTF-PAD comments by Nick (2018-11-27 over IRC)

[Tor Bug Tracker & Wiki]

#28636: Address WTF-PAD comments by Nick (2018-11-27 over IRC)
-------------------------------------------------+-------------------------
 Reporter:  asn                                  |          Owner:  (none)
     Type:  defect                               |         Status:  new
 Priority:  High                                 |      Milestone:  Tor:
                                                 |  unspecified
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  wtf-pad, tor-relay, tor-cell,        |  Actual Points:
  padding, 041-proposed                          |
Parent ID:  #28632                               |         Points:  3
 Reviewer:                                       |        Sponsor:
                                                 |  Sponsor2
-------------------------------------------------+-------------------------

Comment (by nickm):

 Replying to [comment:14 asn]:

 > b) Need to add type-checking to the downcasting macros in prob_distr.c .
 Some of it was done in d82a8a7f9d268728b2447b2dbbaa346140784f9b but I'm
 not sure if this is good enough. Might need to think of the right API here
 to come up with the best plan. **Nick let me know if you have any ideas
 here, but please don't spend too much time, becaues I can probably figure
 it out too.**

 I think that it's best to use inline functions for this kind of thing, so
 that you can assert.  Something like this:
 {{{
 static inline dist_to_geometric(struct dist *obj)
 {
   tor_assert(obj->ops == &geometric_ops);
   return SUBTYPE_P(obj, struct geometric, base));
 }
 }}}
 You could use a macro to define a bunch of these, since they're all
 basically the same.

 > c) `<+nickm> Also there are all these generic dist_ops functions that
 you could use ... but the API seems to encourage you not to use them.
 having wrapper functions instead that call dist->dist_ops.func(dist, ...)
 would be neat`
 >
 >I'm not sure what this comment calls for. **Can you please tell me some
 more about it?** Riastradh seems to expand on this on comment:5 but I
 cannot quite get it.

 It looks like these functions already exist.  They are dist_name,
 dist_sample, dist_cdf, and so on.  They do need documentation, though.

 Once those functions are documented, we should just use them everywhere
 outside of prob_distr.c.  (I think we already do in most places?)  I think
 we should make the definition of `struct dist_ops hidden`, so that other
 modules don't use it.  We'd have to make the various `_dist_ops` extern
 constants only expose their pointers, but that's actually the only thing
 that the rest of the code uses.

 > d) `<+nickm> oh, one list thing: I think src/lib/math is not allowed to
 include lib/crypt_ops, since that would introduce a circularity. Better
 make sure that it doesn't`
 >
 >Is this circular dependency still a problem? i see that `src/lib/math/`
 includes `lib/crypt_ops` but I don't see the other way around.** Is this
 for future proofing this, or is there currently a problem?** Also, would
 we be OK with the suggestion that Riastradh gives in comment:5 where we
 split prob_distr.c into the computational part, and the sampling part
 which calls `crypto_rand()`?

 I was wrong; lib_math is allowed to include lib/crypt_ops; see
 lib/math/.may_include.  Current versions of check-includes will detect
 circularities, so you don't need to worry about this in the future.

 Based on that, I think that splitting the code into sampling and
 computation would be elegant, but I wouldn't call it a super high
 priority.

 For the randomness part, btw, it would be good to look into
 crypto_fast_rng() here.  It's up to 100 times faster for short outputs,
 and this code is likely to get called a lot.

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