Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stricter validation for min/max values for whole numbers #26137

Merged

Conversation

fred84
Copy link
Contributor

@fred84 fred84 commented Aug 10, 2017

Currently we can overcame min/max validation by passing number as string or directly from plugin so its value will be rounded. Example:

curl -XPUT 'localhost:9200/twitter?pretty' -H 'Content-Type: application/json' -d'
{
  "mappings": {
    "tweet": {
      "properties": {
        "size": {
          "type": "long"
        }
      }
    }
  }
}
'

curl -XPUT 'localhost:9200/twitter/tweet/1' -H 'Content-Type: application/json' -d'{ "size":  9223372036854775806 }'
# long max value
curl -XPUT 'localhost:9200/twitter/tweet/2' -H 'Content-Type: application/json' -d'{ "size":  9223372036854775807 }'
# wrapped number > long max value
curl -XPUT 'localhost:9200/twitter/tweet/3' -H 'Content-Type: application/json' -d'{ "size":  "9223372036854775808" }'

curl -XGET 'localhost:9200/twitter/tweet/_search?pretty' -H 'Content-Type: application/json' -d'
{
    "query": {
        "match" : {
            "size" : 9223372036854775807
        }
    }
}
'

{
  #...
  "hits" : {
    "total" : 2,
    "max_score" : 1.0,
    "hits" : [
      {
        "_index" : "twitter",
        "_type" : "tweet",
        "_id" : "2",
        "_score" : 1.0,
        "_source" : {
          "size" : 9223372036854775807
        }
      },
      {
        "_index" : "twitter",
        "_type" : "tweet",
        "_id" : "3",
        "_score" : 1.0,
        "_source" : {
          "size" : "9223372036854775808"
        }
      }
    ]
  }
}

@colings86 @jpountz Please take a look.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@jpountz
Copy link
Contributor

jpountz commented Aug 10, 2017

I'm wondering whether the fix should be upstream in jackson?

@fred84
Copy link
Contributor Author

fred84 commented Aug 11, 2017

@jpountz There are 2 overloaded NumberType.parse methods. I agree that fix for parse(XContentParser parser, boolean coerce) may be in jackson, but for java parse(Object value, boolean coerce) I think it should be in elastic.

@jpountz
Copy link
Contributor

jpountz commented Aug 11, 2017

Maybe I'm missing something but the latter seems to already perform bound checks?

@fred84
Copy link
Contributor Author

fred84 commented Aug 11, 2017

@jpountz Incoming value in parse(Object value, boolean coerce) is converted to double before comparison against min/max value. In case of NumberType.LONG there is not enough precision for double near min/max value.
Here is example demonstrating this:
assertFalse(Double.parseDouble("9223372036854776832") > Long.MAX_VALUE);

@jpountz
Copy link
Contributor

jpountz commented Aug 11, 2017

I am good with improving validation in that case.

@fred84
Copy link
Contributor Author

fred84 commented Aug 11, 2017

I'll update PR. I think same validation for long should be added to AbstractXContentParser.public long longValue(boolean coerce) when handling string values.

@fred84
Copy link
Contributor Author

fred84 commented Aug 12, 2017

@jpountz PR updated. I looked at Jackson's parser.getLongValue() - it validates min/max values correctly. AbstractXContentParser was converting double to short/int/long without proper validation.

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some questions/comments but I think this is going in the right direction.

throw new IllegalArgumentException("Value [" + text() + "] is out of range for a short");
}

return (short) doubleValue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it work if we only had checkCoerceString within this block? From what I read, doShortValue already has the ability to parse strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doShortValue calls parser.getShortValue() which throws exception on non-numeric json values. Even if it is number in wrapped in string:

    public void testGetShortValue() throws Exception {
        JsonFactory factory = new JsonFactory();

        JsonParser parser = factory.createParser("123");
        parser.nextToken();
        assertEquals(123, parser.getShortValue());

        final JsonParser parser2 = factory.createParser("\"123\"");
        parser2.nextToken();
        JsonParseException e = expectThrows(JsonParseException.class, () -> parser2.getShortValue());
        assertEquals("123", parser2.getValueAsString());
    }

}
}
long result = doLongValue();
ensureNumberConversion(coerce, result, Long.class);
return result;
}

public static long preciseLongValue(String stringValue, boolean coerce) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to use it from different classes, please move it to the o.e.common.Numbers class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@fred84
Copy link
Contributor Author

fred84 commented Aug 17, 2017

@jpountz PR updated. Also, when #25861 will be solved, we can futher simplify NumericType.LONG.parse to

Long parse(Object value, boolean coerce) {
    if (value instanceof Number) {
        return Numbers.toExactLong(value);
    }
    String stringValue = (value instanceof BytesRef) ? ((BytesRef) value).utf8ToString() : value.toString();
    return Numbers.toExactLong(stringValue);
}

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it looks good to me.

@jpountz
Copy link
Contributor

jpountz commented Aug 17, 2017

@elasticmachine please test it

@fred84
Copy link
Contributor Author

fred84 commented Aug 17, 2017

@jpountz PR updated

@jpountz
Copy link
Contributor

jpountz commented Aug 17, 2017

@elasticmachine please test it

@jpountz jpountz merged commit 9a3216d into elastic:master Aug 21, 2017
@clintongormley clintongormley added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Dec 12, 2017
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Mar 21, 2019
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 elastic#26137
Closes elastic#40323
DaveCTurner added a commit that referenced this pull request Mar 28, 2019
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
DaveCTurner added a commit that referenced this pull request Mar 28, 2019
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
DaveCTurner added a commit that referenced this pull request Mar 28, 2019
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
DaveCTurner added a commit that referenced this pull request Apr 5, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types v6.1.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants