MDEV-39813 ST_GeomFromGeoJSON does not control recursion depth#5379
MDEV-39813 ST_GeomFromGeoJSON does not control recursion depth#5379grooverdan wants to merge 1 commit into
Conversation
Using stack_p wasn't a portable concept in 12.3 when JSON parsing got unlimited depth. To let ST_GeomFromGeoJSON as a recursive function, needed because object order of "type" may be after the "geometries", some stack checking was required. Use the check_stack_depth function to allow excessively deep GeoJSON objects to error.
There was a problem hiding this comment.
Code Review
This pull request re-enables the gis-json test and addresses a potential infinite recursion issue in Geometry::create_from_json by adding a stack overrun check using check_stack_overrun. Unused variables related to the JSON engine's stack pointer were also cleaned up. The review feedback recommends refactoring Geometry::create_from_json to accept THD *thd as a parameter rather than calling current_thd internally, which avoids costly thread-local lookups during recursive execution.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (check_stack_overrun(current_thd, STACK_MIN_SIZE , NULL)) | ||
| return NULL; |
There was a problem hiding this comment.
Calling current_thd (which performs a thread-local lookup) inside the recursive function Geometry::create_from_json can introduce significant performance overhead, especially since this function is executed per-record and recursively for nested JSON structures.
According to the project's performance guidelines, thread-local lookups should be avoided in performance-critical paths.
Recommendation
Consider refactoring Geometry::create_from_json to accept THD *thd as its first parameter:
static Geometry *create_from_json(THD *thd,
Geometry_buffer *buffer,
json_engine_t *je,
bool er_on_3D,
String *res);This allows the caller (e.g., Item_func_geomfromgeojson::val_str) to retrieve current_thd once and pass it down, avoiding repeated thread-local lookups during recursion.
Additionally, there is an extra space before the comma in STACK_MIN_SIZE , NULL which has been cleaned up in the suggestion below.
if (check_stack_overrun(thd, STACK_MIN_SIZE, NULL))
return NULL;References
- Avoid calling thread-local lookups like
current_thdorthd_to_trxin performance-critical paths (such as per-record operations or frequent lookups). Instead, pass down or cache the transaction pointer (trx_t) to reduce overhead.
Using stack_p wasn't a portable concept in 12.3 when JSON parsing got unlimited depth. To let ST_GeomFromGeoJSON as a recursive function, needed because object order of "type" may be after the "geometries", some stack checking was required.
Use the check_stack_depth function to allow excessively deep GeoJSON objects to error.
@mariadb-RuchaDeodhar , I'm after a concept approval. To run reliably I'm going to move this test to the thread_stack_overun.test that @vaintroub added in 10.11 (#5320) that hasn't yet been merged up.
The 10.6/10.11 fix for this bug is going to change the stack_p, killed_ptr and json_scan_start but what is clear is that 12.3 requires a stack overrun check.