Merge pull request #12882 from vespa-engine/hakonhall/validate-deserialization-of-flag-data

Validate deserialization of flag data

8 files changed, 177 insertions, 8 deletions

diff --git a/vespajlib/abi-spec.json b/vespajlib/abi-spec.json
index 5cddc82d05a..d9467a41f78 100644
--- a/vespajlib/abi-spec.json
+++ b/vespajlib/abi-spec.json
@@ -2863,7 +2863,8 @@
     ],
     "methods": [
       "public static java.lang.String encode(java.util.Map)",
-      "public static java.lang.String escape(java.lang.String)"
+      "public static java.lang.String escape(java.lang.String)",
+      "public static boolean equals(java.lang.String, java.lang.String)"
     ],
     "fields": []
   },
diff --git a/vespajlib/src/main/java/com/yahoo/slime/JsonDecoder.java b/vespajlib/src/main/java/com/yahoo/slime/JsonDecoder.java
index f199fefd185..f677ae23a45 100644
--- a/vespajlib/src/main/java/com/yahoo/slime/JsonDecoder.java
+++ b/vespajlib/src/main/java/com/yahoo/slime/JsonDecoder.java
@@ -3,10 +3,8 @@ package com.yahoo.slime;
 
 import com.yahoo.text.Text;
 import com.yahoo.text.Utf8;
-import org.w3c.dom.CharacterData;
 
 import java.io.ByteArrayOutputStream;
-import java.nio.charset.StandardCharsets;
 
 /**
  * A port of the C++ json decoder intended to be fast.
@@ -47,6 +45,17 @@ public class JsonDecoder {
         return slime;
     }
 
+    /** Decode bytes as a UTF-8 JSON into Slime, or throw {@link JsonParseException} on invalid JSON. */
+    public Slime decodeOrThrow(Slime slime, byte[] bytes) {
+        in = new BufferedInput(bytes);
+        next();
+        decodeValue(slimeInserter.adjust(slime));
+        if (in.failed()) {
+            throw new JsonParseException(in);
+        }
+        return slime;
+    }
+
     private void decodeValue(Inserter inserter) {
         skipWhiteSpace();
         switch (c) {
diff --git a/vespajlib/src/main/java/com/yahoo/slime/JsonParseException.java b/vespajlib/src/main/java/com/yahoo/slime/JsonParseException.java
new file mode 100644
index 00000000000..6c42f7d38c1
--- /dev/null
+++ b/vespajlib/src/main/java/com/yahoo/slime/JsonParseException.java
@@ -0,0 +1,23 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.slime;
+
+/**
+ * @author hakonhall
+ */
+public class JsonParseException extends RuntimeException {
+
+    private static final long serialVersionUID = 1586949558L;
+
+    private final BufferedInput input;
+
+    JsonParseException(BufferedInput input) {
+        super(input.getErrorMessage());
+        this.input = input;
+    }
+
+    public byte[] getOffendingBytes() {
+        // potentially expensive array copy
+        return input.getOffending();
+    }
+
+}
diff --git a/vespajlib/src/main/java/com/yahoo/slime/Slime.java b/vespajlib/src/main/java/com/yahoo/slime/Slime.java
index 8357e3035c0..83934e0c206 100644
--- a/vespajlib/src/main/java/com/yahoo/slime/Slime.java
+++ b/vespajlib/src/main/java/com/yahoo/slime/Slime.java
@@ -159,11 +159,10 @@ public final class Slime {
     }
 
     /**
-     * Tests whether this is equal to Inspector.
+     * Tests whether the two Inspectors are equal.
      *
-     * Since equality of two Inspectors is subtle, {@link Object#equals(Object)} is not used.
+     * <p>Since equality of two Inspectors is subtle, {@link Object#equals(Object)} is not used.</p>
      *
-     * @param that inspector.
      * @return true if they are equal.
      */
     public boolean equalTo(Slime that) {
diff --git a/vespajlib/src/main/java/com/yahoo/slime/SlimeUtils.java b/vespajlib/src/main/java/com/yahoo/slime/SlimeUtils.java
index 4d0b3a8ff2e..2ed7331a60c 100644
--- a/vespajlib/src/main/java/com/yahoo/slime/SlimeUtils.java
+++ b/vespajlib/src/main/java/com/yahoo/slime/SlimeUtils.java
@@ -104,6 +104,17 @@ public class SlimeUtils {
         return jsonToSlime(json.getBytes(StandardCharsets.UTF_8));
     }
 
+    /** Throws {@link JsonParseException} on invalid JSON. */
+    public static Slime jsonToSlimeOrThrow(String json) {
+        return jsonToSlimeOrThrow(json.getBytes(StandardCharsets.UTF_8));
+    }
+
+    public static Slime jsonToSlimeOrThrow(byte[] json) {
+        Slime slime = new Slime();
+        new JsonDecoder().decodeOrThrow(slime, json);
+        return slime;
+    }
+
     public static Optional<String> optionalString(Inspector inspector) {
         return Optional.of(inspector.asString()).filter(s -> !s.isEmpty());
     }
diff --git a/vespajlib/src/main/java/com/yahoo/text/JSON.java b/vespajlib/src/main/java/com/yahoo/text/JSON.java
index cfff16c9aba..11e5be9328e 100644
--- a/vespajlib/src/main/java/com/yahoo/text/JSON.java
+++ b/vespajlib/src/main/java/com/yahoo/text/JSON.java
@@ -1,10 +1,13 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.text;
 
+import com.yahoo.slime.Slime;
+import com.yahoo.slime.SlimeUtils;
+
 import java.util.Map;
 
 /**
- * Static methods for working with the map textual format which is parsed by {@link MapParser}
+ * Static methods for working with JSON.
  *
  * @author bratseth
  */
@@ -56,4 +59,19 @@ public final class JSON {
         return b != null ? b.toString() : s;
     }
 
+    /**
+     * Test whether two JSON strings are equal, e.g. the order of fields in an object is irrelevant.
+     *
+     * <p>When comparing two numbers of the two JSON strings, the result is only guaranteed to be
+     * correct if (a) both are integers (without fraction and exponent) and each fits in a long, or
+     * (b) both are non-integers, are syntactically identical, and fits in a double.</p>
+     *
+     * @throws RuntimeException on invalid JSON
+     */
+    public static boolean equals(String left, String right) {
+        Slime leftSlime = SlimeUtils.jsonToSlimeOrThrow(left);
+        Slime rightSlime = SlimeUtils.jsonToSlimeOrThrow(right);
+        return leftSlime.equalTo(rightSlime);
+    }
+
 }
diff --git a/vespajlib/src/test/java/com/yahoo/slime/SlimeUtilsTest.java b/vespajlib/src/test/java/com/yahoo/slime/SlimeUtilsTest.java
index 8b13ee74aed..237b1575bfb 100644
--- a/vespajlib/src/test/java/com/yahoo/slime/SlimeUtilsTest.java
+++ b/vespajlib/src/test/java/com/yahoo/slime/SlimeUtilsTest.java
@@ -7,8 +7,10 @@ import org.junit.Test;
 import java.io.IOException;
 
 import static org.hamcrest.core.Is.is;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 /**
  * @author Ulf Lilleengen
@@ -78,4 +80,21 @@ public class SlimeUtilsTest {
         assertTrue(slime.get().field("bar").valid());
     }
 
+    @Test
+    public void test_json_to_slime_or_throw() {
+        Slime slime = SlimeUtils.jsonToSlimeOrThrow("{\"foo\":\"foobie\",\"bar\":{}}");
+        assertThat(slime.get().field("foo").asString(), is("foobie"));
+        assertTrue(slime.get().field("bar").valid());
+    }
+
+    @Test
+    public void test_invalid_json() {
+        try {
+            SlimeUtils.jsonToSlimeOrThrow("foo");
+            fail();
+        } catch (RuntimeException e) {
+            assertEquals("Unexpected character 'o'", e.getMessage());
+        }
+    }
+
 }
diff --git a/vespajlib/src/test/java/com/yahoo/text/JSONTest.java b/vespajlib/src/test/java/com/yahoo/text/JSONTest.java
index 22174761571..fbd0f9d0403 100644
--- a/vespajlib/src/test/java/com/yahoo/text/JSONTest.java
+++ b/vespajlib/src/test/java/com/yahoo/text/JSONTest.java
@@ -2,11 +2,16 @@
 package com.yahoo.text;
 
 import org.junit.Test;
-import static org.junit.Assert.assertEquals;
 
 import java.util.LinkedHashMap;
 import java.util.Map;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
 /**
  * @author bratseth
  */
@@ -22,4 +27,88 @@ public class JSONTest {
         assertEquals("{\"a \\\"key\\\"\":3,\"key2\":\"value\",\"key3\":3.3}", JSON.encode(map));
     }
 
+    @Test
+    public void testEquals() {
+        assertTrue(JSON.equals("{}", "{}"));
+
+        // Whitespace is irrelevant
+        assertTrue(JSON.equals("{}", "\n{ }"));
+
+        // Order of fields in object is irrelevant
+        assertTrue(JSON.equals("{\"a\":0, \"c\":1}", "{\"c\":1, \"a\":0}"));
+
+        // Object equality is not using subset
+        assertFalse(JSON.equals("{\"a\":0}", "{\"a\":0, \"b\":0}"));
+        assertFalse(JSON.equals("{\"a\":0, \"b\":0}", "{\"a\":0}"));
+
+        // Order of elements of array is significant
+        assertFalse(JSON.equals("[\"a\",\"b\"]", "[\"b\",\"a\"]"));
+
+        // Verify null-valued fields are not ignored
+        assertFalse(JSON.equals("{\"a\":null}", "{}"));
+
+        // Current impl uses BigInteger if integer doesn't fit in a long.
+        assertEquals(9223372036854775807L, Long.MAX_VALUE);
+        assertTrue(JSON.equals("{\"a\": 9223372036854775807}", "{\"a\": 9223372036854775807}"));
+
+        // double 1.0 and int 1 are different
+        assertTrue(JSON.equals("{\"a\": 1}", "{\"a\": 1}"));
+        assertTrue(JSON.equals("{\"a\": 1.0}", "{\"a\": 1.0}"));
+        assertFalse(JSON.equals("{\"a\": 1.0}", "{\"a\": 1}"));
+
+        // Double-precision on numbers. Constant from Math.E.
+        assertTrue(JSON.equals("{\"e\": 2.71828182845904}", "{\"e\": 2.71828182845904}"));
+
+        // Double.MAX_VALUE is 1.7976931348623157e+308
+        assertTrue(JSON.equals("{\"e\": 1.7976931348623156e+308}", "{\"e\": 1.7976931348623156e+308}"));
+
+        // Justification of above float values
+        double e1 = 2.7182818284590452354;
+        double e2 = 2.718281828459045;
+        double e3 = 2.71828182845904;
+        assertEquals(e1, Math.E, -1);
+        assertEquals(e1, e2, -1);
+        assertNotEquals(e1, e3, -1);
+
+        // Invalid JSON throws RuntimeException
+        assertRuntimeException(() -> JSON.equals("", "{}"));
+        assertRuntimeException(() -> JSON.equals("{}", ""));
+        assertRuntimeException(() -> JSON.equals("{", "{}"));
+        assertRuntimeException(() -> JSON.equals("{}", "{"));
+    }
+
+    @Test
+    public void implementationSpecificEqualsBehavior() {
+        // Exception thrown if outside a long
+        assertTrue(                  JSON.equals("{\"a\": 9223372036854775807}", "{\"a\": 9223372036854775807}"));
+        assertRuntimeException(() -> JSON.equals("{\"a\": 9223372036854775808}", "{\"a\": 9223372036854775808}"));
+
+        // Infinity if floating point number outside of double, and hence equal
+        assertTrue(JSON.equals("{\"a\": 2.7976931348623158e+308}", "{\"a\": 2.7976931348623158e+308}"));
+
+        // Ignores extraneous precision
+        assertTrue(JSON.equals( "{\"e\": 2.7182818284590452354}",
+                                 "{\"e\": 2.7182818284590452354}"));
+        assertTrue(JSON.equals( "{\"e\": 2.7182818284590452354}",
+                                 "{\"e\": 2.7182818284590452355}"));
+        assertFalse(JSON.equals("{\"e\": 2.7182818284590452354}",
+                                 "{\"e\": 2.71828182845904}"));
+
+        // Comparing equal but syntactically different numbers
+        assertFalse(JSON.equals("{\"a\": 1.0}", "{\"a\":1}"));
+        assertTrue(JSON.equals("{\"a\": 1.0}", "{\"a\":1.00}"));
+        assertTrue(JSON.equals("{\"a\": 1.0}", "{\"a\":1.0000000000000000000000000000}"));
+        assertTrue(JSON.equals("{\"a\": 10.0}", "{\"a\":1e1}"));
+        assertTrue(JSON.equals("{\"a\": 1.2}", "{\"a\":12e-1}"));
+    }
+
+    private static void assertRuntimeException(Runnable runnable) {
+        try {
+            runnable.run();
+            fail("Expected RuntimeException to be thrown, but no exception was thrown");
+        } catch (RuntimeException e) {
+            // OK
+        }
+    }
+
 }