Conversation
The JSON encoder is recursive, and it's far from easy to make it iterative. Add a cheap stack limit check to prevent a segfault. This uses the PHP_JSON_ERROR_DEPTH error code that already talks about the stack depth. Previously this was only used for the $depth argument.
| HashTable *myht, *prop_ht; | ||
|
|
||
| if (php_json_check_stack_limit()) { | ||
| encoder->error_code = PHP_JSON_ERROR_DEPTH; |
There was a problem hiding this comment.
If it's not desirable to reuse the PHP_JSON_ERROR_DEPTH error code, then I can add a new one; but then this PR won't be able to target 8.3.
There was a problem hiding this comment.
Given the message associated with this error code, I think it's fine
There was a problem hiding this comment.
Ok I will give some time for Jakub to check this PR too so we know if he agrees because he is codeowner of json.
There was a problem hiding this comment.
It's for a bit different thing but guess we can repurpose it.
| HashTable *myht, *prop_ht; | ||
|
|
||
| if (php_json_check_stack_limit()) { | ||
| encoder->error_code = PHP_JSON_ERROR_DEPTH; |
There was a problem hiding this comment.
Given the message associated with this error code, I think it's fine
bukka
left a comment
There was a problem hiding this comment.
LGTM, just one NIT but feel free to ignore it.
| static zend_always_inline bool php_json_check_stack_limit(void) | ||
| { | ||
| #ifdef ZEND_CHECK_STACK_LIMIT | ||
| return zend_call_stack_overflowed(EG(stack_limit)); | ||
| #else | ||
| return false; | ||
| #endif | ||
| } | ||
|
|
There was a problem hiding this comment.
Wouldn't make sense to make this not json specific as it might be useful for serialization as well possibly?
There was a problem hiding this comment.
Maybe this should be moved to the Zend/zend_call_stack.h header file, I think it would make sense there. What do you think @arnaud-lb ?
There was a problem hiding this comment.
Agree, it would make sense to move this function to Zend/zend_call_stack.h
There was a problem hiding this comment.
It's a bit annoying to do so as it would require us to pull in zend_globals.h to get access to EG fields, which pulls in a whole bunch more..
| HashTable *myht, *prop_ht; | ||
|
|
||
| if (php_json_check_stack_limit()) { | ||
| encoder->error_code = PHP_JSON_ERROR_DEPTH; |
There was a problem hiding this comment.
It's for a bit different thing but guess we can repurpose it.
The JSON encoder is recursive, and it's far from easy to make it iterative. Add a cheap stack limit check to prevent a segfault. This uses the PHP_JSON_ERROR_DEPTH error code that already talks about the stack depth. Previously this was only used for the $depth argument. Closes phpGH-16059.
The JSON encoder is recursive, and it's far from easy to make it iterative. Add a cheap stack limit check to prevent a segfault. This uses the PHP_JSON_ERROR_DEPTH error code that already talks about the stack depth. Previously this was only used for the $depth argument.