[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #19259 [Metrics/Onionoo]: uncaught NFE
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs