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

[Tor Bug Tracker & Wiki]

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

Comment (by karsten):

 Good catch (no pun intended) and good suggestions there.  I started
 looking through the code to find similar issues.  Here's what I found:

 {{{
 diff --git
 a/src/main/java/org/torproject/onionoo/docs/BandwidthStatus.java
 b/src/main/java/org/torproject/onionoo/docs/BandwidthStatus.java
 index 06f295b..9e4664c 100644
 --- a/src/main/java/org/torproject/onionoo/docs/BandwidthStatus.java
 +++ b/src/main/java/org/torproject/onionoo/docs/BandwidthStatus.java
 @@ -62,7 +62,14 @@ public class BandwidthStatus extends Document {
                + "bandwidth history.  Skipping.");
            break;
          }
 -        long bandwidth = Long.parseLong(parts[5]);
 +        long bandwidth;
 +        try {
 +          bandwidth = Long.parseLong(parts[5]);
 +        } catch (NumberFormatException e) {
 +          log.error("Could not parse bandwidth value while reading "
 +              + "bandwidth history.  Skipping.");
 +          break;
 +        }
          long previousEndMillis = history.headMap(startMillis).isEmpty()
              ? startMillis
              : history.get(history.headMap(startMillis).lastKey())[1];
 diff --git a/src/main/java/org/torproject/onionoo/docs/WeightsStatus.java
 b/src/main/java/org/torproject/onionoo/docs/WeightsStatus.java
 index 5d7f23e..b663fc4 100644
 --- a/src/main/java/org/torproject/onionoo/docs/WeightsStatus.java
 +++ b/src/main/java/org/torproject/onionoo/docs/WeightsStatus.java
 @@ -67,13 +67,20 @@ public class WeightsStatus extends Document {
          }
          long[] interval = new long[] { validAfterMillis,
              freshUntilMillis };
 -        double[] weights = new double[] { -1.0,
 -            Double.parseDouble(parts[5]),
 -            Double.parseDouble(parts[6]),
 -            Double.parseDouble(parts[7]),
 -            Double.parseDouble(parts[8]), -1.0, -1.0 };
 -        if (parts.length == 11) {
 -          weights[6] = Double.parseDouble(parts[10]);
 +        double[] weights;
 +        try {
 +          weights = new double[] { -1.0,
 +              Double.parseDouble(parts[5]),
 +              Double.parseDouble(parts[6]),
 +              Double.parseDouble(parts[7]),
 +              Double.parseDouble(parts[8]), -1.0, -1.0 };
 +          if (parts.length == 11) {
 +            weights[6] = Double.parseDouble(parts[10]);
 +          }
 +        } catch (NumberFormatException e) {
 +          log.error("Could not parse weights values in line '" + line
 +              + "' while reading weights status file.  Skipping.");
 +          break;
          }
          this.history.put(interval, weights);
        }
 }}}

 Of course, we'll also need to fix the parts where strings are written
 using the system locale rather than `Locale.US`.

 And then we'll need unit tests for these.  Would you be able to write one
 of those that breaks with the current code?

 However, one thing I don't necessarily agree with: Not every `try` needs a
 `catch`, if we're only using the `try` as try-with-resources.  For
 example, `NumberFormatException` is a runtime exception, and in some cases
 it's simply not possible for one of those to be thrown.  So there may be
 valid cases for using `try` without `catch`.  We might want to add a
 comment to those.

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