[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #28636 [Core Tor/Tor]: Address WTF-PAD comments by Nick (2018-11-27 over IRC)
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs