MDEV-35554/ MDEV-33158 UBSAN runtime error: call to function show_cached_thread_count() (and plugin interface) #3737
Conversation
grooverdan
added
the
MariaDB Foundation
grooverdan
force-pushed
the
MDEV-35554-ubsan-show_cached_thread_count
branch
from
8116295 to
50ebc21
Compare
grooverdan
changed the title
grooverdan
force-pushed
the
MDEV-35554-ubsan-show_cached_thread_count
branch
from
6dcbc88 to
d85cd2a
Compare
|
The spider commit (d85cd2a) lgtm, thanks for the fix |
There was a problem hiding this comment.
Hi @grooverdan !
The first two commits look good, thanks! I left a couple minor notes to consider. Also, I am leaving the third commit for @mariadb-YuchenPei to review.
sql/sql_plugin.cc
Outdated
| @@ -3227,6 +3227,11 @@ static int *mysql_sys_var_int(THD* thd, int offset) | |||
| return (int *) intern_sys_var_ptr(thd, offset, true); | |||
| } | |||
|
|
|||
| static unsigned int *mysql_sys_var_uint(THD* thd, int offset) | |||
| { | |||
| return (unsigned int *) intern_sys_var_ptr(thd, offset, true); | |||
There was a problem hiding this comment.
There's a lot of C-style casts in this area, though it's cpp code. Perhaps now would be a good time to refactor them to the proper casting style.
| @@ -714,8 +714,8 @@ int spider_udf_bg_copy_exec_sql( | |||
| long long spider_copy_tables_body( | |||
| UDF_INIT *initid, | |||
| UDF_ARGS *args, | |||
| char *is_null, | |||
| char *error | |||
| unsigned char *is_null, | |||
There was a problem hiding this comment.
On the MDEV-35735 JIRA you mention
performed manual testing in UBSAN environment as its not completely runable in mtr:
I'd say that'd be good to mention in the commit message too (along with the test case, and also some more detail on why you couldn't use MTR)
There was a problem hiding this comment.
Although now after some of my own attempts to reproduce/test the issue (which I couldn't actually reproduce the error following the JIRA commands), I now see @mariadb-YuchenPei is assigned to review the spider changes, so I'll let him take over for this part.
There was a problem hiding this comment.
@mariadb-YuchenPei has reviewed it. Sorry, was in previous comment.
with manual CONC-711 changes it was quite testable in MTR and this should bemerged into the server soon.
| case PLUGIN_VAR_BOOL: | ||
| ((thdvar_bool_t *) opt)->resolve= mysql_sys_var_char; | ||
| break; | ||
| case PLUGIN_VAR_INT: | ||
| ((thdvar_int_t *) opt)->resolve= mysql_sys_var_int; | ||
| break; | ||
| case PLUGIN_VAR_INT | PLUGIN_VAR_UNSIGNED: | ||
| ((thdvar_uint_t *) opt)->resolve= mysql_sys_var_uint; |
There was a problem hiding this comment.
I think in the second commit message (for MDEV-33158), it'd be good to re-mention the MTR tests that verify the fixes, as you do on JIRA.
through pointer to incorrect function type. The argument is void* rather than char* and was missing system_status_var * as an argument. This shows up with UBSAN testing under clang. Reviewer: Brandon Nesterenko
grooverdan
force-pushed
the
MDEV-35554-ubsan-show_cached_thread_count
branch
from
d85cd2a to
058813b
Compare
In plugins, use the correct resolver for ULONG and ULONGLONG types. InnoDB has a UINT type as evidenced by "Unknown variable type code 0x182 in plugin 'InnoDB'." so the implementation for UNSIGNED INT was added. Any InnoDB mtr test that changes lock_wait_timeout, spider_param_force_commit and some MyRocks unsigned thread server variables can verify this change is correct. Reviewer: Brandon Nesterenko
There were too many C casts rather than C++ casts here. Started a partial conversion covering within-file scoped changes. Suggested by Brandon Nesterenko
The mismatch occurs on the function calls as in the sql/sql_udf.h the types of "error" and "is_null" are unsigned char rather than char. This is corrected for the udf functions: * spider_direct_sql * spider_direct_bg_sql * spider_flush_table_mon_cache * spider_copy_tables * spider_ping_table Reviewer: Yuchen Pei
grooverdan
force-pushed
the
MDEV-35554-ubsan-show_cached_thread_count
branch
from
058813b to
645e2a0
Compare
Description
clang/UBSAN shows this up as an error as the argument is incorrect.
The argument is void* rather than char*.
Release Notes
not required.
How can this PR be tested?
basic bootstrap when compiled WITH_UBSAN under clang.
If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.
Basing the PR against the correct MariaDB version
mainbranch.PR quality check