Ramin Gharib created FLINK-40217:
------------------------------------
Summary: 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.1.3, 2.2.1, 2.3.0
Reporter: Ramin Gharib
Assignee: Ramin Gharib
{{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{}}}:
# {{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}
# {{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)
