fix: std.parseJson replaces lone surrogates with U+FFFD instead of corrupting to '?' - #1112
fix: std.parseJson replaces lone surrogates with U+FFFD instead of corrupting to '?'#1112He-Pin wants to merge 1 commit into
Conversation
…rrupting to '?'
Motivation:
std.parseJson kept unpaired UTF-16 surrogates in the parsed string
(like CPython/JS), but every UTF-8 output path then replaced the
unencodable unit with '?': std.parseJson('"\\ud800"') rendered as "?"
while go-jsonnet renders the Unicode replacement character U+FFFD.
No implementation emits '?', and sjsonnet already follows the U+FFFD
policy elsewhere (std.char and %c replace surrogate codepoints with
U+FFFD; UnicodeHandlingTests documents the alignment), so parseJson
contradicted the project's own convention and silently lost data.
Modification:
Sanitize strings in ValVisitor.visitString and object keys in
visitKeyValue via a new ValVisitor.replaceLoneSurrogates helper:
a fast-path scan that returns the input unchanged without allocation
when it contains no surrogates, and otherwise replaces each unpaired
high/low surrogate with U+FFFD while preserving valid pairs.
Result:
std.parseJson('"\\ud800"') == "\ufffd", byte-identical output with
go-jsonnet (22 ef bf bd 22). Valid surrogate pairs pass through
unchanged. The JSON import path (ujson ByteParser) is unaffected: it
already drops or rejects lone surrogates before any visitor runs
(pre-existing behavior, unchanged by this fix).
References:
Found by four-way differential testing (sjsonnet vs go-jsonnet vs
jrsonnet vs spec).
|
Duplicate of #1085, which I missed before opening this — apologies for the noise. The two PRs make opposite design choices for the same problem:
Deferring to #1085: rejecting aligns with the C++ reference and jrsonnet, and covers the import paths this PR left alone (the ujson ByteParser drops/rejects lone surrogates before the visitor on imports, which #1085's fast-path guard addresses deliberately). One data point that may be useful for #1085's review: sjsonnet's std.char / %c replace surrogate codepoints with U+FFFD (StringModule.scala, Format.scala, documented in UnicodeHandlingTests) — so with #1085's reject policy, parseJson would be stricter than char/%c. That asymmetry is defensible (parseJson validates input, char/%c construct output) but worth a sentence in the PR or a code comment. |
1 participant
Motivation
std.parseJsonkeeps unpaired UTF-16 surrogates in the parsed string, but every UTF-8 output path then replaces the unencodable unit with?:std.parseJson('"\\ud800"')output bytes22 3f 22—"?"22 ef bf bd 22—"\uFFFD"22 ef bf bd 22—"\uFFFD"No implementation emits
?. sjsonnet already follows the U+FFFD policy elsewhere —std.charand%creplace surrogate codepoints with U+FFFD (StringModule.scala,Format.scala), andUnicodeHandlingTestsdocuments that alignment — sostd.parseJsoncontradicted the project's own convention and silently corrupted data on output.Modification
ValVisitor(used bystd.parseJsonandstd.parseYaml) now sanitizes string values and object keys through a newValVisitor.replaceLoneSurrogateshelper:Result
std.parseJson('"\\ud800"') == "\ufffd"— byte-identical with go-jsonnet.std.parseJson('"\\ud83d\\ude00"')still yields the emoji."\ud800\ud800"→ two U+FFFD,"a\udc00b"→"a\ufffdb").std.parseJson('{"\\ud800": 1}')["\ufffd"] == 1.JSON imports unaffected: the
.jsonimport path uses ujson's byte parser, which already drops (in values) or rejects (in keys) lone surrogates before any visitor runs — pre-existing behavior, unchanged by this fix.Test plan
new_test_suite/parseJson_lone_surrogate.jsonnet(+ golden) covering lone high, lone low, doubled, mid-string, valid-pair preservation, and key sanitization./mill 'sjsonnet.jvm[_].test'— all Scala versions (2.12.21 / 2.13.18 / 3.3.8) pass./mill 'sjsonnet.js[_].compile' 'sjsonnet.native[_].compile' 'sjsonnet.wasm[_].compile'— pass./mill __.checkFormat— cleanFound by four-way differential testing (sjsonnet vs go-jsonnet vs jrsonnet vs spec).