Check the scale before converting xcontent long values, rather than the absolute value (#111538) (#111584)

Large numbers are rejected, small numbers rounded to zero (if rounding enabled)

Author: thecoop

libs/x-content/src/main/java/org/elasticsearch/xcontent/support/AbstractXContentParser.java

@@ -142,11 +142,8 @@ public int intValue(boolean coerce) throws IOException {
 
     protected abstract int doIntValue() throws IOException;
 
-    private static BigInteger LONG_MAX_VALUE_AS_BIGINTEGER = BigInteger.valueOf(Long.MAX_VALUE);
-    private static BigInteger LONG_MIN_VALUE_AS_BIGINTEGER = BigInteger.valueOf(Long.MIN_VALUE);
-    // weak bounds on the BigDecimal representation to allow for coercion
-    private static BigDecimal BIGDECIMAL_GREATER_THAN_LONG_MAX_VALUE = BigDecimal.valueOf(Long.MAX_VALUE).add(BigDecimal.ONE);
-    private static BigDecimal BIGDECIMAL_LESS_THAN_LONG_MIN_VALUE = BigDecimal.valueOf(Long.MIN_VALUE).subtract(BigDecimal.ONE);
+    private static final BigInteger LONG_MAX_VALUE_AS_BIGINTEGER = BigInteger.valueOf(Long.MAX_VALUE);
+    private static final BigInteger LONG_MIN_VALUE_AS_BIGINTEGER = BigInteger.valueOf(Long.MIN_VALUE);
 
     /** Return the long that {@code stringValue} stores or throws an exception if the
      *  stored value cannot be converted to a long that stores the exact same
@@ -161,11 +158,21 @@ private static long toLong(String stringValue, boolean coerce) {
         final BigInteger bigIntegerValue;
         try {
             final BigDecimal bigDecimalValue = new BigDecimal(stringValue);
-            if (bigDecimalValue.compareTo(BIGDECIMAL_GREATER_THAN_LONG_MAX_VALUE) >= 0
-                || bigDecimalValue.compareTo(BIGDECIMAL_LESS_THAN_LONG_MIN_VALUE) <= 0) {
+            // long can have a maximum of 19 digits - any more than that cannot be a long
+            // the scale is stored as the negation, so negative scale -> big number
+            if (bigDecimalValue.scale() < -19) {
                 throw new IllegalArgumentException("Value [" + stringValue + "] is out of range for a long");
             }
-            bigIntegerValue = coerce ? bigDecimalValue.toBigInteger() : bigDecimalValue.toBigIntegerExact();
+            // large scale -> very small number
+            if (bigDecimalValue.scale() > 19) {
+                if (coerce) {
+                    bigIntegerValue = BigInteger.ZERO;
+                } else {
+                    throw new ArithmeticException("Number has a decimal part");
+                }
+            } else {
+                bigIntegerValue = coerce ? bigDecimalValue.toBigInteger() : bigDecimalValue.toBigIntegerExact();
+            }
         } catch (ArithmeticException e) {
             throw new IllegalArgumentException("Value [" + stringValue + "] has a decimal part");
         } catch (NumberFormatException e) {

libs/x-content/src/test/java/org/elasticsearch/xcontent/XContentParserTests.java

@@ -33,6 +33,7 @@
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.in;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.nullValue;
 import static org.junit.internal.matchers.ThrowableMessageMatcher.hasMessage;
 
@@ -83,6 +84,44 @@ public void testFloat() throws IOException {
         }
     }
 
+    public void testLongCoercion() throws IOException {
+        XContentType xContentType = randomFrom(XContentType.values());
+
+        try (XContentBuilder builder = XContentBuilder.builder(xContentType.xContent())) {
+            builder.startObject();
+            builder.field("decimal", "5.5");
+            builder.field("expInRange", "5e18");
+            builder.field("expTooBig", "2e100");
+            builder.field("expTooSmall", "2e-100");
+            builder.endObject();
+
+            try (XContentParser parser = createParser(xContentType.xContent(), BytesReference.bytes(builder))) {
+                assertThat(parser.nextToken(), is(XContentParser.Token.START_OBJECT));
+
+                assertThat(parser.nextToken(), is(XContentParser.Token.FIELD_NAME));
+                assertThat(parser.currentName(), is("decimal"));
+                assertThat(parser.nextToken(), is(XContentParser.Token.VALUE_STRING));
+                assertThat(parser.longValue(), equalTo(5L));
+
+                assertThat(parser.nextToken(), is(XContentParser.Token.FIELD_NAME));
+                assertThat(parser.currentName(), is("expInRange"));
+                assertThat(parser.nextToken(), is(XContentParser.Token.VALUE_STRING));
+                assertThat(parser.longValue(), equalTo((long) 5e18));
+
+                assertThat(parser.nextToken(), is(XContentParser.Token.FIELD_NAME));
+                assertThat(parser.currentName(), is("expTooBig"));
+                assertThat(parser.nextToken(), is(XContentParser.Token.VALUE_STRING));
+                expectThrows(IllegalArgumentException.class, parser::longValue);
+
+                // too small goes to zero
+                assertThat(parser.nextToken(), is(XContentParser.Token.FIELD_NAME));
+                assertThat(parser.currentName(), is("expTooSmall"));
+                assertThat(parser.nextToken(), is(XContentParser.Token.VALUE_STRING));
+                assertThat(parser.longValue(), equalTo(0L));
+            }
+        }
+    }
+
     public void testReadList() throws IOException {
         assertThat(readList("{\"foo\": [\"bar\"]}"), contains("bar"));
         assertThat(readList("{\"foo\": [\"bar\",\"baz\"]}"), contains("bar", "baz"));