Skip to content

MDEV-39813 ST_GeomFromGeoJSON does not control recursion depth #5379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
grooverdan wants to merge 1 commit into
base: 12.3
Choose a base branch
from
Open

MDEV-39813 ST_GeomFromGeoJSON does not control recursion depth #5379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion mysql-test/main/disabled.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,3 @@ mysql_embedded : Bug#12561297 2011-05-14 Anitha Dependent on PB2 chang
file_contents : MDEV-6526 these files are not installed anymore
max_statement_time : cannot possibly work, depends on timing
partition_open_files_limit : open_files_limit check broken by MDEV-18360
gis-json : MDEV-39813 infinite recursion
5 changes: 1 addition & 4 deletions mysql-test/main/gis-json.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,7 @@ repeat('{"type":"GeometryCollection","geometries":[', 2000),
'{"type":"Point","coordinates":[0,0]}',
repeat(']}', 2000)
)) as exp;
exp
NULL
Warnings:
Warning 4040 Limit of 32 on JSON nested structures depth is reached in argument 1 to function 'st_geomfromgeojson' at position 473
ERROR HY000: Thread stack overrun: # bytes used of a # byte stack, and # bytes needed. Consider increasing the thread_stack system variable.
# End of 10.6 tests
#
# MDEV-34079: ST_AsGeoJSON returns incorrect value for empty geometry
Expand Down
3 changes: 3 additions & 0 deletions mysql-test/main/gis-json.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ SELECT ST_ASTEXT(ST_GEOMFROMGEOJSON('{"type": ["POINT"], "coINates": [0,0] }'))
--echo # MDEV-39813 ST_GeomFromGeoJSON does not control recursion depth
--echo #

--replace_regex /[0-9]+ bytes used of a [0-9]+ byte stack, and [0-9]+ bytes needed/# bytes used of a # byte stack, and # bytes needed/

--error ER_STACK_OVERRUN_NEED_MORE
SELECT ST_GeomFromGeoJSON(
concat(
repeat('{"type":"GeometryCollection","geometries":[', 2000),
Expand Down
9 changes: 4 additions & 5 deletions sql/spatial.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "spatial.h"
#include "gstream.h" // Gis_read_stream
#include "sql_string.h" // String
#include "sql_parse.h" // check_stack_overrun
#include <vector>

/* This is from item_func.h. Didn't want to #include the whole file. */
Expand Down Expand Up @@ -586,8 +587,10 @@ Geometry *Geometry::create_from_json(Geometry_buffer *buffer,
uint key_len;
int fcoll_type_found= 0, feature_type_found= 0;

if (check_stack_overrun(current_thd, STACK_MIN_SIZE , NULL))
return NULL;
Comment on lines +590 to +591

@gemini-code-assist gemini-code-assist Bot Jul 13, 2026

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.


const uint32_t *killed_ptr= (uint32_t *) je->killed_ptr;
int stack_p;

if (json_read_value(je))
goto err_return;
Expand Down Expand Up @@ -753,10 +756,8 @@ Geometry *Geometry::create_from_json(Geometry_buffer *buffer,

create_geom:

stack_p= je->stack_p;
json_scan_start(je, je->s.cs, coord_start, je->s.str_end);
je->killed_ptr= killed_ptr;
je->stack_p= stack_p;

if (res->reserve(1 + 4, 512))
goto err_return;
Expand All @@ -770,10 +771,8 @@ Geometry *Geometry::create_from_json(Geometry_buffer *buffer,
return result;

handle_geometry_key:
stack_p= je->stack_p;
json_scan_start(je, je->s.cs, geometry_start, je->s.str_end);
je->killed_ptr= killed_ptr;
je->stack_p= stack_p;
return create_from_json(buffer, je, er_on_3D, res);

err_return:
Expand Down