[metrics-lib/release] Accept extra arguments in extra-info descriptors. - tor-commits

17 May 2017

commit 71885a8e694b2a37b8a868389e4ea6d1ac037b30
Author: Karsten Loesing karsten.loesing@gmx.net
Date:   Wed May 3 09:41:10 2017 +0200
Accept extra arguments in extra-info descriptors.
According to the specification it's valid to add extra arguments to
    descriptor lines unless they are tagged with "[No extra arguments]".
    This is not the case for any of the statistics-related lines in
    extra-info descriptors, so we should allow extra arguments there.
Fixes #21934.
---
 CHANGELOG.md                                       |   4 +
 .../descriptor/impl/BandwidthHistoryImpl.java      |   4 +-
 .../descriptor/impl/ExtraInfoDescriptorImpl.java   |  12 +-
 .../torproject/descriptor/impl/ParseHelper.java    |  16 ---
 .../impl/ExtraInfoDescriptorImplTest.java          | 127 +++++++++++++++++++++
 .../descriptor/impl/ServerDescriptorImplTest.java  |   6 +-
 6 files changed, 142 insertions(+), 27 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index da9cb37..a7f0aac 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,10 @@
      below crypto blocks were silently skipped.
    - Add support for six new key-value pairs added by OnionPerf.
+ * Minor changes
+   - Accept extra arguments in statistics-related extra-info
+     descriptor lines, as permitted by dir-spec.txt.
+
# Changes in version 1.6.0 - 2017-02-17
diff --git a/src/main/java/org/torproject/descriptor/impl/BandwidthHistoryImpl.java b/src/main/java/org/torproject/descriptor/impl/BandwidthHistoryImpl.java
index 8f18b62..ef29a3b 100644
--- a/src/main/java/org/torproject/descriptor/impl/BandwidthHistoryImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/BandwidthHistoryImpl.java
@@ -15,7 +15,7 @@ public class BandwidthHistoryImpl implements BandwidthHistory {
       String[] partsNoOpt) throws DescriptorParseException {
     boolean isValid = false;
     this.line = line;
-    if (partsNoOpt.length == 5 || partsNoOpt.length == 6) {
+    if (partsNoOpt.length >= 5) {
       try {
         this.historyEndMillis = ParseHelper.parseTimestampAtIndex(line,
             partsNoOpt, 1, 2);
@@ -32,7 +32,7 @@ public class BandwidthHistoryImpl implements BandwidthHistory {
               && partsNoOpt[4].equals("s)")) {
             /* There are no bandwidth values to parse. */
             isValid = true;
-          } else if (partsNoOpt.length == 6) {
+          } else if (partsNoOpt.length >= 6) {
             /* There are bandwidth values to parse. */
             values = partsNoOpt[5].split(",", -1);
           } else if (partsNoOpt[4].length() > 2) {
diff --git a/src/main/java/org/torproject/descriptor/impl/ExtraInfoDescriptorImpl.java b/src/main/java/org/torproject/descriptor/impl/ExtraInfoDescriptorImpl.java
index 136f31e..bbaae19 100644
--- a/src/main/java/org/torproject/descriptor/impl/ExtraInfoDescriptorImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/ExtraInfoDescriptorImpl.java
@@ -324,7 +324,7 @@ public abstract class ExtraInfoDescriptorImpl extends DescriptorImpl
private void parseGeoipDbDigestLine(String line, String lineNoOpt,
       String[] partsNoOpt) throws DescriptorParseException {
-    if (partsNoOpt.length != 2) {
+    if (partsNoOpt.length < 2) {
       throw new DescriptorParseException("Illegal line '" + line
           + "' in extra-info descriptor.");
     }
@@ -334,7 +334,7 @@ public abstract class ExtraInfoDescriptorImpl extends DescriptorImpl
private void parseGeoip6DbDigestLine(String line, String lineNoOpt,
       String[] partsNoOpt) throws DescriptorParseException {
-    if (partsNoOpt.length != 2) {
+    if (partsNoOpt.length < 2) {
       throw new DescriptorParseException("Illegal line '" + line
           + "' in extra-info descriptor.");
     }
@@ -344,7 +344,7 @@ public abstract class ExtraInfoDescriptorImpl extends DescriptorImpl
private void parseGeoipStartTimeLine(String line, String lineNoOpt,
       String[] partsNoOpt) throws DescriptorParseException {
-    if (partsNoOpt.length != 3) {
+    if (partsNoOpt.length < 3) {
       throw new DescriptorParseException("Illegal line '" + line
           + "' in extra-info descriptor.");
     }
@@ -369,7 +369,7 @@ public abstract class ExtraInfoDescriptorImpl extends DescriptorImpl
private long[] parseStatsEndLine(String line, String[] partsNoOpt,
       int partsNoOptExpectedLength) throws DescriptorParseException {
-    if (partsNoOpt.length != partsNoOptExpectedLength
+    if (partsNoOpt.length < partsNoOptExpectedLength
         || partsNoOpt[3].length() < 2 || !partsNoOpt[3].startsWith("(")
         || !partsNoOpt[4].equals("s)")) {
       throw new DescriptorParseException("Illegal line '" + line + "'.");
@@ -424,7 +424,7 @@ public abstract class ExtraInfoDescriptorImpl extends DescriptorImpl
   private double parseShareLine(String line, String[] partsNoOpt)
       throws DescriptorParseException {
     double share = -1.0;
-    if (partsNoOpt.length == 2 && partsNoOpt[1].length() >= 2
+    if (partsNoOpt.length >= 2 && partsNoOpt[1].length() >= 2
         && partsNoOpt[1].endsWith("%")) {
       String shareString = partsNoOpt[1];
       shareString = shareString.substring(0, shareString.length() - 1);
@@ -550,7 +550,7 @@ public abstract class ExtraInfoDescriptorImpl extends DescriptorImpl
       String lineNoOpt, String[] partsNoOpt)
       throws DescriptorParseException {
     int circuits = -1;
-    if (partsNoOpt.length == 2) {
+    if (partsNoOpt.length >= 2) {
       try {
         circuits = Integer.parseInt(partsNoOpt[1]);
       } catch (NumberFormatException e) {
diff --git a/src/main/java/org/torproject/descriptor/impl/ParseHelper.java b/src/main/java/org/torproject/descriptor/impl/ParseHelper.java
index f73a591..fa6ca75 100644
--- a/src/main/java/org/torproject/descriptor/impl/ParseHelper.java
+++ b/src/main/java/org/torproject/descriptor/impl/ParseHelper.java
@@ -341,10 +341,6 @@ public class ParseHelper {
     if (partsNoOpt.length < index) {
       throw new DescriptorParseException("Line '" + line + "' does not "
           + "contain a key-value list at index " + index + ".");
-    } else if (partsNoOpt.length > index + 1 ) {
-      throw new DescriptorParseException("Line '" + line + "' contains "
-          + "unrecognized values beyond the expected key-value list at "
-          + "index " + index + ".");
     } else if (partsNoOpt.length > index) {
       if (!commaSeparatedKeyValueListPatterns.containsKey(keyLength)) {
         String keyPattern = "[0-9a-zA-Z?<>\-_]"
@@ -390,10 +386,6 @@ public class ParseHelper {
     if (partsNoOpt.length < index) {
       throw new DescriptorParseException("Line '" + line + "' does not "
           + "contain a key-value list at index " + index + ".");
-    } else if (partsNoOpt.length > index + 1 ) {
-      throw new DescriptorParseException("Line '" + line + "' contains "
-          + "unrecognized values beyond the expected key-value list at "
-          + "index " + index + ".");
     } else if (partsNoOpt.length > index) {
       String[] listElements = partsNoOpt[index].split(",", -1);
       for (String listElement : listElements) {
@@ -428,10 +420,6 @@ public class ParseHelper {
       throw new DescriptorParseException("Line '" + line + "' does not "
           + "contain a comma-separated value list at index " + index
           + ".");
-    } else if (partsNoOpt.length > index + 1) {
-      throw new DescriptorParseException("Line '" + line + "' contains "
-          + "unrecognized values beyond the expected comma-separated "
-          + "value list at index " + index + ".");
     } else if (partsNoOpt.length > index) {
       String[] listElements = partsNoOpt[index].split(",", -1);
       result = new Integer[listElements.length];
@@ -456,10 +444,6 @@ public class ParseHelper {
       throw new DescriptorParseException("Line '" + line + "' does not "
           + "contain a comma-separated value list at index " + index
           + ".");
-    } else if (partsNoOpt.length > index + 1) {
-      throw new DescriptorParseException("Line '" + line + "' contains "
-          + "unrecognized values beyond the expected comma-separated "
-          + "value list at index " + index + ".");
     } else if (partsNoOpt.length > index) {
       String[] listElements = partsNoOpt[index].split(",", -1);
       result = new Double[listElements.length];
diff --git a/src/test/java/org/torproject/descriptor/impl/ExtraInfoDescriptorImplTest.java b/src/test/java/org/torproject/descriptor/impl/ExtraInfoDescriptorImplTest.java
index c29df8c..118ada8 100644
--- a/src/test/java/org/torproject/descriptor/impl/ExtraInfoDescriptorImplTest.java
+++ b/src/test/java/org/torproject/descriptor/impl/ExtraInfoDescriptorImplTest.java
@@ -1093,6 +1093,14 @@ public class ExtraInfoDescriptorImplTest {
         + "81281024,,60625920,67922944");
   }
+  @Test()
+  public void testDirreqWriteHistoryExtraArg() throws DescriptorParseException {
+    DescriptorBuilder.createWithDirreqWriteHistoryLine(
+        "dirreq-write-history "
+        + "2012-02-11 09:03:39 (900 s) 81281024,64996352,60625920,"
+        + "67922944 bin_size=1024");
+  }
+
   @Test(expected = DescriptorParseException.class)
   public void testDirreqReadHistoryMissingBytesEnd()
       throws DescriptorParseException {
@@ -1131,6 +1139,17 @@ public class ExtraInfoDescriptorImplTest {
   }
@Test()
+  public void testGeoipDbDigestExtraArg() throws DescriptorParseException {
+    ExtraInfoDescriptor descriptor = DescriptorBuilder
+        .createWithGeoip6DbDigestLine("geoip-db-digest "
+            + "916A3CA8B7DF61473D5AE5B21711F35F301CE9E8 "
+            + "yblgrXtEgF3glaKv5ZvHhRREUI1t1c37SxparXSmYR4Q1yiK5zg4HE8eT9ILPRW9"
+            + "3I5W/pZGQxL8Bu42dGjnAQ");
+    assertEquals("916A3CA8B7DF61473D5AE5B21711F35F301CE9E8",
+        descriptor.getGeoipDbDigest());
+  }
+
+  @Test()
   public void testGeoip6DbDigestValid() throws DescriptorParseException {
     ExtraInfoDescriptor descriptor = DescriptorBuilder
         .createWithGeoip6DbDigestLine("geoip6-db-digest "
@@ -1140,6 +1159,17 @@ public class ExtraInfoDescriptorImplTest {
   }
@Test()
+  public void testGeoip6DbDigestExtraArg() throws DescriptorParseException {
+    ExtraInfoDescriptor descriptor = DescriptorBuilder
+        .createWithGeoip6DbDigestLine("geoip6-db-digest "
+            + "916A3CA8B7DF61473D5AE5B21711F35F301CE9E8 "
+            + "yblgrXtEgF3glaKv5ZvHhRREUI1t1c37SxparXSmYR4Q1yiK5zg4HE8eT9ILPRW9"
+            + "3I5W/pZGQxL8Bu42dGjnAQ");
+    assertEquals("916A3CA8B7DF61473D5AE5B21711F35F301CE9E8",
+        descriptor.getGeoip6DbDigest());
+  }
+
+  @Test()
   public void testGeoipStatsValid() throws DescriptorParseException {
     ExtraInfoDescriptor descriptor = GeoipStatsBuilder
         .createWithDefaultLines();
@@ -1242,6 +1272,14 @@ public class ExtraInfoDescriptorImplTest {
   }
@Test()
+  public void testGeoipClientOriginsExtraArg()
+      throws DescriptorParseException {
+    GeoipStatsBuilder.createWithGeoipClientOriginsLine(
+        "geoip-client-origins de=1152,de=952,cn=896,us=712,it=504 "
+            + "ru=352 fr=208 gb=208 ir=200");
+  }
+
+  @Test()
   public void testDirreqStatsValid() throws DescriptorParseException {
     ExtraInfoDescriptor descriptor = DirreqStatsBuilder
         .createWithDefaultLines();
@@ -1279,6 +1317,13 @@ public class ExtraInfoDescriptorImplTest {
         + "2012-02-11 00:59:53 (172800 s)");
   }
+  @Test()
+  public void testDirreqStatsExtraArg()
+      throws DescriptorParseException {
+    DirreqStatsBuilder.createWithDirreqStatsEndLine("dirreq-stats-end "
+        + "2012-02-11 00:59:53 (172800 s) XXXXXXXXXXXXXXXXXXXXXXXXXXX");
+  }
+
   @Test(expected = DescriptorParseException.class)
   public void testDirreqV3IpsThreeLetterCountry()
       throws DescriptorParseException {
@@ -1287,6 +1332,13 @@ public class ExtraInfoDescriptorImplTest {
   }
@Test()
+  public void testDirreqV3IpsExtraArg()
+      throws DescriptorParseException {
+    DirreqStatsBuilder.createWithDirreqV3IpsLine("dirreq-v3-ips "
+        + "ab=12,cd=34 ef=56");
+  }
+
+  @Test()
   public void testDirreqV2IpsDigitCountry()
       throws DescriptorParseException {
     DirreqStatsBuilder.createWithDirreqV2IpsLine("dirreq-v2-ips 00=8");
@@ -1312,6 +1364,13 @@ public class ExtraInfoDescriptorImplTest {
         + "ok==10848");
   }
+  @Test()
+  public void testDirreqV3RespExtraArg()
+      throws DescriptorParseException {
+    DirreqStatsBuilder.createWithDirreqV3RespLine("dirreq-v3-resp "
+        + "ok=1084 4801=ko");
+  }
+
   @Test(expected = DescriptorParseException.class)
   public void testDirreqV2RespNull()
       throws DescriptorParseException {
@@ -1333,6 +1392,13 @@ public class ExtraInfoDescriptorImplTest {
         + "0.37");
   }
+  @Test()
+  public void testDirreqV3ShareExtraArg()
+      throws DescriptorParseException {
+    DirreqStatsBuilder.createWithDirreqV3ShareLine("dirreq-v3-share "
+        + "0.37% 123456");
+  }
+
   @Test(expected = DescriptorParseException.class)
   public void testDirreqV3DirectDlSpace()
       throws DescriptorParseException {
@@ -1362,6 +1428,13 @@ public class ExtraInfoDescriptorImplTest {
   }
@Test()
+  public void testDirreqV3TunneledDlExtraArg()
+      throws DescriptorParseException {
+    DirreqStatsBuilder.createWithDirreqV2TunneledDlLine(
+        "dirreq-v2-tunneled-dl complete=-8 incomplete=1/-8");
+  }
+
+  @Test()
   public void testEntryStatsValid() throws DescriptorParseException {
     ExtraInfoDescriptor descriptor = EntryStatsBuilder
         .createWithDefaultLines();
@@ -1443,6 +1516,13 @@ public class ExtraInfoDescriptorImplTest {
   }
@Test()
+  public void testCellCircuitsPerDecileExtraArg()
+      throws DescriptorParseException {
+    CellStatsBuilder.createWithCellCircuitsPerDecileLine(
+        "cell-circuits-per-decile 866 866 866 866 866");
+  }
+
+  @Test()
   public void testConnBiDirectValid()
       throws DescriptorParseException {
     ExtraInfoDescriptor descriptor = DescriptorBuilder
@@ -1465,6 +1545,13 @@ public class ExtraInfoDescriptorImplTest {
   }
@Test()
+  public void testConnBiDirectStatsExtraArg()
+      throws DescriptorParseException {
+    DescriptorBuilder.createWithConnBiDirectLine("conn-bi-direct "
+        + "2012-02-11 01:59:39 (86400 s) 42173,1591,1310,1744 +1");
+  }
+
+  @Test()
   public void testExitStatsValid() throws DescriptorParseException {
     ExtraInfoDescriptor descriptor = ExitStatsBuilder
         .createWithDefaultLines();
@@ -1541,6 +1628,12 @@ public class ExtraInfoDescriptorImplTest {
   }
@Test()
+  public void testExitStatsStreamsExtraArg() throws DescriptorParseException {
+    ExitStatsBuilder.createWithExitStreamsOpenedLine(
+        "exit-streams-opened 25=21474 3648");
+  }
+
+  @Test()
   public void testBridgeStatsValid() throws DescriptorParseException {
     ExtraInfoDescriptor descriptor = BridgeStatsBuilder
         .createWithDefaultLines();
@@ -1570,6 +1663,13 @@ public class ExtraInfoDescriptorImplTest {
         + "2012-02-11 01:59:39 (0 s)");
   }
+  @Test()
+  public void testBridgeStatsEndExtraArg()
+      throws DescriptorParseException {
+    BridgeStatsBuilder.createWithBridgeStatsEndLine("bridge-stats-end "
+        + "2012-02-11 01:59:39 (86400 s) 99999999999999999999999999999999");
+  }
+
   @Test(expected = DescriptorParseException.class)
   public void testBridgeIpsDouble()
       throws DescriptorParseException {
@@ -1585,6 +1685,12 @@ public class ExtraInfoDescriptorImplTest {
         0x69, 0x70, 0x73 }, false);               // "ips" (no newline)
   }
+  @Test()
+  public void testBridgeIpsExtraArg()
+      throws DescriptorParseException {
+    BridgeStatsBuilder.createWithBridgeIpsLine("bridge-ips ir=24 5");
+  }
+
   @Test(expected = DescriptorParseException.class)
   public void testBridgeIpVersionsDouble()
       throws DescriptorParseException {
@@ -1592,6 +1698,13 @@ public class ExtraInfoDescriptorImplTest {
         "bridge-ip-versions v4=24.5");
   }
+  @Test()
+  public void testBridgeIpVersionsExtraArg()
+      throws DescriptorParseException {
+    BridgeStatsBuilder.createWithBridgeIpVersionsLine(
+        "bridge-ip-versions v4=24 5");
+  }
+
   @Test(expected = DescriptorParseException.class)
   public void testBridgeIpTransportsDouble()
       throws DescriptorParseException {
@@ -1607,6 +1720,13 @@ public class ExtraInfoDescriptorImplTest {
   }
@Test()
+  public void testBridgeIpTransportsExtraArg()
+      throws DescriptorParseException {
+    BridgeStatsBuilder.createWithBridgeIpTransportsLine(
+        "bridge-ip-transports obfs2=24 5");
+  }
+
+  @Test()
   public void testHidservStatsValid() throws DescriptorParseException {
     ExtraInfoDescriptor descriptor = HidservStatsBuilder
         .createWithDefaultLines();
@@ -1670,6 +1790,13 @@ public class ExtraInfoDescriptorImplTest {
   }
@Test()
+  public void testHidservDirOnionsSeenExtraArg()
+      throws DescriptorParseException {
+    HidservStatsBuilder.createWithHidservDirOnionsSeenLine(
+        "hidserv-dir-onions-seen -3 delta_f=8 epsilon=0.30 bin_size=8 pi=3");
+  }
+
+  @Test()
   public void testRouterSignatureOpt()
       throws DescriptorParseException {
     DescriptorBuilder.createWithRouterSignatureLines("opt "
diff --git a/src/test/java/org/torproject/descriptor/impl/ServerDescriptorImplTest.java b/src/test/java/org/torproject/descriptor/impl/ServerDescriptorImplTest.java
index 80dc815..b713dd6 100644
--- a/src/test/java/org/torproject/descriptor/impl/ServerDescriptorImplTest.java
+++ b/src/test/java/org/torproject/descriptor/impl/ServerDescriptorImplTest.java
@@ -1245,11 +1245,11 @@ public class ServerDescriptorImplTest {
         "write-history 2012-01-01 03:51:44 (900 ");
   }
-  @Test(expected = DescriptorParseException.class)
-  public void testWriteHistoryTrailingNumber()
+  @Test()
+  public void testWriteHistoryExtraArg()
       throws DescriptorParseException {
     DescriptorBuilder.createWithWriteHistoryLine("write-history "
-        + "2012-01-01 03:51:44 (900 s) 4345856 1");
+        + "2012-01-01 03:51:44 (900 s) 4345856 bin_size=1024");
   }
@Test()

    