[
https://issues.apache.org/jira/browse/FLINK-40217?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ramin Gharib updated FLINK-40217:
---------------------------------
Description:
{{PARSE_JSON}} accepts a JSON number whose magnitude exceeds the {{double}}
range and silently stores it as a non-finite {{{}double{}}}.
{{Variant.toJson()}} then serializes that value as the bare token {{Infinity}}
/ {{{}-Infinity{}}}, which is not valid JSON. The result cannot be read back by
{{{}PARSE_JSON{}}}, so the round-trip {{PARSE_JSON(v).toJson()}} is not closed
over valid JSON.
*Reproduction (SQL)*
{code:java}
-- Accepted. Overflows to a non-finite double:
SELECT PARSE_JSON('1e400');
-- +----------+
-- | EXPR$0 |
-- +----------+
-- | Infinity | <-- invalid JSON emitted by toJson()
-- +----------+
-- '-1e400' likewise yields -Infinity.
-- The reverse direction is already (correctly) rejected:
SELECT PARSE_JSON('Infinity');
-- Caused by: com.fasterxml.jackson.core.JsonParseException:
-- Non-standard token 'Infinity': enable
`JsonReadFeature.ALLOW_NON_NUMERIC_NUMBERS` to allow
{code}
So {{PARSE_JSON}} produces output ({{{}Infinity{}}}) that {{PARSE_JSON}} itself
refuses to accept.
*Root cause*
Two spots in {{{}flink-core{}}}, package {{{}org.apache.flink.types.variant{}}}:
{{1. BinaryVariantInternalBuilder.parseFloatingPoint}} — {{1e400}} uses
scientific notation, so {{tryParseDecimal}} returns false and the value goes
through {{{}appendDouble(parser.getDoubleValue()){}}}. Jackson's
{{getDoubleValue()}} coerces the out-of-range number to
{{{}Double.POSITIVE_INFINITY{}}}. The overflow is silent.
{code:java}
private void parseFloatingPoint(JsonParser parser) throws IOException {
if (!tryParseDecimal(parser.getText())) {
appendDouble(parser.getDoubleValue()); // 1e400 -> +Infinity, no error
}
} {code}
{{2. BinaryVariant.toJsonImpl}} — the {{DOUBLE}} and {{FLOAT}} cases append via
{{{}StringBuilder.append(double/float){}}}, i.e. {{{}Double.toString(...){}}},
which yields {{Infinity}} / {{-Infinity}} / {{{}NaN{}}}. None are valid JSON.
{code:java}
case DOUBLE:
sb.append(BinaryVariantUtil.getDouble(value, pos)); // "Infinity" etc.
break;
...
case FLOAT:
sb.append(BinaryVariantUtil.getFloat(value, pos));
break; {code}
The builder API ({{{}Variant.newBuilder().of(Double.NaN){}}}) can also place a
non-finite value into a Variant, which hits the same {{toJson()}} defect.
{{PARSE_JSON}} can only reach {{±Infinity}} (via overflow); {{NaN}} is only
reachable through the builder.
*Expected behavior*
{{toJson()}} must always produce valid JSON, and it should be readable back by
{{{}PARSE_JSON{}}}. A Variant sourced from JSON should never hold a value that
JSON cannot represent.
*Proposed fix*
* *Preferred (parse side):* reject the lossy coercion in
{{{}parseFloatingPoint{}}}. If {{getDoubleValue()}} is non-finite while the
source token was finite text, throw a clear parse error. This makes
{{PARSE_JSON('1e400')}} fail the same way {{PARSE_JSON('Infinity')}} already
does, and keeps non-finite values out of JSON-sourced Variants entirely.
* *Defense-in-depth (serialize side):* in {{{}toJsonImpl{}}}, handle
non-finite {{{}double{}}}/{{{}float{}}} explicitly so a Variant built via the
builder API cannot emit invalid JSON. Options: emit {{null}} (matches
{{JSON.stringify}} semantics) or throw. Emitting {{null}} is lossy; throwing is
safer for a serializer that advertises valid-JSON output. Maintainer's call.
Recommendation: do the parse-side rejection as the root-cause fix, plus a guard
in {{toJson()}} so the builder path is also safe.
*Verifying the change*
Add unit tests in {{BinaryVariantTest}} / the {{PARSE_JSON}} function tests:
assert {{PARSE_JSON('1e400')}} and {{'-1e400'}} fail with a clear message, and
that {{toJson()}} never returns a string containing {{Infinity}} / {{{}NaN{}}}.
Add a round-trip test asserting {{PARSE_JSON(v.toJson())}} succeeds for any
Variant produced by {{{}PARSE_JSON{}}}.
was:
{{PARSE_JSON}} accepts a JSON number whose magnitude exceeds the {{double}}
range and silently stores it as a non-finite {{{}double{}}}.
{{Variant.toJson()}} then serializes that value as the bare token {{Infinity}}
/ {{{}-Infinity{}}}, which is not valid JSON. The result cannot be read back by
{{{}PARSE_JSON{}}}, so the round-trip {{PARSE_JSON(x).toJson()}} is not closed
over valid JSON.
*Reproduction (SQL)*
{code:java}
-- Accepted. Overflows to a non-finite double:
SELECT PARSE_JSON('1e400');
-- +----------+
-- | EXPR$0 |
-- +----------+
-- | Infinity | <-- invalid JSON emitted by toJson()
-- +----------+
-- '-1e400' likewise yields -Infinity.
-- The reverse direction is already (correctly) rejected:
SELECT PARSE_JSON('Infinity');
-- Caused by: com.fasterxml.jackson.core.JsonParseException:
-- Non-standard token 'Infinity': enable
`JsonReadFeature.ALLOW_NON_NUMERIC_NUMBERS` to allow
{code}
So {{PARSE_JSON}} produces output ({{{}Infinity{}}}) that {{PARSE_JSON}} itself
refuses to accept.
*Root cause*
Two spots in {{{}flink-core{}}}, package {{{}org.apache.flink.types.variant{}}}:
{{1. BinaryVariantInternalBuilder.parseFloatingPoint}} — {{1e400}} uses
scientific notation, so {{tryParseDecimal}} returns false and the value goes
through {{{}appendDouble(parser.getDoubleValue()){}}}. Jackson's
{{getDoubleValue()}} coerces the out-of-range number to
{{{}Double.POSITIVE_INFINITY{}}}. The overflow is silent.
{code:java}
private void parseFloatingPoint(JsonParser parser) throws IOException {
if (!tryParseDecimal(parser.getText())) {
appendDouble(parser.getDoubleValue()); // 1e400 -> +Infinity, no error
}
} {code}
{{2. BinaryVariant.toJsonImpl}} — the {{DOUBLE}} and {{FLOAT}} cases append via
{{{}StringBuilder.append(double/float){}}}, i.e. {{{}Double.toString(...){}}},
which yields {{Infinity}} / {{-Infinity}} / {{{}NaN{}}}. None are valid JSON.
{code:java}
case DOUBLE:
sb.append(BinaryVariantUtil.getDouble(value, pos)); // "Infinity" etc.
break;
...
case FLOAT:
sb.append(BinaryVariantUtil.getFloat(value, pos));
break; {code}
The builder API ({{{}Variant.newBuilder().of(Double.NaN){}}}) can also place a
non-finite value into a Variant, which hits the same {{toJson()}} defect.
{{PARSE_JSON}} can only reach {{±Infinity}} (via overflow); {{NaN}} is only
reachable through the builder.
*Expected behavior*
{{toJson()}} must always produce valid JSON, and it should be readable back by
{{{}PARSE_JSON{}}}. A Variant sourced from JSON should never hold a value that
JSON cannot represent.
*Proposed fix*
* *Preferred (parse side):* reject the lossy coercion in
{{{}parseFloatingPoint{}}}. If {{getDoubleValue()}} is non-finite while the
source token was finite text, throw a clear parse error. This makes
{{PARSE_JSON('1e400')}} fail the same way {{PARSE_JSON('Infinity')}} already
does, and keeps non-finite values out of JSON-sourced Variants entirely.
* *Defense-in-depth (serialize side):* in {{{}toJsonImpl{}}}, handle
non-finite {{{}double{}}}/{{{}float{}}} explicitly so a Variant built via the
builder API cannot emit invalid JSON. Options: emit {{null}} (matches
{{JSON.stringify}} semantics) or throw. Emitting {{null}} is lossy; throwing is
safer for a serializer that advertises valid-JSON output. Maintainer's call.
Recommendation: do the parse-side rejection as the root-cause fix, plus a guard
in {{toJson()}} so the builder path is also safe.
*Verifying the change*
Add unit tests in {{BinaryVariantTest}} / the {{PARSE_JSON}} function tests:
assert {{PARSE_JSON('1e400')}} and {{'-1e400'}} fail with a clear message, and
that {{toJson()}} never returns a string containing {{Infinity}} / {{{}NaN{}}}.
Add a round-trip test asserting {{PARSE_JSON(v.toJson())}} succeeds for any
Variant produced by {{{}PARSE_JSON{}}}.
> Variant.toJson() can emit invalid JSON for numbers that overflow to
> non-finite doubles
> --------------------------------------------------------------------------------------
>
> Key: FLINK-40217
> URL: https://issues.apache.org/jira/browse/FLINK-40217
> Project: Flink
> Issue Type: Bug
> Components: API / Type Serialization System, Table SQL / API
> Affects Versions: 2.3.0, 2.2.1, 2.1.3
> Reporter: Ramin Gharib
> Assignee: Ramin Gharib
> Priority: Minor
>
>
> {{PARSE_JSON}} accepts a JSON number whose magnitude exceeds the {{double}}
> range and silently stores it as a non-finite {{{}double{}}}.
> {{Variant.toJson()}} then serializes that value as the bare token
> {{Infinity}} / {{{}-Infinity{}}}, which is not valid JSON. The result cannot
> be read back by {{{}PARSE_JSON{}}}, so the round-trip
> {{PARSE_JSON(v).toJson()}} is not closed over valid JSON.
> *Reproduction (SQL)*
> {code:java}
> -- Accepted. Overflows to a non-finite double:
> SELECT PARSE_JSON('1e400');
> -- +----------+
> -- | EXPR$0 |
> -- +----------+
> -- | Infinity | <-- invalid JSON emitted by toJson()
> -- +----------+
> -- '-1e400' likewise yields -Infinity.
> -- The reverse direction is already (correctly) rejected:
> SELECT PARSE_JSON('Infinity');
> -- Caused by: com.fasterxml.jackson.core.JsonParseException:
> -- Non-standard token 'Infinity': enable
> `JsonReadFeature.ALLOW_NON_NUMERIC_NUMBERS` to allow
> {code}
> So {{PARSE_JSON}} produces output ({{{}Infinity{}}}) that {{PARSE_JSON}}
> itself refuses to accept.
> *Root cause*
> Two spots in {{{}flink-core{}}}, package
> {{{}org.apache.flink.types.variant{}}}:
> {{1. BinaryVariantInternalBuilder.parseFloatingPoint}} — {{1e400}} uses
> scientific notation, so {{tryParseDecimal}} returns false and the value goes
> through {{{}appendDouble(parser.getDoubleValue()){}}}. Jackson's
> {{getDoubleValue()}} coerces the out-of-range number to
> {{{}Double.POSITIVE_INFINITY{}}}. The overflow is silent.
> {code:java}
> private void parseFloatingPoint(JsonParser parser) throws IOException {
> if (!tryParseDecimal(parser.getText())) {
> appendDouble(parser.getDoubleValue()); // 1e400 -> +Infinity, no error
> }
> } {code}
> {{2. BinaryVariant.toJsonImpl}} — the {{DOUBLE}} and {{FLOAT}} cases append
> via {{{}StringBuilder.append(double/float){}}}, i.e.
> {{{}Double.toString(...){}}}, which yields {{Infinity}} / {{-Infinity}} /
> {{{}NaN{}}}. None are valid JSON.
> {code:java}
> case DOUBLE:
> sb.append(BinaryVariantUtil.getDouble(value, pos)); // "Infinity" etc.
> break;
> ...
> case FLOAT:
> sb.append(BinaryVariantUtil.getFloat(value, pos));
> break; {code}
> The builder API ({{{}Variant.newBuilder().of(Double.NaN){}}}) can also place
> a non-finite value into a Variant, which hits the same {{toJson()}} defect.
> {{PARSE_JSON}} can only reach {{±Infinity}} (via overflow); {{NaN}} is only
> reachable through the builder.
> *Expected behavior*
> {{toJson()}} must always produce valid JSON, and it should be readable back
> by {{{}PARSE_JSON{}}}. A Variant sourced from JSON should never hold a value
> that JSON cannot represent.
> *Proposed fix*
> * *Preferred (parse side):* reject the lossy coercion in
> {{{}parseFloatingPoint{}}}. If {{getDoubleValue()}} is non-finite while the
> source token was finite text, throw a clear parse error. This makes
> {{PARSE_JSON('1e400')}} fail the same way {{PARSE_JSON('Infinity')}} already
> does, and keeps non-finite values out of JSON-sourced Variants entirely.
> * *Defense-in-depth (serialize side):* in {{{}toJsonImpl{}}}, handle
> non-finite {{{}double{}}}/{{{}float{}}} explicitly so a Variant built via the
> builder API cannot emit invalid JSON. Options: emit {{null}} (matches
> {{JSON.stringify}} semantics) or throw. Emitting {{null}} is lossy; throwing
> is safer for a serializer that advertises valid-JSON output. Maintainer's
> call.
> Recommendation: do the parse-side rejection as the root-cause fix, plus a
> guard in {{toJson()}} so the builder path is also safe.
> *Verifying the change*
> Add unit tests in {{BinaryVariantTest}} / the {{PARSE_JSON}} function tests:
> assert {{PARSE_JSON('1e400')}} and {{'-1e400'}} fail with a clear message,
> and that {{toJson()}} never returns a string containing {{Infinity}} /
> {{{}NaN{}}}. Add a round-trip test asserting {{PARSE_JSON(v.toJson())}}
> succeeds for any Variant produced by {{{}PARSE_JSON{}}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
