Repository navigation
Conversation
The C SDK's *_from_json decoders read string values wrongly in two ways. The key lookup was strstr() on the quoted key name, which also matches a string value with the same text. eipc_ack_from_json() looks up "error" and found the status value "error" first, so every failed ack came back with an empty error message. A header whose service_id is "priority" lost its priority the same way. A key is now a quoted name followed by a colon. String values were copied verbatim between the quotes, so escapes were never decoded. A value written by eipc_ack_to_json() or eipc_intent_to_json() came back with its backslashes, and text from Go's encoding/json, which writes < > & as \u003c \u003e \u0026, came back as those six-character codes. The decoder now handles \" \\ \/ \b \f \n \r \t and \uXXXX (as UTF-8, with surrogate pairs); an over-long value is still truncated, but never inside a decoded \uXXXX. \u0000 becomes U+FFFD, since a NUL would cut the C string short. test_chat_json gains two cases that fail before this change and pass after it.
Reviewer's GuideImplementationAdd key-aware value lookup and decode JSON string escapes into UTF-8, then use the shared lookup for string, integer, and float values.
TestsAdd regression coverage for keys that also appear in string values and for escaped strings, including Unicode and NUL handling.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
You've used all your free Kodus-paid PR reviews 🎁Your trial is still active — this just means the PR reviews we cover during the trial are used up. Connect your own AI key to keep Kody reviewing — unlimited reviews, on any plan (Free included). Want more trial reviews to finish evaluating before adding a key? Talk to our founders. 😎 |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Linked issuesChecked at commit cacadc3 #64 C SDK: *_from_json returns the wrong string values (a key matches an equal value; escapes are not decoded) · 2 of 2 met
|
![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 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="sdk/c/src/eipc_json.c" line_range="25" />
<code_context>
+ uint32_t v = 0;
+ int i;
+ for (i = 0; i < 4; i++) {
+ char c = p[i];
+ v <<= 4;
+ if (c >= '0' && c <= '9')
</code_context>
<issue_to_address>
**Truncated escapes read past input**
When a JSON string ends with a truncated `\u` escape, `json_hex4` reads four bytes without checking that they remain before the input terminator, causing an out-of-bounds read and undefined behavior.
Check that four hex digits remain before reading them, and handle truncated escapes without reading past the input.
</issue_to_address>
### Comment 2
<location path="sdk/c/src/eipc_json.c" line_range="74-78" />
<code_context>
- out[0] = '\0';
- return -1;
+ plen = strlen(pattern);
+ while ((p = strstr(p, pattern)) != NULL) {
+ const char *v = p + plen;
+ while (*v == ' ' || *v == '\t' || *v == '\n' || *v == '\r')
+ v++;
+ if (*v == ':') {
+ v++;
+ while (*v == ' ' || *v == '\t' || *v == '\n' || *v == '\r')
</code_context>
<issue_to_address>
**Decoders return embedded field values**
When an earlier string contains escaped field-like text, or a nested object contains the requested field before the root-level field, `json_find_value` scans raw text and accepts the first matching `"key":` without checking whether it is inside a string or at the requested object depth, so decoders return the embedded or nested value instead of the root-level field.
Parse the JSON while tracking strings, escapes, and object depth, and return only a matching root-level member.
</issue_to_address>
### Comment 3
<location path="sdk/c/src/eipc_json.c" line_range="116" />
<code_context>
+ enc[0] = *end++;
+ } else if (end[1] == 'u') {
+ uint32_t cp = json_hex4(end + 2);
+ end += (cp == JSON_BAD_HEX) ? 2 : 6;
+ if (cp >= 0xD800 && cp <= 0xDBFF && end[0] == '\\' && end[1] == 'u') {
+ uint32_t lo = json_hex4(end + 2);
</code_context>
<issue_to_address>
**Malformed escapes alter decoded text**
When input contains an unknown escape such as `\q` or an invalid `\u` sequence, `json_find_string` copies the escaped character and drops the backslash for unknown escapes, while invalid `\u` sequences can leave following characters to be copied; callers receive altered text instead of the documented U+FFFD replacement.
Replace each malformed escape with U+FFFD and consume the malformed escape so its remaining characters are not copied as ordinary text.
Also at `sdk/c/src/eipc_json.c:136-137`.
</issue_to_address>
### Comment 4
<location path="sdk/c/src/eipc_json.c" line_range="127" />
<code_context>
+ /* A NUL would cut the C string short, so \u0000 becomes
+ * U+FFFD, like a malformed escape or an unpaired surrogate. */
+ if (cp == 0 || cp == JSON_BAD_HEX || (cp >= 0xD800 && cp <= 0xDFFF))
+ cp = 0xFFFD;
+ n = json_utf8(cp, enc);
+ } else if (end[1]) {
</code_context>
<issue_to_address>
**Malformed escape regressions go unnoticed**
When an input contains malformed `\u` hex, an unpaired surrogate, or a trailing backslash, the U+FFFD handling and trailing-backslash branches have no regression assertions, so changes that break these decoder paths still pass the C tests and callers receive incorrectly decoded strings.
Add public-decoder regression tests asserting the specified output for malformed Unicode escapes, unpaired surrogates, and a trailing backslash.
Also at `sdk/c/src/eipc_json.c:140`.
</issue_to_address>Sourcery assessment
Approval pending. 4 findings to address first.
Blocking findings: sdk/c/src/eipc_json.c:25, sdk/c/src/eipc_json.c:78, sdk/c/src/eipc_json.c:116, sdk/c/src/eipc_json.c:127
| uint32_t v = 0; | ||
| int i; | ||
| for (i = 0; i < 4; i++) { | ||
| char c = p[i]; |
There was a problem hiding this comment.
🟡 Medium · Truncated escapes read past input
When a JSON string ends with a truncated \u escape, json_hex4 reads four bytes without checking that they remain before the input terminator, causing an out-of-bounds read and undefined behavior.
Check that four hex digits remain before reading them, and handle truncated escapes without reading past the input.
Prompt for AI agents
In `sdk/c/src/eipc_json.c` at line 25:
**Truncated escapes read past input**
When a JSON string ends with a truncated `\u` escape, `json_hex4` reads four bytes without checking that they remain before the input terminator, causing an out-of-bounds read and undefined behavior.
Check that four hex digits remain before reading them, and handle truncated escapes without reading past the input.| while ((p = strstr(p, pattern)) != NULL) { | ||
| const char *v = p + plen; | ||
| while (*v == ' ' || *v == '\t' || *v == '\n' || *v == '\r') | ||
| v++; | ||
| if (*v == ':') { |
There was a problem hiding this comment.
🟡 Medium · Decoders return embedded field values
When an earlier string contains escaped field-like text, or a nested object contains the requested field before the root-level field, json_find_value scans raw text and accepts the first matching "key": without checking whether it is inside a string or at the requested object depth, so decoders return the embedded or nested value instead of the root-level field.
Parse the JSON while tracking strings, escapes, and object depth, and return only a matching root-level member.
Prompt for AI agents
In `sdk/c/src/eipc_json.c` at lines 74-78:
**Decoders return embedded field values**
When an earlier string contains escaped field-like text, or a nested object contains the requested field before the root-level field, `json_find_value` scans raw text and accepts the first matching `"key":` without checking whether it is inside a string or at the requested object depth, so decoders return the embedded or nested value instead of the root-level field.
Parse the JSON while tracking strings, escapes, and object depth, and return only a matching root-level member.| enc[0] = *end++; | ||
| } else if (end[1] == 'u') { | ||
| uint32_t cp = json_hex4(end + 2); | ||
| end += (cp == JSON_BAD_HEX) ? 2 : 6; |
There was a problem hiding this comment.
🟡 Medium · Malformed escapes alter decoded text
When input contains an unknown escape such as \q or an invalid \u sequence, json_find_string copies the escaped character and drops the backslash for unknown escapes, while invalid \u sequences can leave following characters to be copied; callers receive altered text instead of the documented U+FFFD replacement.
Replace each malformed escape with U+FFFD and consume the malformed escape so its remaining characters are not copied as ordinary text.
Also at sdk/c/src/eipc_json.c:136-137.
Prompt for AI agents
In `sdk/c/src/eipc_json.c` at line 116:
**Malformed escapes alter decoded text**
When input contains an unknown escape such as `\q` or an invalid `\u` sequence, `json_find_string` copies the escaped character and drops the backslash for unknown escapes, while invalid `\u` sequences can leave following characters to be copied; callers receive altered text instead of the documented U+FFFD replacement.
Replace each malformed escape with U+FFFD and consume the malformed escape so its remaining characters are not copied as ordinary text.
Also at `sdk/c/src/eipc_json.c:136-137`.From SECURITY-STANDARDS.md L12-14:
Validate every input at the boundary it enters: type, range, length, format and
allowed values. Reject what does not match rather than coercing it. Validation
performed only in the client is not validation.| /* A NUL would cut the C string short, so \u0000 becomes | ||
| * U+FFFD, like a malformed escape or an unpaired surrogate. */ | ||
| if (cp == 0 || cp == JSON_BAD_HEX || (cp >= 0xD800 && cp <= 0xDFFF)) | ||
| cp = 0xFFFD; |
There was a problem hiding this comment.
🟡 Medium · Malformed escape regressions go unnoticed
When an input contains malformed \u hex, an unpaired surrogate, or a trailing backslash, the U+FFFD handling and trailing-backslash branches have no regression assertions, so changes that break these decoder paths still pass the C tests and callers receive incorrectly decoded strings.
Add public-decoder regression tests asserting the specified output for malformed Unicode escapes, unpaired surrogates, and a trailing backslash.
Also at sdk/c/src/eipc_json.c:140.
Prompt for AI agents
In `sdk/c/src/eipc_json.c` at line 127:
**Malformed escape regressions go unnoticed**
When an input contains malformed `\u` hex, an unpaired surrogate, or a trailing backslash, the U+FFFD handling and trailing-backslash branches have no regression assertions, so changes that break these decoder paths still pass the C tests and callers receive incorrectly decoded strings.
Add public-decoder regression tests asserting the specified output for malformed Unicode escapes, unpaired surrogates, and a trailing backslash.
Also at `sdk/c/src/eipc_json.c:140`.From TESTING.md L10:
- **Unit** — logic and its branches, including edge cases.
|
Thanks — the premise checks out against master |
Summary
The C SDK's
*_from_jsondecoders read string values wrongly in two ways:strstr()on the quoted name, soeipc_ack_from_json()found the status value"error"before the"error"key and returned an empty error message for every failed ack.\",\nand Go's\u003creached callers as literal backslash sequences.A key is now a quoted name followed by a colon, and string values are decoded as they are copied.
Type of Change
Changes
sdk/c/src/eipc_json.c:json_find_value(): it walks thestrstr()matches of"key"and accepts the first one followed (after optional whitespace) by:.json_find_string,json_find_uint64andjson_find_floatall use it, so the three copies of the lookup are now one.json_find_stringdecodes\" \\ \/ \b \f \n \r \tand\uXXXXwhile copying.\uXXXXbecomes UTF-8, surrogate pairs are combined, and a malformed or unpaired escape becomes U+FFFD.\u0000also becomes U+FFFD, because a NUL would cut the C string short. Values longer than the output buffer are still truncated, but a decoded\uXXXXis never cut in half.json_hex4()andjson_utf8().sdk/c/tests/test_chat_json.c: two new cases.test_json_key_not_matched_in_value: an ack with status"error", and a header whoseservice_idis"priority".test_json_string_escapes_decoded: ack and intent round trips with quotes, a backslash, a newline and a tab, plus a chat response containing\u003c,\u0026,\u00e9,\/and a surrogate pair, and an intent containing\u0000, which must decode to U+FFFD.Testing
ctest --test-dir sdk/c/build --output-on-failure): 5/5eipc_json.cmacOS arm64, Apple clang 21.0.0, CMake 4.4.2, go1.26.10. Base
741690e, headcacadc3.git checkout origin/master -- sdk/c/src/eipc_json.c(keeping the new tests),cmake --build sdk/c/build && ./sdk/c/build/tests/test_chat_jsontest_json_key_not_matched_in_value ... FAIL: error message after an "error" status,test_json_string_escapes_decoded ... FAIL: ack error round-trips,3/5 passed, 2 failed, exit 1cacadc35/5 passed, 0 failed, exit 0cmake -S sdk/c -B sdk/c/build -DEIPC_BUILD_TESTS=ON && cmake --build sdk/c/build && ctest --test-dir sdk/c/build --output-on-failure100% tests passed out of 5-DCMAKE_C_FLAGS="-Wall -Wextra -Werror"go build ./...go vet ./...gofmt -l .go test -race -count=1 -p 1 ./...(CI has no-p 1; it is used here to run one package at a time)make build-allPre-Submission Checklist
-Wall -Wextra -Werrorfor C)<type>(<scope>): <description>convention741690e)Closing issue
Fixes #64
Screenshots / Logs
Raw logs:
raw-fail-before.log,raw-pass-after.log,raw-ctest.log,raw-ci.log.Additional Notes
sdk/c) is not built by any workflow, so its tests run only locally. The table above shows that run.Summary by Sourcery
Fix C SDK JSON parsing to reliably locate object fields and return decoded string values.
Bug Fixes:
Enhancements:
Tests: