Hi, Sanja, ok to push. but see comments below: On Jul 15, Oleksandr Byelkin wrote:

revision-id: 8e9c68b49dd (mariadb-10.4.30-9-g8e9c68b49dd) parent(s): 423c28f0aa4 author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2023-07-10 13:40:46 +0200 message:

MDEV-25237 Assertion `global_system_variables.session_track_system_variables' failed in Session_sysvars_tracker::init | SIGSEGV's in __strlen_avx2 | UBSAN: runtime error: null pointer passed as argument 1, which is declared to never be null in my_strdup

again, if you see a bug with a very long summary, particularly if it basically makes no sense to a user - in that case, please, don't hesitate to make it shorter and more meaningful. For example "crash after setting session_track_system_variables to an invalid value"

Fix of typo in checking variable list corectness.

Fix of error handling in case of variable list parse error

diff --git a/sql/session_tracker.cc b/sql/session_tracker.cc index 2f6c5e29bf2..3e1558f6759 100644 --- a/sql/session_tracker.cc +++ b/sql/session_tracker.cc @@ -221,7 +221,7 @@ bool sysvartrack_validate_value(THD *thd, const char *str, size_t len) /* Remove leading/trailing whitespace. */ trim_whitespace(system_charset_info, &var);

- if (!strcmp(var.str, "*") && !find_sys_var(thd, var.str, var.length)) + if (strcmp(var.str, "*") && !find_sys_var(thd, var.str, var.length)) return true;

if (lasts) @@ -331,9 +331,10 @@ void Session_sysvars_tracker::init(THD *thd) mysql_mutex_assert_owner(&LOCK_global_system_variables); DBUG_ASSERT(thd->variables.session_track_system_variables == global_system_variables.session_track_system_variables); - DBUG_ASSERT(global_system_variables.session_track_system_variables); thd->variables.session_track_system_variables= - my_strdup(global_system_variables.session_track_system_variables, + my_strdup((global_system_variables.session_track_system_variables? + global_system_variables.session_track_system_variables: + ""),

this is such a common pattern that we have a helper for that, use my_strdup(safe_str(global_system_variables.session_track_system_variables))

MYF(MY_WME | MY_THREAD_SPECIFIC)); }

@@ -572,6 +573,12 @@ bool sysvartrack_global_update(THD *thd, char *str, size_t len) { LEX_STRING tmp= { str, len }; Session_sysvars_tracker::vars_list dummy; + DBUG_EXECUTE_IF("dbug_session_tracker_parse_error", + { + my_error(ER_OUTOFMEMORY, MYF(0), 1); + return true; + }); + if (!dummy.parse_var_list(thd, tmp, false, system_charset_info)) { dummy.construct_var_list(str, len + 1); diff --git a/sql/sys_vars.inl b/sql/sys_vars.inl index 84d1cd6b331..3e282de439a 100644 --- a/sql/sys_vars.inl +++ b/sql/sys_vars.inl @@ -620,7 +620,11 @@ class Sys_var_sesvartrack: public Sys_var_charptr_base { if (sysvartrack_global_update(thd, new_val, var->save_result.string_value.length)) + { + if (new_val) + my_free(new_val);

it's ok, as you like. But technically you don't need an `if` here, my_free() just as free() works with NULL pointers fine.

new_val= 0; + } } global_update_finish(new_val); return (new_val == 0 && var->save_result.string_value.str != 0);

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org