Skip to content

MDEV-39813 ST_GeomFromGeoJSON does not control recursion depth#5379

Open
grooverdan wants to merge 1 commit into
MariaDB:12.3from
grooverdan:MDEV-39813_12.3
Open

MDEV-39813 ST_GeomFromGeoJSON does not control recursion depth#5379
grooverdan wants to merge 1 commit into
MariaDB:12.3from
grooverdan:MDEV-39813_12.3

Conversation

@grooverdan

Copy link
Copy Markdown
Member

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.

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread sql/spatial.cc
Comment on lines +590 to +591
if (check_stack_overrun(current_thd, STACK_MIN_SIZE , NULL))
return NULL;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. Avoid calling thread-local lookups like current_thd or thd_to_trx in performance-critical paths (such as per-record operations or frequent lookups). Instead, pass down or cache the transaction pointer (trx_t) to reduce overhead.

@gkodinov gkodinov added the MariaDB Foundation Pull requests created by MariaDB Foundation label Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

MariaDB Foundation Pull requests created by MariaDB Foundation

Development

Successfully merging this pull request may close these issues.

3 participants