Hi, Oleksandr,
On Jul 07, Oleksandr Byelkin wrote:
> revision-id: d8e04ef367b (mariadb-10.4.30-9-gd8e04ef367b)
> parent(s): 423c28f0aa4
> author: Oleksandr Byelkin
> committer: Oleksandr Byelkin
> timestamp: 2023-07-03 17:50:49 +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
first, if you see a bug with a very long summary, particularly if it's
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"
> --- a/sql/sys_vars.inl
> +++ b/sql/sys_vars.inl
> @@ -620,7 +620,12 @@ class Sys_var_sesvartrack: public Sys_var_charptr_base
> {
> if (sysvartrack_global_update(thd, new_val,
> var->save_result.string_value.length))
> - new_val= 0;
> + {
> + if (new_val)
> + my_free(new_val);
> + // keep the old value in case of error
> + new_val= (char*)my_strdup(global_var(const char*), MYF(MY_WME));
> + }
No, this is wrong. Well, not wrong, but an incorrect bug fix.
update should not fail, this is the contract. check() is supposed to
verify that the value is valid, so basically update must succeed.
technically it can fail because of OOM, I suppose, so restoring the
value here might be needed still. But in the case of OOM, strdup() might be not
such a great idea. Perhaps you can make sure that OOM is impossible and
update truly cannot fail?
Anyway, the actual fix for MDEV-25237 is to make check fail on an
invalid value. In fact, it was a simple typo:
@@ -221,7 +221,7 @@ bool sysvartrack_validate_value(THD *thd, const char *str, >
/* 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;
> }
> 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
