MDEV-40185/MDEV-40185 JSON_ARRAY_INTERSECT and JSON_OBJECT_FILTER_KEYS#5420
Open
grooverdan wants to merge 3 commits into
Open
MDEV-40185/MDEV-40185 JSON_ARRAY_INTERSECT and JSON_OBJECT_FILTER_KEYS#5420grooverdan wants to merge 3 commits into
grooverdan wants to merge 3 commits into
Conversation
JSON_OBJECT_FILTER_KEYS was only written to handle
a constant array argument. The handing of arguments in
Item_func_json_object_filter_keys::fix_length_and_dec was
incorrect as ::val_str was called with different
values as it iterates over table for instance.
Moved implementation into Item_func_json_object_filter_keys::val_str.
Preserved the caching of the arg2, (array of keys) when it was
a constant by making create_hash reset the item list if it was already
initialised.
Adjusted filter_keys use the arg provided as output and
avoid inline mallocs.
Added error handing in filter_keys.
The JSON_OBJECT_FILTER_KEYS also now respects max_statement_time
and is killable with KILL {queryid}.
The added error handing inthe previous commit made JSON_OBJECT_FILTER_KEYS interuptable with KILL QUERY and respect the max_statement_time.
JSON_ARRAY_INTERSECT when passed arrays out of tables returned incorrect results. Corrected the caching to all occur within val_str() so that non-constant arguments get parsed corrected. Like previous implementation, if arg1 is a constant take care not to reprocess this. When JSON_ARRAY_INTERSECT arguments are swapped to make const the first arg, errors are reported against the wrong argument. OOM errors of create_or_reset_hash aren't reported as errors. Renamed create_hash to create_or_reset_hash to better reflect its function. Non-array arguments to this function are reported as syntax warnings. Remove unnecessary memory allocation/copy in Item_func_json_array_intersect::get_intersect_between_arrays Adjust tests to show the normalization comparion that occurs.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the JSON_ARRAY_INTERSECT and JSON_OBJECT_FILTER_KEYS functions to support non-constant arguments. However, the current implementation introduces several critical issues. Specifically, moving temporary string buffers from class members to local variables causes use-after-free and dangling pointer bugs when reusing hashes. Additionally, the use of goto statements jumping over local variable initializations leads to compilation errors, and checking null_value during fix_length_and_dec or before evaluating arguments results in stale checks and incorrect metadata. The feedback provides detailed restructuring suggestions to resolve these issues safely.
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 on lines
938
to
+941
| HASH items, seen; | ||
| MEM_ROOT hash_root; | ||
| bool parse_for_each_row, is_array; | ||
| bool item_hash_inited, seen_hash_inited, root_inited; | ||
| bool swapped; |
Contributor
There was a problem hiding this comment.
The temporary string buffers tmp_js1 and tmp_js2 must be kept as class members. Since the hash table items stores pointers directly into the evaluated JSON string buffers to avoid copying, making these buffers local to val_str causes them to be destroyed at the end of the function call. On subsequent rows (when the arguments are constant and the hash is reused), this leads to critical use-after-free and dangling pointer bugs.
String tmp_js1, tmp_js2;
HASH items, seen;
MEM_ROOT hash_root;
bool item_hash_inited, seen_hash_inited, root_inited;
bool swapped;
Comment on lines
974
to
977
| protected: | ||
| String tmp_js1, tmp_js2; | ||
| bool hash_inited, root_inited; | ||
| HASH items; | ||
| MEM_ROOT hash_root; |
Contributor
There was a problem hiding this comment.
Keep tmp_js1 and tmp_js2 as class members to prevent dangling pointers in the items hash table on subsequent rows when the second argument is constant.
String tmp_js1, tmp_js2;
bool hash_inited, root_inited;
HASH items;
MEM_ROOT hash_root;
Comment on lines
+5558
to
5666
| if (args[0]->null_value || args[1]->null_value) | ||
| goto null_return; | ||
|
|
||
| if (parse_for_each_row) | ||
| thd= current_thd; | ||
| /* | ||
| Scan value uses the hash table to get the intersection of two arrays. | ||
| */ | ||
| if (!seen_hash_inited) | ||
| { | ||
| if (args[0]->null_value) | ||
| if (my_hash_init(PSI_INSTRUMENT_ME, &seen, nullptr, 0, 0, 0, | ||
| get_key_name, NULL, 0)) | ||
| { | ||
| err_oom: | ||
| my_error(ER_OUT_OF_RESOURCES, MYF(0)); | ||
| goto null_return; | ||
| } | ||
| seen_hash_inited= true; | ||
| init_alloc_root(PSI_NOT_INSTRUMENTED, &hash_root, 1024, 0, MYF(0)); | ||
| root_inited= true; | ||
| new_arg1: | ||
| String tmp_js1; | ||
| String *js1= args[0]->val_json(&tmp_js1); | ||
| json_engine_t je1; | ||
| if (!js1) | ||
| goto null_return; | ||
| if (item_hash_inited) | ||
| my_hash_free(&items); | ||
| if (seen_hash_inited) | ||
| my_hash_free(&seen); | ||
| if (root_inited) | ||
| free_root(&hash_root, MYF(0)); | ||
| root_inited= false; | ||
| item_hash_inited= false; | ||
| prepare_json_and_create_hash(&je1, js1); | ||
| } | ||
|
|
||
| if (!is_array || args[1]->null_value) | ||
| goto null_return; | ||
| json_scan_start(&je1, js1->charset(), (const uchar *) js1->ptr(), | ||
| (const uchar *) js1->ptr() + js1->length()); | ||
| je1.killed_ptr= (uint32_t *) &thd->killed; | ||
|
|
||
| str->set_charset(js2->charset()); | ||
| str->length(0); | ||
| if (json_read_value(&je1)) | ||
| goto je1_error_return; | ||
| if (je1.value_type != JSON_VALUE_ARRAY) | ||
| { | ||
| je1.s.error= JE_SYN; | ||
| je1_error_return: | ||
| report_json_error(js1, &je1, swapped ? 0 : 1); | ||
| goto null_return; | ||
| } | ||
| if (create_or_reset_hash(&je1, &items, item_hash_inited, &hash_root)) | ||
| { | ||
| if (je1.s.error) | ||
| goto je1_error_return; | ||
| else | ||
| goto err_oom; | ||
| } | ||
| seen.charset= je1.s.cs; | ||
| } | ||
| else if (!args[0]->const_item()) | ||
| goto new_arg1; | ||
|
|
||
| thd= current_thd; | ||
| JSON_DO_PAUSE_EXECUTION(thd, 0.0002); | ||
| json_scan_start(&je2, js2->charset(), (const uchar *) js2->ptr(), | ||
| (const uchar *) js2->ptr() + js2->length()); | ||
| je2.killed_ptr= (uint32_t *) &thd->killed; | ||
| { | ||
| /* arg2 */ | ||
| String tmp_js2; | ||
| String *js2= args[1]->val_json(&tmp_js2); | ||
| if (!js2) | ||
| goto null_return; | ||
| json_engine_t je2; | ||
|
|
||
| JSON_DO_PAUSE_EXECUTION(thd, 0.0002); | ||
| json_scan_start(&je2, js2->charset(), (const uchar *) js2->ptr(), | ||
| (const uchar *) js2->ptr() + js2->length()); | ||
| je2.killed_ptr= (uint32_t *) &thd->killed; | ||
|
|
||
| if (json_read_value(&je2)) | ||
| goto je2_error_return; | ||
| if (json_read_value(&je2)) | ||
| goto je2_error_return; | ||
|
|
||
| if (je2.value_type != JSON_VALUE_ARRAY) | ||
| { | ||
| je2.s.error= JE_SYN; | ||
| goto je2_error_return; | ||
| } | ||
| if (je2.value_type != JSON_VALUE_ARRAY) | ||
| { | ||
| je2.s.error= JE_SYN; | ||
| je2_error_return: | ||
| report_json_error(js2, &je2, swapped ? 0 : 1); | ||
| goto null_return; | ||
| } | ||
|
|
||
| if (get_intersect_between_arrays(str, &je2, &items, &seen)) | ||
| goto error_return; | ||
| { | ||
| /* result */ | ||
| String res; | ||
| res.set_charset(js2->charset()); | ||
| if (get_intersect_between_arrays(&res, &je2, &items, &seen)) | ||
| { | ||
| if (je2.s.error) | ||
| goto je2_error_return; | ||
| goto err_oom; | ||
| } | ||
|
|
||
| if (str->length()) | ||
| { | ||
| json_scan_start(&res_je, str->charset(), (const uchar *) str->ptr(), | ||
| (const uchar *) str->ptr() + str->length()); | ||
| res_je.killed_ptr= (uint32_t *) &thd->killed; | ||
| str= &tmp_js1; | ||
| if (json_nice(&res_je, str, Item_func_json_format::LOOSE)) | ||
| goto error_return; | ||
| json_engine_t res_je; | ||
| /* and make output nice */ | ||
| if (res.length()) | ||
| { | ||
| json_scan_start(&res_je, res.charset(), (const uchar *) res.ptr(), | ||
| (const uchar *) res.ptr() + res.length()); | ||
| res_je.killed_ptr= (uint32_t *) &thd->killed; | ||
|
|
||
| null_value= 0; | ||
| return str; | ||
| } | ||
| else | ||
| { | ||
| goto null_return; | ||
| str->set_charset(js2->charset()); | ||
| str->length(0); | ||
| if (json_nice(&res_je, str, Item_func_json_format::LOOSE)) | ||
| { | ||
| report_json_error(str, &res_je, 0); | ||
| goto null_return; | ||
| } | ||
| null_value= 0; | ||
| return str; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| error_return: | ||
| if (je2.s.error) | ||
| je2_error_return: | ||
| report_json_error(js2, &je2, 1); | ||
| null_return: | ||
| null_value= 1; | ||
| return NULL; |
Contributor
There was a problem hiding this comment.
This implementation contains several critical issues:
- Stale
null_valueCheck: Checkingargs[i]->null_valuebefore callingval_json()checks the stale NULL status from the previous row, leading to incorrect query results. - Compilation Error: The
goto new_arg1;statement jumps into theif (!seen_hash_inited)block, bypassing the initialization of local variables with non-trivial constructors (likeString tmp_js1), which is illegal in C++. - Dangling Pointers: Using local
tmp_js1andtmp_js2variables causes use-after-free bugs on subsequent rows when arguments are constant. - Incorrect Argument Index: In
je1_error_return, the argument index is reported asswapped ? 0 : 1, which is inverted and causes syntax errors in the first argument to be reported as argument 2 (as seen in the test result changes). - Redundant
killed_ptrInitialization:json_read_valueis not interruptible and does not calljson_scan_nextinternally, so initializingkilled_ptrforje1andje2is redundant.
We can resolve all these issues cleanly by keeping tmp_js1 and tmp_js2 as class members, restructuring the control flow to avoid goto entirely, and correctly mapping the argument index.
thd= current_thd;
if (seen_hash_inited && args[0]->null_value)
goto null_return;
if (!seen_hash_inited || !args[0]->const_item())
{
if (!seen_hash_inited)
{
if (my_hash_init(PSI_INSTRUMENT_ME, &seen, nullptr, 0, 0, 0,
get_key_name, NULL, 0))
{
err_oom:
my_error(ER_OUT_OF_RESOURCES, MYF(0));
goto null_return;
}
seen_hash_inited= true;
init_alloc_root(PSI_NOT_INSTRUMENTED, &hash_root, 1024, 0, MYF(0));
root_inited= true;
}
String *js1= args[0]->val_json(&tmp_js1);
if (!js1 || args[0]->null_value)
goto null_return;
json_engine_t je1;
json_scan_start(&je1, js1->charset(), (const uchar *) js1->ptr(),
(const uchar *) js1->ptr() + js1->length());
if (json_read_value(&je1))
goto je1_error_return;
if (je1.value_type != JSON_VALUE_ARRAY)
{
je1.s.error= JE_SYN;
je1_error_return:
report_json_error(js1, &je1, swapped ? 1 : 0);
goto null_return;
}
if (create_or_reset_hash(&je1, &items, item_hash_inited, &hash_root))
{
if (je1.s.error)
goto je1_error_return;
else
goto err_oom;
}
seen.charset= je1.s.cs;
}
{
/* arg2 */
String *js2= args[1]->val_json(&tmp_js2);
if (!js2 || args[1]->null_value)
goto null_return;
json_engine_t je2;
JSON_DO_PAUSE_EXECUTION(thd, 0.0002);
json_scan_start(&je2, js2->charset(), (const uchar *) js2->ptr(),
(const uchar *) js2->ptr() + js2->length());
if (json_read_value(&je2))
goto je2_error_return;
if (je2.value_type != JSON_VALUE_ARRAY)
{
je2.s.error= JE_SYN;
je2_error_return:
report_json_error(js2, &je2, swapped ? 0 : 1);
goto null_return;
}
{
/* result */
String res;
res.set_charset(js2->charset());
if (get_intersect_between_arrays(&res, &je2, &items, &seen))
{
if (je2.s.error)
goto je2_error_return;
goto err_oom;
}
json_engine_t res_je;
/* and make output nice */
if (res.length())
{
json_scan_start(&res_je, res.charset(), (const uchar *) res.ptr(),
(const uchar *) res.ptr() + res.length());
res_je.killed_ptr= (uint32_t *) &thd->killed;
str->set_charset(js2->charset());
str->length(0);
if (json_nice(&res_je, str, Item_func_json_format::LOOSE))
{
report_json_error(str, &res_je, 0);
goto null_return;
}
null_value= 0;
return str;
}
}
}References
- Do not assume that json_read_value internally calls json_scan_next or is interruptible. Only paths that explicitly call json_scan_next (or functions that do) are interruptible and require killed_ptr initialization.
Comment on lines
+5756
to
5848
| if (args[0]->null_value || args[1]->null_value) | ||
| goto null_return; | ||
|
|
||
| str->set_charset(js1->charset()); | ||
| str->length(0); | ||
| thd= current_thd; | ||
| if (!root_inited) | ||
| { | ||
| init_alloc_root(PSI_NOT_INSTRUMENTED, &hash_root, 1024, 0, MYF(0)); | ||
| root_inited= true; | ||
|
|
||
| json_scan_start(&je1, js1->charset(),(const uchar *) js1->ptr(), | ||
| (const uchar *) js1->ptr() + js1->length()); | ||
| new_arg2: | ||
| String tmp_js2; | ||
| String *js2= args[1]->val_json(&tmp_js2); | ||
| if (!js2) | ||
| goto null_return; | ||
| json_engine_t je2; | ||
| json_scan_start(&je2, js2->charset(),(const uchar *) js2->ptr(), | ||
| (const uchar *) js2->ptr() + js2->length()); | ||
| je2.killed_ptr= (uint32_t *) &thd->killed; | ||
| if (json_read_value(&je2)) | ||
| goto je2_error_return; | ||
|
|
||
| if (json_read_value(&je1) || je1.value_type != JSON_VALUE_OBJECT) | ||
| goto error_return; | ||
| if (je2.value_type != JSON_VALUE_ARRAY) | ||
| { | ||
| je2.s.error= JE_SYN; | ||
| je2_error_return: | ||
| report_json_error(js2, &je2, 1); | ||
| goto null_return; | ||
| } | ||
| if (create_or_reset_hash(&je2, &items, hash_inited, &hash_root)) | ||
| { | ||
| if (je2.s.error) | ||
| goto je2_error_return; | ||
| my_error(ER_OUTOFMEMORY, MYF(0), 64); | ||
| } | ||
| } | ||
| else if (!args[1]->const_item()) | ||
| goto new_arg2; | ||
|
|
||
| if(filter_keys(&je1, str, items)) | ||
| goto null_return; | ||
| { | ||
| json_engine_t je1; | ||
| String *js1; | ||
| String tmp_js1; | ||
| js1= args[0]->val_json(&tmp_js1); | ||
|
|
||
| if (str->length()) | ||
| { | ||
| json_scan_start(&res_je, str->charset(), (const uchar *) str->ptr(), | ||
| (const uchar *) str->ptr() + str->length()); | ||
| str= &tmp_js1; | ||
| if (json_nice(&res_je, str, Item_func_json_format::LOOSE)) | ||
| JSON_DO_PAUSE_EXECUTION(thd, 0.0002); | ||
| json_scan_start(&je1, js1->charset(),(const uchar *) js1->ptr(), | ||
| (const uchar *) js1->ptr() + js1->length()); | ||
| je1.killed_ptr= (uint32_t *) &thd->killed; | ||
|
|
||
| if (json_read_value(&je1)) | ||
| goto error_return; | ||
|
|
||
| null_value= 0; | ||
| return str; | ||
| } | ||
| else | ||
| { | ||
| goto null_return; | ||
| } | ||
| if (je1.value_type != JSON_VALUE_OBJECT) | ||
| { | ||
| je1.s.error= JE_SYN; | ||
| error_return: | ||
| report_json_error(js1, &je1, 0); | ||
| goto null_return; | ||
| } | ||
|
|
||
| { | ||
| String fil_res; | ||
| fil_res.set_charset(js1->charset()); | ||
| if (filter_keys(&je1, &fil_res, items)) | ||
| goto error_return; | ||
|
|
||
| if (fil_res.length()) | ||
| { | ||
| json_engine_t res_je; | ||
| json_scan_start(&res_je, fil_res.charset(), (const uchar *) fil_res.ptr(), | ||
| (const uchar *) fil_res.ptr() + fil_res.length()); | ||
| res_je.killed_ptr= (uint32_t *) &thd->killed; | ||
|
|
||
| str->set_charset(js1->charset()); | ||
| str->length(0); | ||
|
|
||
| if (json_nice(&res_je, str, Item_func_json_format::LOOSE)) | ||
| { | ||
| report_json_error(str, &res_je, 0); | ||
| goto null_return; | ||
| } | ||
|
|
||
| null_value= 0; | ||
| return str; | ||
| } | ||
| else | ||
| goto null_return; | ||
| } | ||
| } | ||
|
|
||
| error_return: | ||
| if (je1.s.error) | ||
| report_json_error(js1, &je1, 0); | ||
| null_return: | ||
| null_value= 1; | ||
| return NULL; |
Contributor
There was a problem hiding this comment.
This implementation has the same critical issues as json_array_intersect:
- Stale
null_valueCheck: Checkingargs[i]->null_valuebefore evaluating the arguments uses stale values from the previous row. - Compilation Error: The
goto new_arg2;statement jumps into theif (!root_inited)block, bypassing local variable initializations. - Dangling Pointers: Local
tmp_js1andtmp_js2cause use-after-free bugs when the second argument is constant. - Redundant
killed_ptrInitialization:json_read_valueis not interruptible and does not calljson_scan_nextinternally, so initializingkilled_ptrforje1andje2is redundant.
Restructuring the control flow to avoid goto and keeping the buffers as class members resolves all these issues.
thd= current_thd;
if (root_inited && args[1]->null_value)
goto null_return;
if (!root_inited || !args[1]->const_item())
{
if (!root_inited)
{
init_alloc_root(PSI_NOT_INSTRUMENTED, &hash_root, 1024, 0, MYF(0));
root_inited= true;
}
String *js2= args[1]->val_json(&tmp_js2);
if (!js2 || args[1]->null_value)
goto null_return;
json_engine_t je2;
json_scan_start(&je2, js2->charset(),(const uchar *) js2->ptr(),
(const uchar *) js2->ptr() + js2->length());
if (json_read_value(&je2))
goto je2_error_return;
if (je2.value_type != JSON_VALUE_ARRAY)
{
je2.s.error= JE_SYN;
je2_error_return:
report_json_error(js2, &je2, 1);
goto null_return;
}
if (create_or_reset_hash(&je2, &items, hash_inited, &hash_root))
{
if (je2.s.error)
goto je2_error_return;
my_error(ER_OUTOFMEMORY, MYF(0), 64);
goto null_return;
}
}
{
String *js1= args[0]->val_json(&tmp_js1);
if (!js1 || args[0]->null_value)
goto null_return;
json_engine_t je1;
JSON_DO_PAUSE_EXECUTION(thd, 0.0002);
json_scan_start(&je1, js1->charset(),(const uchar *) js1->ptr(),
(const uchar *) js1->ptr() + js1->length());
if (json_read_value(&je1))
goto error_return;
if (je1.value_type != JSON_VALUE_OBJECT)
{
je1.s.error= JE_SYN;
error_return:
report_json_error(js1, &je1, 0);
goto null_return;
}
{
String fil_res;
fil_res.set_charset(js1->charset());
if (filter_keys(&je1, &fil_res, items))
goto error_return;
if (fil_res.length())
{
json_engine_t res_je;
json_scan_start(&res_je, fil_res.charset(), (const uchar *) fil_res.ptr(),
(const uchar *) fil_res.ptr() + fil_res.length());
res_je.killed_ptr= (uint32_t *) &thd->killed;
str->set_charset(js1->charset());
str->length(0);
if (json_nice(&res_je, str, Item_func_json_format::LOOSE))
{
report_json_error(str, &res_je, 0);
goto null_return;
}
null_value= 0;
return str;
}
else
goto null_return;
}
}References
- Do not assume that json_read_value internally calls json_scan_next or is interruptible. Only paths that explicitly call json_scan_next (or functions that do) are interruptible and require killed_ptr initialization.
Comment on lines
+5854
to
5856
| set_maybe_null(); | ||
| if (args[1]->null_value || args[0]->null_value) | ||
| return FALSE; |
Contributor
There was a problem hiding this comment.
Checking null_value during fix_length_and_dec is incorrect because it is called during query preparation/compilation when parameter values are not yet evaluated (and may be NULL). Skipping max_length assignment here will result in max_length being set to 0, leading to incorrect metadata and potential truncation during execution.
set_maybe_null();
gkodinov
added
the
MariaDB Foundation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both these functions attempted to cache things in fix_length_and_dec. Corrected implementations on both.