Fix silently dropped TRMNL image_url: decode \uXXXX escapes, widen token buffer#2
Open
mcmphd wants to merge 5 commits into
Open
Conversation
TRMNL's /api/display response JSON-escapes ampersands in the presigned S3 image_url inconsistently across responses -- sometimes a literal '&' (1 byte), sometimes & (6 bytes; both are valid JSON). StreamingJsonParser intentionally passes \uXXXX escapes through as their literal 6-character source text rather than decoding them (see the 'u' case in onEscapedChar()), so a URL with ~6-7 escaped ampersands can run ~30-35 bytes longer in the token buffer than the same URL with literal '&'. TOKEN_BUF_SIZE was 512. A real image_url measured 486 bytes with literal '&' -- comfortably under the limit -- but 521 bytes with & escaping for the identical response, over it. When a token overflows the buffer, the parser's tokenOverflow guard silently skips the onString() callback entirely: no error, no log. TrmnlDisplayJsonParser's imageUrlFound simply stays false, TrmnlSleepClient::fetchLatest() logs a bare "TRMNL fetch failed (fetch=0 validate=0)" with nothing pointing at the real cause, and the device falls back to a stale cached image or the default sleep screen -- indistinguishable on-device from a genuine network failure. Raise TOKEN_BUF_SIZE and the matching TrmnlDisplayJsonParser::imageUrl buffer from 512 to 1024 bytes, with real headroom rather than tuned to one observed case. Added a regression test (testHandlesLongEscapedImageUrl) using a real measured TRMNL response with &-escaped ampersands substituted in. Verified it fails at exactly parser.foundImageUrl() under the old 512-byte size (reproducing the silent failure) and passes at 1024. Full existing suite (test/run_trmnl_display_json_parser_test.sh) and a `pio run -e simulator` firmware build both pass. Not fixed here: \uXXXX escapes are still passed through literally rather than decoded, which is the deeper correctness issue (a passed-through & in a URL is not itself valid for use in an HTTP request if it were ever needed downstream, independent of buffer size). That requires adding real escape-sequence state to StreamingJsonParser (hex digit consumption, surrogate pairs, UTF-8 encoding) and is scoped as a separate, more involved fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Follow-up to the previous commit on this branch, which mitigated the
silent image_url drop by widening TOKEN_BUF_SIZE. This addresses the
deeper cause it deliberately left open: StreamingJsonParser passed
\uXXXX escapes through as their literal 6-character source text (see the
old 'u' case in handleStringChar()) rather than decoding them, on the
assumption that "our use case only needs ASCII field matching." That
assumption doesn't hold for image_url -- its *value*, not just its key,
is used downstream to build the actual HTTP request for the image, so a
literal & sitting where an HTTP client needs a real '&' silently
breaks that request rather than merely wasting buffer space.
Implements real \uXXXX decoding to UTF-8:
- Hex digits are consumed via new member state (unicodeDigitsRemaining,
unicodeValue), not locals, since a single escape can arrive split
across separate feed() calls -- same reason literalPos/literalExpected
already exist for split true/false/null literals.
- UTF-16 surrogate pairs (codepoints beyond the BMP, e.g. emoji) are
combined into one codepoint before encoding, via pendingHighSurrogate.
- Encodes to 1-4 byte UTF-8 depending on codepoint range, via a new
appendUtf8CodePoint() shared by both the surrogate-pair and
direct-codepoint paths.
- A lone high surrogate, a lone low surrogate, or a non-hex-digit inside
\uXXXX now sets the existing error flag (hasError()) instead of
producing corrupt output -- consistent with how this parser already
treats other malformed input (bad literals, nesting overflow).
TOKEN_BUF_SIZE stays at 1024 from the previous commit -- decoding removes
the escape-inflation problem for the common ASCII-punctuation case (a
decoded '&' is back to costing 1 byte, not 6), but there's no reason to
shrink it back down; the extra headroom is cheap (this device has 200+KB
free heap) and guards against genuinely long tokens regardless of cause.
Tests:
- StreamingJsonParserTest.cpp: replaced the old UnicodeEscapePassthrough
test (which asserted the raw-passthrough behavior this commit removes)
with coverage for 1/2/3/4-byte UTF-8 output, the real & case that
motivated this, all three malformed-escape error cases, and a
chunked-feed split landing mid-hex-digit (proving the state survives
across feed() boundaries). 38/38 pass via the existing CMake+gtest
target (test/streaming_json_parser).
- TrmnlDisplayJsonParserTest.cpp: updated testHandlesLongEscapedImageUrl
to assert against the correctly decoded URL instead of the old
passthrough text, building the escaped wire-format input from it
programmatically. 6/6 pass via
test/run_trmnl_display_json_parser_test.sh.
- Full `pio run -e simulator` firmware build passes.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewer's GuideImplements proper \uXXXX Unicode escape decoding (including surrogate pairs) in StreamingJsonParser, treats malformed escapes as errors, and increases the JSON token/image_url buffer sizes to prevent silent truncation, with corresponding regression tests for decoding, error cases, chunked feeds, and a long TRMNL image_url. Sequence diagram for \uXXXX decoding in StreamingJsonParsersequenceDiagram
participant Client
participant StreamingJsonParser
Client->>StreamingJsonParser: feed(data_with_escaped_url, len)
loop for_each_character
StreamingJsonParser->>StreamingJsonParser: handleStringChar(c)
alt unicodeDigitsRemaining > 0
StreamingJsonParser->>StreamingJsonParser: hexDigitValue(c)
opt unicodeDigitsRemaining == 0
StreamingJsonParser->>StreamingJsonParser: finishUnicodeEscape()
alt pendingHighSurrogate != 0 and valid_low_surrogate
StreamingJsonParser->>StreamingJsonParser: appendUtf8CodePoint(codepoint)
else malformed_surrogate_pair
StreamingJsonParser->>StreamingJsonParser: error = true
end
end
else escaped && c == 'u'
StreamingJsonParser->>StreamingJsonParser: unicodeDigitsRemaining = 4
else normal_char
StreamingJsonParser->>StreamingJsonParser: appendToken(c)
end
end
opt string_terminated_without_error
StreamingJsonParser->>StreamingJsonParser: emitToken()
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="lib/JsonParser/StreamingJsonParser.cpp" line_range="133-134" />
<code_context>
void StreamingJsonParser::handleStringChar(char c) {
+ if (unicodeDigitsRemaining > 0) {
+ const int digit = hexDigitValue(c);
+ if (digit < 0) {
+ error = true;
+ return;
+ }
</code_context>
<issue_to_address>
**suggestion:** Clear unicode parsing state when encountering an invalid hex digit to keep parser state self-consistent after error.
When `digit < 0` you set `error = true` but leave `unicodeDigitsRemaining` and `unicodeValue` unchanged, so if a caller keeps sending data after an error, the parser will still treat subsequent chars as part of the same `\uXXXX` sequence. Since `error` is terminal this won’t affect correct usage, but you should reset `unicodeDigitsRemaining` (and ideally `unicodeValue`) here to keep the internal state consistent and more robust to misuse.
Suggested implementation:
```cpp
void StreamingJsonParser::handleStringChar(char c) {
if (unicodeDigitsRemaining > 0) {
const int digit = hexDigitValue(c);
if (digit < 0) {
// Invalid hex digit in a \uXXXX sequence: mark error and reset unicode parsing state
error = true;
unicodeDigitsRemaining = 0;
unicodeValue = 0;
return;
}
```
This edit assumes that:
1. `unicodeValue` is a member field already used elsewhere to accumulate the value of a `\uXXXX` sequence.
2. The rest of `handleStringChar` (after the snippet you showed) correctly updates `unicodeValue` and decrements `unicodeDigitsRemaining` for valid digits.
If `unicodeValue` has a different name in this file (e.g., `currentUnicodeValue`), adjust the assignment accordingly.
</issue_to_address>
### Comment 2
<location path="test/streaming_json_parser/StreamingJsonParserTest.cpp" line_range="153-159" />
<code_context>
-TEST(StreamingJsonParser, UnicodeEscapePassthrough) {
- auto events = parse(R"({"u": "\u0041\u0042"})");
+TEST(StreamingJsonParser, UnicodeEscapeBasicAscii) {
+ // AB -- two BMP code points in the 1-byte-UTF-8 range -- decode to "AB".
+ auto events = parse(R"({"u": "AB"})");
ASSERT_EQ(events.size(), 4u);
EXPECT_EQ(events[2].type, EventType::STRING);
- // \uXXXX passed through as literal \u followed by the hex digits
- EXPECT_EQ(events[2].value, "\\u0041\\u0042");
+ EXPECT_EQ(events[2].value, "AB");
+}
+
</code_context>
<issue_to_address>
**issue (testing):** This test no longer exercises \uXXXX decoding and is now misleadingly named/documented
With this change, the test no longer drives the `\u` escape path (`unicodeDigitsRemaining` / `finishUnicodeEscape()`), since it now parses an already-decoded "AB" literal. Given the test name and comment, it should validate decoding of `\u0041\u0042` into "AB". Please change the JSON back to something like `R"({"u": "\u0041\u0042"})` and assert the decoded value is "AB" to keep explicit coverage of ASCII-range `\uXXXX` handling.
</issue_to_address>
### Comment 3
<location path="test/streaming_json_parser/StreamingJsonParserTest.cpp" line_range="173-179" />
<code_context>
+ EXPECT_EQ(events[2].value, "a&b&c");
+}
+
+TEST(StreamingJsonParser, UnicodeEscapeTwoByteUtf8) {
+ // e-acute -- first code point requiring 2-byte UTF-8 (0xC3 0xA9).
+ auto events = parse(R"({"u": "café"})");
+
+ ASSERT_EQ(events.size(), 4u);
+ EXPECT_EQ(events[2].type, EventType::STRING);
+ EXPECT_EQ(events[2].value, "caf\xc3\xa9");
+}
+
</code_context>
<issue_to_address>
**issue (testing):** Two-byte UTF-8 test uses a literal UTF-8 character instead of a \u escape, so it doesn't cover the new decoder
This test currently only checks that the compiler’s pre-encoded UTF‑8 string is passed through. To exercise the Unicode escape decoding path, the JSON should contain a `\uXXXX` escape (e.g. `"c\u00E9"` or `"\u0063\u00E9"`) and then assert that the decoded UTF‑8 bytes equal `"caf\xC3\xA9"`. That way the test actually validates 2-byte UTF‑8 output from `\u` escapes.
</issue_to_address>
### Comment 4
<location path="test/streaming_json_parser/StreamingJsonParserTest.cpp" line_range="182-189" />
<code_context>
+ EXPECT_EQ(events[2].value, "caf\xc3\xa9");
+}
+
+TEST(StreamingJsonParser, UnicodeEscapeThreeByteUtf8) {
+ // SNOWMAN (U+2603) -- requires 3-byte UTF-8 (0xE2 0x98 0x83).
+ auto events = parse(R"({"u": "☃"})");
+
+ ASSERT_EQ(events.size(), 4u);
+ EXPECT_EQ(events[2].type, EventType::STRING);
+ EXPECT_EQ(events[2].value, "\xe2\x98\x83");
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Three-byte UTF-8 test should use a \uXXXX escape to cover decoding logic, not a literal snowman
This test currently embeds `"☃"` directly in the JSON string, so it only checks that existing UTF‑8 bytes are preserved. To validate the new `\uXXXX` decoding path, please change the JSON to `R"({"u": "\u2603"})"` while keeping the same UTF‑8 expectation. That will exercise `finishUnicodeEscape()` for a non‑ASCII, non‑surrogate BMP code point.
```suggestion
TEST(StreamingJsonParser, UnicodeEscapeThreeByteUtf8) {
// SNOWMAN (U+2603) -- requires 3-byte UTF-8 (0xE2 0x98 0x83).
// Use \uXXXX escape to exercise finishUnicodeEscape() for a non-ASCII BMP code point.
auto events = parse(R"({"u": "\u2603"})");
ASSERT_EQ(events.size(), 4u);
EXPECT_EQ(events[2].type, EventType::STRING);
EXPECT_EQ(events[2].value, "\xe2\x98\x83");
}
```
</issue_to_address>
### Comment 5
<location path="test/streaming_json_parser/StreamingJsonParserTest.cpp" line_range="191-199" />
<code_context>
+ EXPECT_EQ(events[2].value, "\xe2\x98\x83");
+}
+
+TEST(StreamingJsonParser, UnicodeEscapeSurrogatePairFourByteUtf8) {
+ // U+1F600 GRINNING FACE, beyond the BMP -- JSON encodes it as a UTF-16 surrogate
+ // pair (😀) that must combine into a single 4-byte UTF-8 sequence
+ // (0xF0 0x9F 0x98 0x80), not two separate (invalid) 3-byte sequences.
+ auto events = parse(R"({"u": "😀"})");
+
+ ASSERT_EQ(events.size(), 4u);
+ EXPECT_EQ(events[2].type, EventType::STRING);
+ EXPECT_EQ(events[2].value, "\xf0\x9f\x98\x80");
+}
+
</code_context>
<issue_to_address>
**issue (testing):** Surrogate-pair success path is not exercised via explicit \uDxxx/\uDxxx sequences
This test currently uses the literal "😀", so it never exercises the JSON `\u` escape path or surrogate-pair handling. As a result, we lack a positive test that `"\uD83D\uDE00"` is accepted and decoded into the expected 4‑byte UTF‑8 sequence. Please change this test to use `R"({"u": "\uD83D\uDE00"})"` so it covers surrogate combination, and consider adding a variant that passes the high and low surrogates in separate `feed()` calls to verify `pendingHighSurrogate` across chunk boundaries.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+153
to
+159
| TEST(StreamingJsonParser, UnicodeEscapeBasicAscii) { | ||
| // AB -- two BMP code points in the 1-byte-UTF-8 range -- decode to "AB". | ||
| auto events = parse(R"({"u": "AB"})"); | ||
|
|
||
| ASSERT_EQ(events.size(), 4u); | ||
| EXPECT_EQ(events[2].type, EventType::STRING); | ||
| // \uXXXX passed through as literal \u followed by the hex digits | ||
| EXPECT_EQ(events[2].value, "\\u0041\\u0042"); | ||
| EXPECT_EQ(events[2].value, "AB"); |
There was a problem hiding this comment.
issue (testing): This test no longer exercises \uXXXX decoding and is now misleadingly named/documented
With this change, the test no longer drives the \u escape path (unicodeDigitsRemaining / finishUnicodeEscape()), since it now parses an already-decoded "AB" literal. Given the test name and comment, it should validate decoding of \u0041\u0042 into "AB". Please change the JSON back to something like R"({"u": "\u0041\u0042"}) and assert the decoded value is "AB" to keep explicit coverage of ASCII-range \uXXXX handling.
Comment on lines
+173
to
+179
| TEST(StreamingJsonParser, UnicodeEscapeTwoByteUtf8) { | ||
| // e-acute -- first code point requiring 2-byte UTF-8 (0xC3 0xA9). | ||
| auto events = parse(R"({"u": "café"})"); | ||
|
|
||
| ASSERT_EQ(events.size(), 4u); | ||
| EXPECT_EQ(events[2].type, EventType::STRING); | ||
| EXPECT_EQ(events[2].value, "caf\xc3\xa9"); |
There was a problem hiding this comment.
issue (testing): Two-byte UTF-8 test uses a literal UTF-8 character instead of a \u escape, so it doesn't cover the new decoder
This test currently only checks that the compiler’s pre-encoded UTF‑8 string is passed through. To exercise the Unicode escape decoding path, the JSON should contain a \uXXXX escape (e.g. "c\u00E9" or "\u0063\u00E9") and then assert that the decoded UTF‑8 bytes equal "caf\xC3\xA9". That way the test actually validates 2-byte UTF‑8 output from \u escapes.
Comment on lines
+182
to
+189
| TEST(StreamingJsonParser, UnicodeEscapeThreeByteUtf8) { | ||
| // SNOWMAN (U+2603) -- requires 3-byte UTF-8 (0xE2 0x98 0x83). | ||
| auto events = parse(R"({"u": "☃"})"); | ||
|
|
||
| ASSERT_EQ(events.size(), 4u); | ||
| EXPECT_EQ(events[2].type, EventType::STRING); | ||
| EXPECT_EQ(events[2].value, "\xe2\x98\x83"); | ||
| } |
There was a problem hiding this comment.
suggestion (testing): Three-byte UTF-8 test should use a \uXXXX escape to cover decoding logic, not a literal snowman
This test currently embeds "☃" directly in the JSON string, so it only checks that existing UTF‑8 bytes are preserved. To validate the new \uXXXX decoding path, please change the JSON to R"({"u": "\u2603"})" while keeping the same UTF‑8 expectation. That will exercise finishUnicodeEscape() for a non‑ASCII, non‑surrogate BMP code point.
Suggested change
| TEST(StreamingJsonParser, UnicodeEscapeThreeByteUtf8) { | |
| // SNOWMAN (U+2603) -- requires 3-byte UTF-8 (0xE2 0x98 0x83). | |
| auto events = parse(R"({"u": "☃"})"); | |
| ASSERT_EQ(events.size(), 4u); | |
| EXPECT_EQ(events[2].type, EventType::STRING); | |
| EXPECT_EQ(events[2].value, "\xe2\x98\x83"); | |
| } | |
| TEST(StreamingJsonParser, UnicodeEscapeThreeByteUtf8) { | |
| // SNOWMAN (U+2603) -- requires 3-byte UTF-8 (0xE2 0x98 0x83). | |
| // Use \uXXXX escape to exercise finishUnicodeEscape() for a non-ASCII BMP code point. | |
| auto events = parse(R"({"u": "\u2603"})"); | |
| ASSERT_EQ(events.size(), 4u); | |
| EXPECT_EQ(events[2].type, EventType::STRING); | |
| EXPECT_EQ(events[2].value, "\xe2\x98\x83"); | |
| } |
Comment on lines
+191
to
+199
| TEST(StreamingJsonParser, UnicodeEscapeSurrogatePairFourByteUtf8) { | ||
| // U+1F600 GRINNING FACE, beyond the BMP -- JSON encodes it as a UTF-16 surrogate | ||
| // pair (😀) that must combine into a single 4-byte UTF-8 sequence | ||
| // (0xF0 0x9F 0x98 0x80), not two separate (invalid) 3-byte sequences. | ||
| auto events = parse(R"({"u": "😀"})"); | ||
|
|
||
| ASSERT_EQ(events.size(), 4u); | ||
| EXPECT_EQ(events[2].type, EventType::STRING); | ||
| EXPECT_EQ(events[2].value, "\xf0\x9f\x98\x80"); |
There was a problem hiding this comment.
issue (testing): Surrogate-pair success path is not exercised via explicit \uDxxx/\uDxxx sequences
This test currently uses the literal "😀", so it never exercises the JSON \u escape path or surrogate-pair handling. As a result, we lack a positive test that "\uD83D\uDE00" is accepted and decoded into the expected 4‑byte UTF‑8 sequence. Please change this test to use R"({"u": "\uD83D\uDE00"})" so it covers surrogate combination, and consider adding a variant that passes the high and low surrogates in separate feed() calls to verify pendingHighSurrogate across chunk boundaries.
…ken tests Comment 1: handleStringChar() set error=true on an invalid hex digit inside \uXXXX but left unicodeDigitsRemaining/unicodeValue unchanged. error is terminal in normal use (feed()'s loop stops on it), but reset both fields anyway so the parser's internal state stays self-consistent for any caller that inspects or reuses it after an error rather than discarding it. Comments 2-5: five of the new StreamingJsonParserTest.cpp tests (UnicodeEscapeBasicAscii, UnicodeEscapeAmpersand, UnicodeEscapeTwoByteUtf8, UnicodeEscapeThreeByteUtf8, UnicodeEscapeSurrogatePairFourByteUtf8) turned out to contain pre-decoded literal text instead of \uXXXX escape sequences in their JSON input -- e.g. UnicodeEscapeAmpersand's input was literally "a&b&c", not "a&b&c". Verified byte-for-byte with od(1): every one of the five had the same defect. These tests exercised nothing new -- they'd have passed identically against the parser before this branch's first two commits, since a parser that never decodes \uXXXX still passes already-decoded text through unchanged. Rewrote all five to contain genuine \uXXXX escapes in their input and assert against the correctly decoded output, so they actually exercise finishUnicodeEscape() and appendUtf8CodePoint() as intended. Also added UnicodeEscapeSurrogatePairSplitAcrossFeed per the reviewer's suggestion: feeds the high surrogate and low surrogate of the same pair in separate feed() calls, proving pendingHighSurrogate survives as member state across a feed() boundary landing between the two halves of a pair -- a different and more important edge case than the existing mid-hex-digit split test. All 39 tests in test/streaming_json_parser pass (was 38; added 1). All 6 tests in test/trmnl_display_json_parser pass. Full `pio run -e simulator` build passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
TRMNL's
/api/displayresponse JSON-escapes ampersands in the presigned S3image_urlinconsistently across responses — sometimes a literal&,sometimes
\u0026(both valid JSON). The parser passed\uXXXXthrough asits literal 6-character source text rather than decoding it, which caused
two compounding issues:
URL length. An escaped-ampersand response could run ~30-35 bytes longer
than the same URL with literal
&s — enough to overflow the buffer.Overflow is handled by silently skipping the callback (
tokenOverflow),so
image_urljust never gets set — no error, no log. The device wouldlog a bare "TRMNL fetch failed" with nothing pointing at the cause, and
fall back to a stale cached image or the default sleep screen.
\u0026sittingwhere an HTTP client needs a real
&produces a URL that's invalid forthe actual image request.
Fix (two commits)
TOKEN_BUF_SIZE/ the matchingimage_urlbuffer from 512 to1024 bytes — immediate mitigation, real headroom rather than tuned to
one observed case.
\uXXXX→ UTF-8 decoding inStreamingJsonParser,including UTF-16 surrogate pairs (codepoints beyond the BMP, e.g. emoji).
A lone high/low surrogate or invalid hex digit now sets the existing
hasError()flag instead of producing corrupt output, consistent withhow the parser already treats other malformed input.
Testing
test/streaming_json_parser: added coverage for 1/2/3/4-byte UTF-8output, the real
&case that motivated this, all threemalformed-escape error cases, and a chunked-feed split landing
mid-hex-digit. 38/38 pass.
test/trmnl_display_json_parser: updated the existing regression testto assert against the correctly decoded URL; verified it fails at
exactly
foundImageUrl()under the old 512-byte size (reproducing thesilent failure) and passes at 1024. 6/6 pass.
pio run -e simulatorbuild passes.TRMNL image fetched successfullyfor a response using\u0026escaping, which previouslyfailed silently on this exact device/account.
Summary by Sourcery
Decode JSON Unicode escape sequences in the streaming parser and increase TRMNL image URL buffer capacity to prevent silently dropped or corrupted image URLs.
Bug Fixes:
Enhancements:
Tests: