Code Review found 2 critical bugs

[cpsource]

A code audit of cJSON v1.7.19 against upstream master surfaced six findings that do not appear to have existing tickets after cross-referencing the issue tracker and open PRs. Two are critical and reachable from the public API on attacker-supplied input; the other four are lower-severity and grouped at the end for context.

A summary of previously-known findings this audit also confirmed (so you know I checked the tracker and am not re-filing) is at the bottom.

---

Critical #1 — NULL dereference on cJSON_malloc failure in cJSON_Utils.c (three call sites)

Three public-API-reachable code paths allocate via cJSON_malloc and use the result with no NULL check. cJSON_malloc delegates to the user's malloc_fn hook, which is trivially returnable as NULL, so on OOM / memory pressure the process segfaults.

Call sites

1. compose_patch — cJSON_Utils.c:1120

unsigned char *full_path = (unsigned char*)cJSON_malloc(path_length + suffix_length + sizeof(\"/\"));
sprintf((char*)full_path, \"%s/\", (const char*)path);   // line 1122: deref of possibly-NULL full_path
encode_string_as_pointer(full_path + path_length + 1, suffix);

Reachable via the public cJSONUtils_AddPatchToArray.

2. create_patches — cJSON_Utils.c:1175 (array branch) and :1247 (object branch)

unsigned char *new_path = (unsigned char*)cJSON_malloc(strlen((const char*)path) + 20 + sizeof(\"/\"));
// no NULL check
// ...
sprintf((char*)new_path, \"%s/%lu\", path, (unsigned long)index);   // line 1188: deref

Reachable via cJSONUtils_GeneratePatches / cJSONUtils_GeneratePatchesCaseSensitive.

3. cJSONUtils_FindPointerFromObjectTo — cJSON_Utils.c:224 (array branch) and :242 (object branch)

unsigned char *full_pointer = (unsigned char*)cJSON_malloc(strlen((char*)target_pointer) + 20 + sizeof(\"/\"));
if (child_index > ULONG_MAX) { ... }   // checks the index, not full_pointer
sprintf((char*)full_pointer, \"/%lu%s\", ...);   // line 234: deref

The :242 branch is worse: the very next line (full_pointer[0] = '/'; at :243) is an immediate deref, and on this failure path target_pointer (already allocated by the recursive call) is also leaked.

Suggested fix

if (full_path == NULL) {
    /* free any already-allocated siblings, then bail */
    return;        /* or return NULL, depending on the function */
}

For FindPointerFromObjectTo, the fix additionally needs to cJSON_free(target_pointer) before returning — this also closes the long-standing leak originally reported in #414 (closed but still present on master at line 224).

Verification

No test in tests/*.c exercises malloc failure via cJSON_InitHooks; these paths are unverified under OOM but read as straightforward NULL-check omissions.

---

Critical #2 — apply_patch move op missing the RFC 6902 §4.4 descendant check

apply_patch in cJSON_Utils.c:905-940 implements the move op as detach_path(from) followed by an add at path, with no check that path is not a descendant of from. RFC 6902 §4.4 explicitly forbids this:

The "from" location MUST NOT be a proper prefix of the "path" location; i.e., a location cannot be moved into one of its children.

Reproducer

Starting document: {\"a\":{\"b\":{}}}
Patch: [{\"op\":\"move\",\"from\":\"/a\",\"path\":\"/a/b\"}]

Current behaviour:

1. detach_path(\"/a\") succeeds — removes /a from the document.
2. The subsequent lookup of the parent /a/b fails (/a no longer exists).
3. apply_patch returns error status 9, but the document has already been mutated — a is gone.

Per RFC the patch should be rejected before any mutation. This is both an RFC conformance bug and a data-loss path for any caller that trusts the error return (the earlier ops in a multi-op patch are not rolled back; this is adjacent to but distinct from the atomicity concern in #997).

Suggested fix

Before the detach_path(from) call at line 918, validate:

if (opcode == MOVE) {
    const char *f = from->valuestring;
    const char *p = path->valuestring;
    size_t f_len = strlen(f);
    if (strncmp(p, f, f_len) == 0 && (p[f_len] == '/' || p[f_len] == '\0')) {
        status = 14;   /* or whichever distinct code */
        goto cleanup;
    }
}

Verification

No test in tests/json-patch-tests/*.json (spec_tests.json, tests.json, or cjson-utils-tests.json) exercises move-into-descendant — grep for "descendant" or "prefix" in that directory returns no matches.

---

Other findings not currently in the tracker

Reported briefly; happy to open separate issues if preferred.

Major — generate_merge_patch case-sensitivity inconsistency (cJSON_Utils.c:1423)

Lines 1406-1407 sort both objects with sort_object(..., case_sensitive). The subsequent merge loop at line 1423 compares keys with plain strcmp, ignoring the flag:

diff = strcmp(from_child->string, to_child->string);

When case_sensitive == false and two keys differ only in case (e.g. \"Foo\" vs \"foo\"), sort_object places them adjacent but strcmp reports them as different, producing a spurious add + remove pair in the patch. The sibling code in create_patches at line 1238 gets this right — it uses compare_strings(..., case_sensitive). Fix: use the same helper here.

Minor — ensure() size_t overflow on 32-bit (cJSON.c:512)

if (needed > INT_MAX) { return NULL; }
needed += p->offset + 1;    /* can overflow size_t when p->offset is near SIZE_MAX */

Unreachable on 64-bit (you would need > 2 GB already buffered); a theoretical hazard on 32-bit with caller-supplied large buffers. Guard: if (p->offset > SIZE_MAX - 1 - needed) return NULL; before the add.

Minor — cJSON_ReplaceItemInArray silent out-of-range (cJSON.c:2419-2427)

When which exceeds array length, get_array_item returns NULL and cJSON_ReplaceItemViaPointer at line 2370 correctly rejects the call. Not a crash, but the single false return is indistinguishable from NULL-arg / allocation failures. Consider documenting the out-of-range contract, or splitting the status.

Minor — parse_hex4 dual-meaning return (cJSON.c:666-699)

Returns 0 both for the valid sequence 0000 and for any invalid hex digit. Callers (e.g. utf16_literal_to_utf8) disambiguate by pointer position. A comment on the function explaining the dual meaning would help the next reader.

---

Findings already tracked (confirmed on master, not re-filing)

To close the loop — I also verified these on current master and will not re-file them:

• decode_pointer_inplace ~1 off-by-one — [cJSON_Utils] decode_pointer_inplace off-by-one causes incorrect decoding of "~1" (JSON Pointer) #977, PR fix: correct ~1 decode in decode_pointer_inplace() #1001, PR fix decode_pointer_inplace to handle escape sequences correctly #972, PR fix: off-by-one in decode_pointer_inplace for ~1 escape sequence #998, PR Fix decode_pointer_inplace ~1 escape and minify_string escape handling #993 (confirmed with a standalone reproducer: a~1b decodes to a~/)
• Integer overflow in decode_array_index_from_pointer — PR fix: add overflow check in decode_array_index_from_pointer() #1002
• cJSON_Compare O(n²) on objects — PR Optimize object comparison #971
• cJSON_CreateNumber(NaN) UB via (int)num cast — Undefined-Behavior (Float-Cast-Overflow) in cJSON_CreateNumber via NaN values #999, PR Add nan and inf flot number check to avoid integer overflow #1000
• cJSON_Minify NUL-termination / escape handling — PR Add a check in cJSON_Minify #967, PR fix: handle \\ escape sequence in minify_string() #1003
• cJSON_Delete recursion depth (no guard analogous to cJSON_Duplicate's CJSON_CIRCULAR_LIMIT) — related to Double Free Vulnerability Discovered in cJSON_Delete Function Through Fuzzing #833 and [Security] Stack Overflow via Uncontrolled Recursion in cJSONUtils_MergePatch #995

Happy to expand any of the above into a focused PR if helpful; wanted to get the critical pair in front of maintainers first.