Conversation
008c235 to
769cfaf
Compare
769cfaf to
8058cb5
Compare
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 4 total — 1 posted, 3 skipped
Posted findings
- [Medium] Node leaked in dict_clean early error return —
src/json/centijson_value.c:1797-1809
Skipped findings
- [Medium] Test coverage gap for duration overflow checks
- [Medium] Test coverage gap for lock timeout recovery thread accounting
- [Low] Initializing this_zerospan_offset to 0 is correct but the variable could use a comment
Review generated by Skoll via openclaw
| #endif | ||
| &node->key); | ||
| if (ret < 0) | ||
| if (ret < 0) { |
There was a problem hiding this comment.
🟡 [Medium] Node leaked in dict_clean early error return
💡 SUGGEST bug
The new error-return paths correctly free the stack, but the node that was just popped off the stack (line 1789) is never freed when json_value_fini fails. After node = stack[--stack_size], if either json_value_fini(&node->key) or json_value_fini(&node->json_value) fails, the code does free((void *)stack); return ret; but skips free(node). This leaks the node. Additionally, remaining tree nodes referenced through right and the rest of the stack are also leaked, but that's harder to address.
Suggestion:
| if (ret < 0) { | |
| if (ret < 0) { | |
| free(node); | |
| free((void *)stack); | |
| return ret; | |
| } |
There was a problem hiding this comment.
Added free(node) before free((void *)stack) in both error returns.
b0cb0f3 to
3b71fba
Compare
3b71fba to
c41cdec
Compare
Fixes F-968, F-969, F-970, F-972, F-973, F-974, F-975, F-976, F-977, F-978, F-985, F-986, F-1204, F-1205, F-1206, F-1207, F-1208, F-1209, F-1213, F-1651, F-1652, F-1653, F-1654, F-1655, F-1666, F-1667, F-1668, F-1670, F-1675