MDEV-28404 JSON functions don't respect KILL QUERY#5117
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to make JSON processing interruptible by checking a killed_ptr within the JSON engine and returning a new JE_KILLED error code. While the changes integrate this check into several JSON functions and update error reporting, the review identifies a critical risk of NULL pointer dereference in json_scan_next if the pointer is not initialized. Additionally, feedback points out that some functions incorrectly assume they are not interruptible or have lost interruptibility due to the removal of manual check_killed() calls without proper pointer initialization.
| if ((null_value= args[0]->null_value)) | ||
| return 0; | ||
|
|
||
| /* no json_scan_next called so not interuptable */ |
There was a problem hiding this comment.
This comment is incorrect. Item_func_json_unquote::read_json returns the engine to val_str, which then calls json_read_value(&je). Since json_read_value internally uses json_scan_next, this path is indeed interruptible and je.killed_ptr must be initialized to avoid a crash (due to the missing NULL check in json_lib.c) or a lack of interruptibility.
There was a problem hiding this comment.
/gemini - I'm pretty sure that json_read_value doesn't call json_scan_next.
There was a problem hiding this comment.
You are correct that Item_func_json_unquote::read_json itself does not call json_scan_next (which is why the comment you added there is accurate). However, json_read_value is the core function used to parse JSON values, and it internally iterates through tokens by calling json_scan_next. Because json_read_value is used in many other functions (like Item_func_json_contains::val_bool), those functions become interruptible once je.killed_ptr is initialized. If you look at the implementation of json_read_value in strings/json_lib.c, you will see the while ((t= json_scan_next(j)) > 0) loop that performs the parsing. Therefore, initializing killed_ptr before calling json_read_value is necessary to ensure those functions respect the KILL signal.
b080842 to
f80774b
Compare
Or max_statement_time limit. Implement interuptability of: * JSON_CONTAINS * JSON_CONTAINS_PATH * JSON_EXISTS * JSON_EXTRACT * JSON_KEYS * JSON_OVERLAPS * ST_GeomFromGeoJSON Populate in the json library the killed as a specific error message. As the json library has a killed_ptr is approprate it handles it. As json_report_error now processes the killed there's no need of thd->check_killed(). Verified that the path after every json_scan_next, the location of checking the killed_ptr, reports errors correctly. Correct JSON error reporting on JSON_EXISTS.
Or max_statement_time limit.
Implement interuptability of:
Populate in the json library the killed as a specific error message. As the json library has a killed_ptr is approprate it handles it.
As json_report_error now processes the killed there's no need of thd->check_killed().
Verified that the path after every json_scan_next, the location of checking the killed_ptr, reports errors correctly.