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 secur...@mariadb.org
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
