Skip to content

Fix #880 Max recursion depth for cJSON_Duplicate to prevent stack exhaustion#888

Merged
Alanscut merged 2 commits intoDaveGamble:masterfrom
vwvw:circular_reference
Sep 23, 2024
Merged

Fix #880 Max recursion depth for cJSON_Duplicate to prevent stack exhaustion#888
Alanscut merged 2 commits intoDaveGamble:masterfrom
vwvw:circular_reference

Conversation

@vwvw
Copy link
Contributor

@vwvw vwvw commented Aug 30, 2024

Dear maintainers,

Here is a suggestion of a fix to prevent the stack exhaustion in case of circular reference.

I duplicated the cJSON_Duplicate so that it could take a depth argument. This one is compared to CJSON_CIRCULAR_LIMIT which is currently set to 10'000 but likely need adaption based on your knowledge of what is use in practice.

I also added a test under cjson_should_not_follow_too_deep_circular_references.

If you don't like the extra function cJSON_Duplicate_rec, this fix could also be implemented through an additional field in the cJSON struct (likely break the ABI?) or through a global variable (making cJSON more thread unsafe).

What is not yet fixed

In addition to cJSON_Duplicate, the same issue happens in cJSON_Delete. I have a similar fix for it too but I'm unsure on what to do once we reach CJSON_CIRCULAR_LIMIT. Aborting or exiting looks like the correct solution as it is likely that memory corruption occurred through a double free.

What would you suggest to do in this case?

@vwvw vwvw marked this pull request as ready for review September 3, 2024 10:05
@vwvw
Copy link
Contributor Author

vwvw commented Sep 3, 2024

This would fix #880

@vwvw vwvw changed the title Fix #880 Max recusrion depth for cJSON_Duplicate to prevent stack exhaustion Fix #880 Max recursion depth for cJSON_Duplicate to prevent stack exhaustion Sep 4, 2024
@PeterAlfredLee
Copy link
Contributor

Looks good to me for now.

This PR changes the original behavior of cJSON_Duplicate. For nested objects with a depth of over 10,000(which is deep enough IMO), cJSON_Duplicate would stop.
If this makes any inconvenience to anyone, feel free to tell me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants