#24012 closed defect (fixed)

Fix NullPointerExceptions from accessing descriptor parts that may be null

Reported by: karsten Owned by: metrics-team
Priority: High Milestone:
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

There's a relay running an alternate Tor version that produces descriptors without "uptime" line, and the directory authorities don't include a "v" line for that relay, likely because its platform string does not include the magic word "Tor". Maybe there are similar issues.

I'm working on a hotfix that I'll post here shortly and that needs review and more testing.

Child Tickets

Change History (6)

comment:1 Changed 20 months ago by karsten

Status: newneeds_review

Here's the server descriptor:

@type server-descriptor 1.0
router pearl000 35.203.138.1 9001 0 0
signing-key
-----BEGIN RSA PUBLIC KEY-----
MIGJAoGBAKzTaN4tZGv1kiQWBzeuOk+ovr2LtIURlaVC38j6j/fQuYfuAZX/XvV1
fQr9EVh+T617dh+frt2D0QDuzLUvP3hpgVozW94w+Ib85pUCne03f4rj3QYu5Qtg
GvzShslZI6vgyy0g2jAOGa4jxT/UYAcKE5dQo8CBKA6Qb0P5Joc1AgMBAAE=
-----END RSA PUBLIC KEY-----
fingerprint 6832 5B4B 1E17 7374 B84D 372F 0304 6351 BEE7 FF6A
onion-key
-----BEGIN RSA PUBLIC KEY-----
MIGJAoGBANN/gLTe05kWKPSEyYJeknuxQst+cVsVmZrZgIYNXuhPn+3XnWhEc10r
ICa82FkB7hBH6REuW0ugGDc2QLwENmDiaBiFW1LDujEFeVlV8o0VSDwrL3VCPsPL
zC4/zHqR4DmLFXp5V238MKj85Pud04g65piZCIAsy6hiGMDCoGdtAgMBAAE=
-----END RSA PUBLIC KEY-----
ntor-onion-key ATSN2Q9KwCeRu35agh/ChjX8MsgM/FGFRDUX6o9Sbmk
platform Pearl 0fd5756 on Linux
contact pearl@m15n.org https://github.com/mmcloughlin/pearl
bandwidth 153600 307200 153600
published 2017-10-25 18:00:28
reject *:*
proto Link=4 LinkAuth=1 Relay=2
router-signature
-----BEGIN SIGNATURE-----
OyY0vQc5n2RYdkrXqfn09HoACJBx7GrBHZMnmNtlX5nJIL9N4eyyPvmxhmuC+A94
dDE0u/6w3nCABikFFLHcKaBAdmYBdxrzk3imfVjzYZazHWWr/se8HxK1jibP186A
8K8bdtMih127CGv3mn+g17uXFTbbuylM7r1xf8NpqRs=
-----END SIGNATURE-----

Here's the consensus entry:

r pearl000 aDJbSx4Xc3S4TTcvAwRjUb7n/2o 97QY9wS38Dw33qiyzDiGOi4Mu/E 2017-10-25 04:09:33 35.203.138.1 9001 0
s Running Valid
pr Link=4 LinkAuth=1 Relay=2
w Bandwidth=20 Unmeasured=1
p reject 1-65535

And my task-24012 branch contains what I came up with so far. Please review, test, improve!

comment:2 Changed 20 months ago by iwakeh

Please review my branch task-24012. It contains another commit for making Onionoo more stable when history computation fails and another commit for preventing NPEs when bridges don't have an uptime value.

(There are still tests missing here.)

comment:3 Changed 20 months ago by iwakeh

I tested a single run against the current collector recent directory, which should contain all problematic descriptors encountered during the last few days.

All ant tests and checks pass.

Unless anything else surfaces, this seems ready for merge and hotfix.

comment:4 Changed 20 months ago by karsten

Thanks for reviewing and testing!

Regarding your commit b7bd616, I'm afraid that it's flawed: we cannot create a FileWriter for this.historyFile and create its parent directory afterwards by calling this.historyFile.getParentFile().mkdirs(). I'd say let's keep this change out of the patch release and focus on the bugs that we're aware of. We can still make things more robust when we have more time.

Your commit 3a72ead is a good catch! I tweaked it a bit by renaming lastRestartedMillis() to calculateLastRestartedMillis(). Save the verbs!

I pushed two branches to my public repository: task-24012 contains your second commit rebased onto my earlier branch plus two squash commits, and next contains the same changes squashed into one commit plus preparations for the patch release.

comment:5 in reply to:  4 Changed 20 months ago by iwakeh

Replying to karsten:

Thanks for reviewing and testing!

Regarding your commit b7bd616, I'm afraid that it's flawed: we cannot create a FileWriter for this.historyFile and create its parent directory afterwards by calling this.historyFile.getParentFile().mkdirs(). I'd say let's keep this change out of the patch release and focus on the bugs that we're aware of. We can still make things more robust when we have more time.

Oh, I wanted to save typing, grr. Good that you noticed, but I'd really like to have the enclosing try-catch(Exception) for the entire contents of that method. That shouldn't hurt to be there.

Your commit 3a72ead is a good catch! I tweaked it a bit by renaming lastRestartedMillis() to calculateLastRestartedMillis(). Save the verbs!

ok.

I pushed two branches to my public repository: task-24012 contains your second commit rebased onto my earlier branch plus two squash commits, and next contains the same changes squashed into one commit plus preparations for the patch release.

comment:6 Changed 20 months ago by iwakeh

Resolution: fixed
Status: needs_reviewclosed

Released and working (cf. announcement).
Closing.

Thanks!

Note: See TracTickets for help on using tickets.