combine utf-16 surrogate pairs in cupsJSONImportString

[tanjiroK-coder]

cupsJSONImportString decodes each \uXXXX escape on its own and never combines a UTF-16 surrogate pair, so "\uD834\uDD1E" (U+1D11E) decodes to two 3-byte CESU-8 sequences (ed a0 b4 ed b4 9e) instead of the 4-byte UTF-8 f0 9d 84 9e, and a lone surrogate leaves a raw ed a0 b4 in the value. The JSON here is untrusted: cupsJSONImportURL/cupsOAuthGetTokens feed it from an OAuth/OIDC endpoint and cupsJWTImportString runs it over token contents, so a hostile server can drop invalid UTF-8 into strings that later get compared, re-encoded or logged. I spotted it reading the \u branch after noticing dnssd.c already folds surrogate pairs correctly. The decoder now pairs a high surrogate (D800-DBFF) with the following low surrogate (DC00-DFFF) into one code point emitted as 4-byte UTF-8, and substitutes U+FFFD for an unpaired half rather than emitting a bare surrogate. The pre-scan already reserves five bytes per \uXXXX, so a combined pair uses four of the ten reserved bytes and the allocation is unchanged. testjson gets two cases that fail on the current code.

Assisted-by: Claude Code:claude-opus-4-8

[michaelrsweet]

I don't really think a specific unit test for this is necessary.

Will look at the rest but I'm inclined to refactor the code a bit first.

[michaelrsweet]

Also, I don't even think that surrogates are technically valid here - they are a UTF-16 encoding side-effect using a block of reserved Unicode code points while \uXXXX escapes a Unicode code point.

[tanjiroK-coder]

Dropped the testjson cases and rebased on master since CHANGES.md had drifted into conflict. Fine with the shape changing in your refactor.

On the surrogates: RFC 8259 §7 defines \u in terms of UTF-16 code units rather than code points, and says a character outside the BMP is written as its surrogate pair, with 𝄞 for U+1D11E as the worked example. So a pair is the only way JSON can escape anything above U+FFFF. Agree a lone half isn't a character; §8.2 calls \uDEAD out as unpredictable behaviour, so whether that becomes U+FFFD or a parse error is your call.

[michaelrsweet]

I hate JSON. Why they would even bother supporting surrogates when the industry has standardized on using UTF-8 (4 bytes UTF-8 vs. 12 bytes \uXXXX\uXXXX) for JSON is beyond me. No, we do not need to keep EBCDIC compatibility...

OK, so first I don't like the current change for a bunch of reasons; ultimately I want to simplify/refactor things, so thanks for what you've done so far but in this case I want to write my own fix and have you verify it, if you'll be so kind...

WRT goals, I'll want to support valid surrogate pairs but error out on invalid hanging/second surrogates and things like escaped BOMs. Valid characters get converted back into UTF-8 with minimal escaping on output/export.

[tanjiroK-coder]

Sure, happy to verify. Push it wherever suits and I'll run a valid pair (U+1D11E should come out as f0 9d 84 9e), a hanging high half, a bare low half and an escaped BOM, plus a round-trip back out through cupsJSONExportString to check the minimal escaping.

No argument on erroring out rather than substituting U+FFFD, strict is easier to reason about. Close this one out whenever you like.

[michaelrsweet]

I pushed the following changes, let me know what you think:

[master ab203c8] Implement JSON escaped Unicode surrogate pair handling (Issue #169)

[tanjiroK-coder]

Built 64f5e42 with ASAN and ran the cases from earlier. The rejections all behave: a hanging high half, a bare low half, a high half followed by a non-surrogate, \uFEFF and an escaped NUL each return NULL with "Bad Unicode escape.". The valid-pair path has two bugs though.

Every pair is rejected at the moment, {"a":"\uD834\uDD1E"} included. After the first hex loop s is still on the last digit of the high half, so strncmp(s, "\\u", 2) looks one byte early and never matches.

With that fixed the combine is wrong for the even planes. (ch - 0xd800) << 10 already has bit 16 set there, so OR-ing in 0x10000 does nothing where it should carry: \uD840\uDC00 (U+20000) decodes as f0 90 80 80 (U+10000) and \uDBFF\uDFFF (U+10FFFF) as f3 bf bf bf. It has to be an add.

diff --git a/cups/json.c b/cups/json.c
index b80b9eb..d06894e 100644
--- a/cups/json.c
+++ b/cups/json.c
@@ -948,14 +948,14 @@ cupsJSONImportString(const char *s)	// I - JSON string
               // embed plane N Unicode characters in 12 bytes instead of 4...
               int	lch;		// "Lower" surrogate
 
-              if (strncmp(s, "\\u", 2) || !isxdigit(s[2] & 255) || !isxdigit(s[3] & 255) || !isxdigit(s[4] & 255) || !isxdigit(s[5] & 255))
+              if (strncmp(s + 1, "\\u", 2) || !isxdigit(s[3] & 255) || !isxdigit(s[4] & 255) || !isxdigit(s[5] & 255) || !isxdigit(s[6] & 255))
               {
 		_cupsSetError(IPP_STATUS_ERROR_INTERNAL, _("Bad Unicode escape."), true);
 		goto error;
               }
 
               // Grab the second escaped Unicode character...
-              for (s ++, lch = 0, digit = 0; digit < 4; digit ++)
+              for (s += 2, lch = 0, digit = 0; digit < 4; digit ++)
               {
                 // Already know we have "\uXXXX"...
                 s ++;
@@ -974,7 +974,7 @@ cupsJSONImportString(const char *s)	// I - JSON string
 	      }
 
 	      // Combine to form a single 20-bit Unicode character...
-	      ch = 0x10000 | ((ch - 0xd800) << 10) | (lch - 0xdc00);
+	      ch = 0x10000 + (((ch - 0xd800) << 10) | (lch - 0xdc00));
 	    }
 
 	    // Validate the Unicode character...

With that applied U+10000, U+1D11E, U+1F600, U+20000, U+2A6DF, U+E0001 and U+10FFFF all decode to the right four bytes, cupsJSONExportString writes them back out as raw UTF-8 and the result re-imports to the same value, and testjson still passes.

[michaelrsweet]

@tanjiroK-coder Thanks! I incorporated this change and went ahead and added a unit test to verify the success path:

[master 6e67afd] Fix issues in JSON surrogate support, add unit test (Issue #169)

I might also add a bunch of "bad" JSON to test for basic grammar/usage conformance (like we already do for URIs) but I also know this code is getting fuzz-tested so I don't want to spend too much time on that...