[
https://issues.apache.org/jira/browse/FLINK-40217?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Sergey Nuyanzin resolved FLINK-40217.
-------------------------------------
Fix Version/s: 2.4.0
Resolution: Fixed
> 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
> Labels: pull-request-available
> Fix For: 2.4.0
>
>
>
> {{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)
