commit 817a29c62670468170094f891c3ffc0c7c4c448e Author: Karsten Loesing <karsten.loesing@gmx.net> Date: Mon Dec 8 11:22:32 2014 +0100 Avoid silently exiting methods. Whenever we return from a method before it ends, we should either log a warning or leave a comment why this is intended behavior. Suggested by iwakeh on #12651. --- .../torproject/onionoo/docs/ClientsHistory.java | 20 ++++++++++++++++++++ .../org/torproject/onionoo/docs/DocumentStore.java | 16 ++++++++++++++++ .../org/torproject/onionoo/docs/UptimeHistory.java | 18 ++++++++++++++++-- .../org/torproject/onionoo/server/NodeIndexer.java | 1 + .../torproject/onionoo/server/RequestHandler.java | 14 ++++++++++++++ .../torproject/onionoo/server/ResourceServlet.java | 13 +++++++++++++ .../onionoo/updater/ClientsStatusUpdater.java | 3 +++ .../onionoo/updater/DescriptorQueue.java | 2 ++ .../onionoo/updater/NodeDetailsStatusUpdater.java | 2 ++ .../onionoo/writer/ClientsDocumentWriter.java | 3 +++ .../onionoo/writer/UptimeDocumentWriter.java | 4 ++++ .../onionoo/writer/WeightsDocumentWriter.java | 3 +++ 12 files changed, 97 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/torproject/onionoo/docs/ClientsHistory.java b/src/main/java/org/torproject/onionoo/docs/ClientsHistory.java index 35a063a..effddf5 100644 --- a/src/main/java/org/torproject/onionoo/docs/ClientsHistory.java +++ b/src/main/java/org/torproject/onionoo/docs/ClientsHistory.java @@ -6,8 +6,14 @@ import java.util.Map; import java.util.SortedMap; import java.util.TreeMap; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + public class ClientsHistory implements Comparable<ClientsHistory> { + private final static Logger log = LoggerFactory.getLogger( + ClientsHistory.class); + private long startMillis; public long getStartMillis() { return this.startMillis; @@ -55,20 +61,29 @@ public class ClientsHistory implements Comparable<ClientsHistory> { String responseHistoryString) { String[] parts = responseHistoryString.split(" ", 8); if (parts.length != 8) { + log.warn("Invalid number of space-separated strings in clients " + + "history: '" + responseHistoryString + "'. Skipping"); return null; } long startMillis = DateTimeHelper.parse(parts[0] + " " + parts[1]); long endMillis = DateTimeHelper.parse(parts[2] + " " + parts[3]); if (startMillis < 0L || endMillis < 0L) { + log.warn("Invalid start or end timestamp in clients history: '" + + responseHistoryString + "'. Skipping."); return null; } if (startMillis >= endMillis) { + log.warn("Start timestamp must be smaller than end timestamp in " + + "clients history: '" + responseHistoryString + + "'. Skipping."); return null; } double totalResponses = 0.0; try { totalResponses = Double.parseDouble(parts[4]); } catch (NumberFormatException e) { + log.warn("Invalid response number format in clients history: '" + + responseHistoryString + "'. Skipping."); return null; } SortedMap<String, Double> responsesByCountry = @@ -79,6 +94,9 @@ public class ClientsHistory implements Comparable<ClientsHistory> { parseResponses(parts[7]); if (responsesByCountry == null || responsesByTransport == null || responsesByVersion == null) { + log.warn("Invalid format of responses by country, transport, or " + + "version in clients history: '" + responseHistoryString + + "'. Skipping."); return null; } return new ClientsHistory(startMillis, endMillis, totalResponses, @@ -92,12 +110,14 @@ public class ClientsHistory implements Comparable<ClientsHistory> { for (String pair : responsesString.split(",")) { String[] keyValue = pair.split("="); if (keyValue.length != 2) { + /* Logged by caller */ return null; } double value = 0.0; try { value = Double.parseDouble(keyValue[1]); } catch (NumberFormatException e) { + /* Logged by caller */ return null; } responses.put(keyValue[0], value); diff --git a/src/main/java/org/torproject/onionoo/docs/DocumentStore.java b/src/main/java/org/torproject/onionoo/docs/DocumentStore.java index 4b7bca5..a880e53 100644 --- a/src/main/java/org/torproject/onionoo/docs/DocumentStore.java +++ b/src/main/java/org/torproject/onionoo/docs/DocumentStore.java @@ -393,6 +393,9 @@ public class DocumentStore { DetailsDocument detailsDocument = this.retrieveDocumentFile( DetailsDocument.class, true, fingerprint); if (detailsDocument == null) { + /* There is no details document available that we could serve as + * basis for generating a summary document on-the-fly. Nothing to + * worry about. */ return null; } boolean isRelay = detailsDocument.getHashedFingerprint() == null; @@ -402,6 +405,10 @@ public class DocumentStore { String countryCode = null, aSNumber = null, contact = null; for (String orAddressAndPort : detailsDocument.getOrAddresses()) { if (!orAddressAndPort.contains(":")) { + log.warn("Attempt to create summary document from details " + + "document for fingerprint " + fingerprint + " failed " + + "because of invalid OR address/port: '" + orAddressAndPort + + "'. Not returning a summary document in this case."); return null; } String orAddress = orAddressAndPort.substring(0, @@ -431,6 +438,7 @@ public class DocumentStore { Class<T> documentType, boolean parse, String fingerprint) { File documentFile = this.getDocumentFile(documentType, fingerprint); if (documentFile == null || !documentFile.exists()) { + /* Document file does not exist. That's okay. */ return null; } else if (documentFile.isDirectory()) { log.error("Could not read file '" @@ -451,6 +459,7 @@ public class DocumentStore { bis.close(); byte[] allData = baos.toByteArray(); if (allData.length == 0) { + /* Document file is empty. */ return null; } documentString = new String(allData, "US-ASCII"); @@ -598,6 +607,9 @@ public class DocumentStore { File documentFile = null; if (fingerprint == null && !documentType.equals(UpdateStatus.class) && !documentType.equals(UptimeStatus.class)) { + log.warn("Attempted to locate a document file of type " + + documentType.getName() + " without providing a fingerprint. " + + "Such a file does not exist."); return null; } File directory = null; @@ -685,6 +697,8 @@ public class DocumentStore { private void writeNodeStatuses() { File directory = this.statusDir; if (directory == null) { + log.error("Unable to write node statuses without knowing the " + + "'status' directory to write to!"); return; } File summaryFile = new File(directory, "summary"); @@ -766,6 +780,8 @@ public class DocumentStore { private void writeUpdateStatus() { if (this.outDir == null) { + log.error("Unable to write update status file without knowing the " + + "'out' directory to write to!"); return; } UpdateStatus updateStatus = new UpdateStatus(); diff --git a/src/main/java/org/torproject/onionoo/docs/UptimeHistory.java b/src/main/java/org/torproject/onionoo/docs/UptimeHistory.java index 0d27242..b686c7e 100644 --- a/src/main/java/org/torproject/onionoo/docs/UptimeHistory.java +++ b/src/main/java/org/torproject/onionoo/docs/UptimeHistory.java @@ -1,8 +1,13 @@ package org.torproject.onionoo.docs; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; -public class UptimeHistory - implements Comparable<UptimeHistory> { + +public class UptimeHistory implements Comparable<UptimeHistory> { + + private final static Logger log = LoggerFactory.getLogger( + UptimeHistory.class); private boolean relay; public boolean isRelay() { @@ -29,23 +34,32 @@ public class UptimeHistory public static UptimeHistory fromString(String uptimeHistoryString) { String[] parts = uptimeHistoryString.split(" ", 3); if (parts.length != 3) { + log.warn("Invalid number of space-separated strings in uptime " + + "history: '" + uptimeHistoryString + "'. Skipping"); return null; } boolean relay = false; if (parts[0].equals("r")) { relay = true; } else if (!parts[0].equals("b")) { + log.warn("Invalid node type in uptime history: '" + + uptimeHistoryString + "'. Supported types are 'r' and 'b'. " + + "Skipping."); return null; } long startMillis = DateTimeHelper.parse(parts[1], DateTimeHelper.DATEHOUR_NOSPACE_FORMAT); if (DateTimeHelper.NO_TIME_AVAILABLE == startMillis) { + log.warn("Invalid start timestamp in uptime history: '" + + uptimeHistoryString + "'. Skipping."); return null; } int uptimeHours = -1; try { uptimeHours = Integer.parseInt(parts[2]); } catch (NumberFormatException e) { + log.warn("Invalid number format in uptime history: '" + + uptimeHistoryString + "'. Skipping."); return null; } return new UptimeHistory(relay, startMillis, uptimeHours); diff --git a/src/main/java/org/torproject/onionoo/server/NodeIndexer.java b/src/main/java/org/torproject/onionoo/server/NodeIndexer.java index f8870db..1a67cc5 100644 --- a/src/main/java/org/torproject/onionoo/server/NodeIndexer.java +++ b/src/main/java/org/torproject/onionoo/server/NodeIndexer.java @@ -105,6 +105,7 @@ public class NodeIndexer implements ServletContextListener, Runnable { } synchronized (this) { if (updateStatusMillis <= this.lastIndexed) { + /* Index on disk is no more recent than the one in memory. */ return; } } diff --git a/src/main/java/org/torproject/onionoo/server/RequestHandler.java b/src/main/java/org/torproject/onionoo/server/RequestHandler.java index 28b5531..2d8dd58 100644 --- a/src/main/java/org/torproject/onionoo/server/RequestHandler.java +++ b/src/main/java/org/torproject/onionoo/server/RequestHandler.java @@ -154,6 +154,7 @@ public class RequestHandler { private void filterByType() { if (this.type == null) { + /* Not filtering by type. */ return; } else if (this.type.equals("relay")) { this.filteredBridges.clear(); @@ -164,6 +165,7 @@ public class RequestHandler { private void filterByRunning() { if (this.running == null) { + /* Not filtering by running or not. */ return; } boolean runningRequested = this.running.equals("true"); @@ -191,6 +193,7 @@ public class RequestHandler { private void filterBySearchTerms() { if (this.search == null) { + /* Not filtering by search terms. */ return; } for (String searchTerm : this.search) { @@ -276,6 +279,7 @@ public class RequestHandler { private void filterByLookup() { if (this.lookup == null) { + /* Not filtering by looking up relay or bridge. */ return; } String fingerprint = this.lookup; @@ -293,6 +297,7 @@ public class RequestHandler { private void filterByFingerprint() { if (this.fingerprint == null) { + /* Not filtering by fingerprint. */ return; } this.filteredRelays.clear(); @@ -311,6 +316,7 @@ public class RequestHandler { private void filterByCountryCode() { if (this.country == null) { + /* Not filtering by country code. */ return; } String countryCode = this.country.toLowerCase(); @@ -335,6 +341,7 @@ public class RequestHandler { private void filterByASNumber() { if (this.as == null) { + /* Not filtering by AS number. */ return; } String aSNumber = this.as.toUpperCase(); @@ -361,6 +368,7 @@ public class RequestHandler { private void filterByFlag() { if (this.flag == null) { + /* Not filtering by relay flag. */ return; } String flag = this.flag.toLowerCase(); @@ -398,6 +406,7 @@ public class RequestHandler { private void filterNodesByFirstSeenDays() { if (this.firstSeenDays == null) { + /* Not filtering by first-seen days. */ return; } filterNodesByDays(this.filteredRelays, @@ -408,6 +417,7 @@ public class RequestHandler { private void filterNodesByLastSeenDays() { if (this.lastSeenDays == null) { + /* Not filtering by last-seen days. */ return; } filterNodesByDays(this.filteredRelays, @@ -436,6 +446,7 @@ public class RequestHandler { private void filterByContact() { if (this.contact == null) { + /* Not filtering by contact information. */ return; } Set<String> removeRelays = new HashSet<String>(); @@ -458,6 +469,7 @@ public class RequestHandler { private void filterByFamily() { if (this.family == null) { + /* Not filtering by relay family. */ return; } Set<String> removeRelays = new HashSet<String>( @@ -506,6 +518,7 @@ public class RequestHandler { private void offset() { if (this.offset == null) { + /* Not skipping first results. */ return; } int offsetValue = Integer.parseInt(this.offset); @@ -522,6 +535,7 @@ public class RequestHandler { private void limit() { if (this.limit == null) { + /* Not limiting number of results. */ return; } int limitValue = Integer.parseInt(this.limit); diff --git a/src/main/java/org/torproject/onionoo/server/ResourceServlet.java b/src/main/java/org/torproject/onionoo/server/ResourceServlet.java index f6f060a..6182704 100644 --- a/src/main/java/org/torproject/onionoo/server/ResourceServlet.java +++ b/src/main/java/org/torproject/onionoo/server/ResourceServlet.java @@ -372,6 +372,7 @@ public class ResourceServlet extends HttpServlet { Matcher searchQueryStringMatcher = searchQueryStringPattern.matcher( queryString); if (!searchQueryStringMatcher.matches()) { + /* Search query contains illegal character(s). */ return null; } String parameter = searchQueryStringMatcher.group(1); @@ -383,6 +384,7 @@ public class ResourceServlet extends HttpServlet { } for (String searchParameter : searchParameters) { if (!searchParameterPattern.matcher(searchParameter).matches()) { + /* Illegal search term. */ return null; } } @@ -393,9 +395,11 @@ public class ResourceServlet extends HttpServlet { Pattern.compile("^[0-9a-zA-Z]{1,40}$"); private String parseFingerprintParameter(String parameter) { if (!fingerprintParameterPattern.matcher(parameter).matches()) { + /* Fingerprint contains non-hex character(s). */ return null; } if (parameter.length() != 40) { + /* Only full fingerprints are accepted. */ return null; } return parameter; @@ -405,6 +409,8 @@ public class ResourceServlet extends HttpServlet { Pattern.compile("^[0-9a-zA-Z]{2}$"); private String parseCountryCodeParameter(String parameter) { if (!countryCodeParameterPattern.matcher(parameter).matches()) { + /* Country code contains illegal characters or is shorter/longer + * than 2 characters. */ return null; } return parameter; @@ -414,6 +420,7 @@ public class ResourceServlet extends HttpServlet { Pattern.compile("^[asAS]{0,2}[0-9]{1,10}$"); private String parseASNumberParameter(String parameter) { if (!aSNumberParameterPattern.matcher(parameter).matches()) { + /* AS number contains illegal character(s). */ return null; } return parameter; @@ -423,6 +430,7 @@ public class ResourceServlet extends HttpServlet { Pattern.compile("^[a-zA-Z0-9]{1,20}$"); private String parseFlagParameter(String parameter) { if (!flagPattern.matcher(parameter).matches()) { + /* Flag contains illegal character(s). */ return null; } return parameter; @@ -431,6 +439,7 @@ public class ResourceServlet extends HttpServlet { private static Pattern daysPattern = Pattern.compile("^[0-9-]{1,10}$"); private int[] parseDaysParameter(String parameter) { if (!daysPattern.matcher(parameter).matches()) { + /* Days contain illegal character(s). */ return null; } int x = 0, y = Integer.MAX_VALUE; @@ -448,9 +457,11 @@ public class ResourceServlet extends HttpServlet { } } } catch (NumberFormatException e) { + /* Invalid format. */ return null; } if (x > y) { + /* Second number or days must exceed first number. */ return null; } return new int[] { x, y }; @@ -459,6 +470,7 @@ public class ResourceServlet extends HttpServlet { private String[] parseContactParameter(String parameter) { for (char c : parameter.toCharArray()) { if (c < 32 || c >= 127) { + /* Only accept printable ASCII. */ return null; } } @@ -469,6 +481,7 @@ public class ResourceServlet extends HttpServlet { Pattern.compile("^[0-9a-zA-Z_,]*$"); private String[] parseFieldsParameter(String parameter) { if (!fieldsParameterPattern.matcher(parameter).matches()) { + /* Fields contain illegal character(s). */ return null; } return parameter.split(","); diff --git a/src/main/java/org/torproject/onionoo/updater/ClientsStatusUpdater.java b/src/main/java/org/torproject/onionoo/updater/ClientsStatusUpdater.java index f3f2e0f..72f5a92 100644 --- a/src/main/java/org/torproject/onionoo/updater/ClientsStatusUpdater.java +++ b/src/main/java/org/torproject/onionoo/updater/ClientsStatusUpdater.java @@ -73,10 +73,13 @@ public class ClientsStatusUpdater implements DescriptorListener, if (dirreqStatsEndMillis < 0L || dirreqStatsIntervalLengthMillis != DateTimeHelper.ONE_DAY || responses == null || !responses.containsKey("ok")) { + /* No directory request responses in the descriptor that we would + * include in a clients document. */ return; } double okResponses = (double) (responses.get("ok") - 4); if (okResponses < 0.0) { + /* Response numbers are not supposed to be negative. Skipping. */ return; } String hashedFingerprint = descriptor.getFingerprint().toUpperCase(); diff --git a/src/main/java/org/torproject/onionoo/updater/DescriptorQueue.java b/src/main/java/org/torproject/onionoo/updater/DescriptorQueue.java index 8ba4260..93a291c 100644 --- a/src/main/java/org/torproject/onionoo/updater/DescriptorQueue.java +++ b/src/main/java/org/torproject/onionoo/updater/DescriptorQueue.java @@ -168,6 +168,8 @@ class DescriptorQueue { public void writeHistoryFile() { if (this.historyFile == null) { + log.error("Unable to write history file, possibly because of an " + + "unknown descriptor type. Skipping."); return; } SortedMap<String, Long> excludedAndParsedFiles = diff --git a/src/main/java/org/torproject/onionoo/updater/NodeDetailsStatusUpdater.java b/src/main/java/org/torproject/onionoo/updater/NodeDetailsStatusUpdater.java index 935aa4e..ef87233 100644 --- a/src/main/java/org/torproject/onionoo/updater/NodeDetailsStatusUpdater.java +++ b/src/main/java/org/torproject/onionoo/updater/NodeDetailsStatusUpdater.java @@ -150,6 +150,7 @@ public class NodeDetailsStatusUpdater implements DescriptorListener, } else if (detailsStatus.getDescPublished() != null && detailsStatus.getDescPublished() >= descriptor.getPublishedMillis()) { + /* Already parsed more recent server descriptor from this relay. */ return; } long lastRestartedMillis = descriptor.getPublishedMillis() @@ -276,6 +277,7 @@ public class NodeDetailsStatusUpdater implements DescriptorListener, detailsStatus = new DetailsStatus(); } else if (descriptor.getPublishedMillis() <= detailsStatus.getDescPublished()) { + /* Already parsed more recent server descriptor from this bridge. */ return; } long lastRestartedMillis = descriptor.getPublishedMillis() diff --git a/src/main/java/org/torproject/onionoo/writer/ClientsDocumentWriter.java b/src/main/java/org/torproject/onionoo/writer/ClientsDocumentWriter.java index 885494c..84d5a30 100644 --- a/src/main/java/org/torproject/onionoo/writer/ClientsDocumentWriter.java +++ b/src/main/java/org/torproject/onionoo/writer/ClientsDocumentWriter.java @@ -198,6 +198,7 @@ public class ClientsDocumentWriter implements DocumentWriter { } } if (firstNonNullIndex < 0) { + /* Not a single non-negative value in the data points. */ return null; } long firstDataPointMillis = (((this.now - graphInterval) @@ -273,6 +274,8 @@ public class ClientsDocumentWriter implements DocumentWriter { if (foundTwoAdjacentDataPoints) { return graphHistory; } else { + /* There are no two adjacent values in the data points that are + * required to draw a line graph. */ return null; } } diff --git a/src/main/java/org/torproject/onionoo/writer/UptimeDocumentWriter.java b/src/main/java/org/torproject/onionoo/writer/UptimeDocumentWriter.java index 093bc98..c5f71a1 100644 --- a/src/main/java/org/torproject/onionoo/writer/UptimeDocumentWriter.java +++ b/src/main/java/org/torproject/onionoo/writer/UptimeDocumentWriter.java @@ -39,6 +39,7 @@ public class UptimeDocumentWriter implements DocumentWriter { UptimeStatus uptimeStatus = this.documentStore.retrieve( UptimeStatus.class, true); if (uptimeStatus == null) { + /* No global uptime information available. */ return; } UpdateStatus updateStatus = this.documentStore.retrieve( @@ -233,6 +234,7 @@ public class UptimeDocumentWriter implements DocumentWriter { } } if (firstNonNullIndex < 0) { + /* Not a single non-negative value in the data points. */ return null; } long firstDataPointMillis = (((this.now - graphInterval) @@ -273,6 +275,8 @@ public class UptimeDocumentWriter implements DocumentWriter { if (foundTwoAdjacentDataPoints) { return graphHistory; } else { + /* There are no two adjacent values in the data points that are + * required to draw a line graph. */ return null; } } diff --git a/src/main/java/org/torproject/onionoo/writer/WeightsDocumentWriter.java b/src/main/java/org/torproject/onionoo/writer/WeightsDocumentWriter.java index 7d52a47..62ff533 100644 --- a/src/main/java/org/torproject/onionoo/writer/WeightsDocumentWriter.java +++ b/src/main/java/org/torproject/onionoo/writer/WeightsDocumentWriter.java @@ -157,6 +157,7 @@ public class WeightsDocumentWriter implements DocumentWriter { } } if (firstNonNullIndex < 0) { + /* Not a single non-negative value in the data points. */ return null; } long firstDataPointMillis = (((this.now - graphInterval) @@ -199,6 +200,8 @@ public class WeightsDocumentWriter implements DocumentWriter { if (foundTwoAdjacentDataPoints) { return graphHistory; } else { + /* There are no two adjacent values in the data points that are + * required to draw a line graph. */ return null; } }