From 1a3916a8de06b84a0d40800818c0984695242857 Mon Sep 17 00:00:00 2001 From: David Turner Date: Thu, 28 Mar 2019 12:13:25 +0000 Subject: [PATCH] Optimise rejection of out-of-range `long` values (#40325) Today if you try and insert a very large number like `1e9999999` into a long field we first construct this number as a `BigDecimal`, convert this to a `BigInteger` and then reject it because it is out of range. Unfortunately making such a large `BigInteger` is rather expensive. We can avoid this expense by performing a (weaker) range check on the `BigDecimal` representation of incoming `long`s too. Relates #26137 Closes #40323 --- .../support/AbstractXContentParser.java | 16 ++++++++++++--- .../org/elasticsearch/common/Numbers.java | 8 ++++++++ .../elasticsearch/common/NumbersTests.java | 20 +++++++++++++------ .../index/mapper/NumberFieldMapperTests.java | 8 ++++++++ 4 files changed, 43 insertions(+), 9 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/support/AbstractXContentParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/support/AbstractXContentParser.java index 51bb5c3c65f..fa6ffdd0407 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/support/AbstractXContentParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/support/AbstractXContentParser.java @@ -151,6 +151,12 @@ public abstract class AbstractXContentParser implements XContentParser { 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); + /** 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 * value and {@code coerce} is false. */ @@ -163,7 +169,11 @@ public abstract class AbstractXContentParser implements XContentParser { final BigInteger bigIntegerValue; try { - BigDecimal bigDecimalValue = new BigDecimal(stringValue); + 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) { + throw new IllegalArgumentException("Value [" + stringValue + "] is out of range for a long"); + } bigIntegerValue = coerce ? bigDecimalValue.toBigInteger() : bigDecimalValue.toBigIntegerExact(); } catch (ArithmeticException e) { throw new IllegalArgumentException("Value [" + stringValue + "] has a decimal part"); @@ -171,11 +181,11 @@ public abstract class AbstractXContentParser implements XContentParser { throw new IllegalArgumentException("For input string: \"" + stringValue + "\""); } - if (bigIntegerValue.compareTo(BigInteger.valueOf(Long.MAX_VALUE)) > 0 || - bigIntegerValue.compareTo(BigInteger.valueOf(Long.MIN_VALUE)) < 0) { + if (bigIntegerValue.compareTo(LONG_MAX_VALUE_AS_BIGINTEGER) > 0 || bigIntegerValue.compareTo(LONG_MIN_VALUE_AS_BIGINTEGER) < 0) { throw new IllegalArgumentException("Value [" + stringValue + "] is out of range for a long"); } + assert bigIntegerValue.longValueExact() <= Long.MAX_VALUE; // asserting that no ArithmeticException is thrown return bigIntegerValue.longValue(); } diff --git a/server/src/main/java/org/elasticsearch/common/Numbers.java b/server/src/main/java/org/elasticsearch/common/Numbers.java index 27c1dd18e97..51aecb5e19c 100644 --- a/server/src/main/java/org/elasticsearch/common/Numbers.java +++ b/server/src/main/java/org/elasticsearch/common/Numbers.java @@ -125,6 +125,10 @@ public final class Numbers { } } + // 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); + /** 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 * value and {@code coerce} is false. */ @@ -138,6 +142,10 @@ public final class Numbers { final BigInteger bigIntegerValue; try { BigDecimal bigDecimalValue = new BigDecimal(stringValue); + if (bigDecimalValue.compareTo(BIGDECIMAL_GREATER_THAN_LONG_MAX_VALUE) >= 0 || + bigDecimalValue.compareTo(BIGDECIMAL_LESS_THAN_LONG_MIN_VALUE) <= 0) { + throw new IllegalArgumentException("Value [" + stringValue + "] is out of range for a long"); + } bigIntegerValue = coerce ? bigDecimalValue.toBigInteger() : bigDecimalValue.toBigIntegerExact(); } catch (ArithmeticException e) { throw new IllegalArgumentException("Value [" + stringValue + "] has a decimal part"); diff --git a/server/src/test/java/org/elasticsearch/common/NumbersTests.java b/server/src/test/java/org/elasticsearch/common/NumbersTests.java index 46378ccc9e9..4cab3206b7f 100644 --- a/server/src/test/java/org/elasticsearch/common/NumbersTests.java +++ b/server/src/test/java/org/elasticsearch/common/NumbersTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.common; +import com.carrotsearch.randomizedtesting.annotations.Timeout; import org.elasticsearch.test.ESTestCase; import java.math.BigDecimal; @@ -27,19 +28,26 @@ import java.util.concurrent.atomic.AtomicInteger; public class NumbersTests extends ESTestCase { + @Timeout(millis = 10000) public void testToLong() { assertEquals(3L, Numbers.toLong("3", false)); assertEquals(3L, Numbers.toLong("3.1", true)); assertEquals(9223372036854775807L, Numbers.toLong("9223372036854775807.00", false)); assertEquals(-9223372036854775808L, Numbers.toLong("-9223372036854775808.00", false)); + assertEquals(9223372036854775807L, Numbers.toLong("9223372036854775807.00", true)); + assertEquals(-9223372036854775808L, Numbers.toLong("-9223372036854775808.00", true)); + assertEquals(9223372036854775807L, Numbers.toLong("9223372036854775807.99", true)); + assertEquals(-9223372036854775808L, Numbers.toLong("-9223372036854775808.99", true)); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> Numbers.toLong("9223372036854775808", false)); - assertEquals("Value [9223372036854775808] is out of range for a long", e.getMessage()); + assertEquals("Value [9223372036854775808] is out of range for a long", expectThrows(IllegalArgumentException.class, + () -> Numbers.toLong("9223372036854775808", false)).getMessage()); + assertEquals("Value [-9223372036854775809] is out of range for a long", expectThrows(IllegalArgumentException.class, + () -> Numbers.toLong("-9223372036854775809", false)).getMessage()); - e = expectThrows(IllegalArgumentException.class, - () -> Numbers.toLong("-9223372036854775809", false)); - assertEquals("Value [-9223372036854775809] is out of range for a long", e.getMessage()); + assertEquals("Value [1e99999999] is out of range for a long", expectThrows(IllegalArgumentException.class, + () -> Numbers.toLong("1e99999999", false)).getMessage()); + assertEquals("Value [-1e99999999] is out of range for a long", expectThrows(IllegalArgumentException.class, + () -> Numbers.toLong("-1e99999999", false)).getMessage()); } public void testToLongExact() { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/NumberFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/NumberFieldMapperTests.java index ba7f5d84684..b4b9242daa4 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/NumberFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/NumberFieldMapperTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.index.mapper; +import com.carrotsearch.randomizedtesting.annotations.Timeout; import org.apache.lucene.index.DocValuesType; import org.apache.lucene.index.IndexableField; import org.elasticsearch.common.Strings; @@ -367,17 +368,20 @@ public class NumberFieldMapperTests extends AbstractNumericFieldMapperTestCase { } } + @Timeout(millis = 30000) public void testOutOfRangeValues() throws IOException { final List> inputs = Arrays.asList( OutOfRangeSpec.of(NumberType.BYTE, "128", "is out of range for a byte"), OutOfRangeSpec.of(NumberType.SHORT, "32768", "is out of range for a short"), OutOfRangeSpec.of(NumberType.INTEGER, "2147483648", "is out of range for an integer"), OutOfRangeSpec.of(NumberType.LONG, "9223372036854775808", "out of range for a long"), + OutOfRangeSpec.of(NumberType.LONG, "1e999999999", "out of range for a long"), OutOfRangeSpec.of(NumberType.BYTE, "-129", "is out of range for a byte"), OutOfRangeSpec.of(NumberType.SHORT, "-32769", "is out of range for a short"), OutOfRangeSpec.of(NumberType.INTEGER, "-2147483649", "is out of range for an integer"), OutOfRangeSpec.of(NumberType.LONG, "-9223372036854775809", "out of range for a long"), + OutOfRangeSpec.of(NumberType.LONG, "-1e999999999", "out of range for a long"), OutOfRangeSpec.of(NumberType.BYTE, 128, "is out of range for a byte"), OutOfRangeSpec.of(NumberType.SHORT, 32768, "out of range of Java short"), @@ -419,6 +423,10 @@ public class NumberFieldMapperTests extends AbstractNumericFieldMapperTestCase { e.getCause().getMessage(), containsString(item.message)); } } + + // the following two strings are in-range for a long after coercion + parseRequest(NumberType.LONG, createIndexRequest("9223372036854775807.9")); + parseRequest(NumberType.LONG, createIndexRequest("-9223372036854775808.9")); } private void parseRequest(NumberType type, BytesReference content) throws IOException {