[tor-bugs] #19259 [Metrics/Onionoo]: uncaught NFE

[Tor Bug Tracker & Wiki]

#19259: uncaught NFE
-----------------------------+-----------------------------------
 Reporter:  iwakeh           |          Owner:  iwakeh
     Type:  defect           |         Status:  needs_information
 Priority:  High             |      Milestone:
Component:  Metrics/Onionoo  |        Version:
 Severity:  Major            |     Resolution:
 Keywords:                   |  Actual Points:
Parent ID:                   |         Points:
 Reviewer:                   |        Sponsor:
-----------------------------+-----------------------------------

Comment (by iwakeh):

 OK, I thought you could have a specification (maybe, on paper ;-)
 somewhere, that's why I asked  :-)


 Now, I basically just looked at the code and tried to infer what was
 intended.
 The resulting test draft is added for review.  This is just concerned with
 `WeightsStatus`; locale issue follows.

 Here some findings:

 1. First, weights are only counted as missing if less than -.5:
 {{{
         if (weights[i] < -0.5) {
           missingValues += 1 << i;
         }
 }}}
 See also `testMissingValues`.
 This should be moved into a separate method to make it testable.
 Does it really do what is intended?

 2. There are design choices that could cause trouble:

 The `setHistory`  method could introduce a history set with a totally
 different comparator. As this method is not used anywhere, it can just be
 removed.  Or, if it should be kept, just take the given history set and
 add the elements to the history after clearing it.  But, better remove
 unused code.

 The second more intricate issue is the Comparator itself:

 {{{
   private SortedMap<long[], double[]> history =
       new TreeMap<long[], double[]>(new Comparator<long[]>() {
     public int compare(long[] a, long[] b) {
       return a[0] < b[0] ? -1 : a[0] > b[0] ? 1 : 0;
     }
   });
 }}}

 (As an aside, the comparison of `long` values should be accomplished with
 `Long.compare()` and analogously with other numbers.)

 The used comparison is not consistent with 'equals', as no other measures
 are taken (except partially in `addToHistory`) this could lead to an
 inconsistent history list. The test `compareTest` prints an example (cf.
 the key value combination).


 3. So, the comparator (and related code) needs to be revised in order to
 reflect all the assumptions that can be inferred from `addToHistory` and
 `compressHistory`. Here the beginning of such a list:
 * All history intervals at most overlap in one point or have an empty
 intersection.  If the overlap contains more than a point the interval is
 rejected (should such a rejection be logged?).
 * History intervals are supposed to be a minimum size, which?
 * what else?

 4. History compression:
 Here the condition for compressing, i.e. averaging respective values:

 {{{
 1: lastEndMillis == startMillis &&
 2:          ((lastEndMillis - 1L) / intervalLengthMillis) ==
 3:          ((endMillis - 1L) / intervalLengthMillis) &&
 4:          lastMonthString.equals(monthString) &&
 5:          lastMissingValues == missingValues
 }}}

 Looking more closely at lines 2 and 3. This condition evaluates to true,
 if
 `endMillis - 1 == k *intervalLengthMillis + r_em` and
 `lastEndMillis - 1 == k * intervalLengthMillis + r_lem`
 for a suitable natural Number `k`, and `r_em`, `r_lem` natural Numbers
 strictly less than `intervalLengthMillis`.
 With line 1 and the given that interval starts should be less than their
 ends (cf. wrong date test) this implies `r_em` >= `r_lem`
 and lines 2, 3 are equivalent to `endMillis - lastEndMillis <
 intervalLengthMillis`.

 What did I overlook and what other assumptions are not reflected in the
 current code?


 5. I didn't yet check. But, other comparator implementations and histories
 should be checked, too.  All these could be the cause of inconsistencies.
 Add new trac issues for that?

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