Skip to content

Commit

Permalink
Fix all warnings given by UBSAN
Browse files Browse the repository at this point in the history
The 'special' cases where we disable, suppress or circumvent UBSAN are:
- ref10 source (as here we intentionally do some shifts that UBSAN
  complains about.
- x86 version of optimized int#korr() methods. UBSAN do not like unaligned
  memory access of integers.  Fixed by using byte_order_generic.h when
  compiling with UBSAN
- We use smaller thread stack with ASAN and UBSAN, which forced me to
  disable a few tests that prints the thread stack size.
- Verifying class types does not work for shared libraries. I added
  suppression in mysql-test-run.pl for this case.
- Added '#ifdef WITH_UBSAN' when using integer arithmetic where it is
  safe to have overflows (two cases, in item_func.cc).

Things fixed:
- Don't left shift signed values
  (byte_order_generic.h, mysqltest.c, item_sum.cc and many more)
- Don't assign not non existing values to enum variables.
- Ensure that bool and enum values are properly initialized in
  constructors.  This was needed as UBSAN checks that these types has
  correct values when one copies an object.
  (gcalc_tools.h, ha_partition.cc, item_sum.cc, partition_element.h ...)
- Ensure we do not called handler functions on unallocated objects or
  deleted objects.
  (events.cc, sql_acl.cc).
- Fixed bugs in Item_sp::Item_sp() where we did not call constructor
  on Query_arena object.
- Fixed several cast of objects to an incompatible class!
  (Item.cc, Item_buff.cc, item_timefunc.cc, opt_subselect.cc, sql_acl.cc,
   sql_select.cc ...)
- Ensure we do not do integer arithmetic that causes over or underflows.
  This includes also ++ and -- of integers.
  (Item_func.cc, Item_strfunc.cc, item_timefunc.cc, sql_base.cc ...)
- Added JSON_VALUE_UNITIALIZED to json_value_types and ensure that
  value_type is initialized to this instead of to -1, which is not a valid
  enum value for json_value_types.
- Ensure we do not call memcpy() when second argument could be null.

Other things:

- Changed struct st_position to an OBJECT and added an initialization
  function to it to ensure that we do not copy or use uninitialized
  members. The change to a class was also motived that we used "struct
  st_position" and POSITION randomly trough the code which was
  confusing.
- Notably big rewrite in sql_acl.cc to avoid using deleted objects.
- Changed in sql_partition to use '^' instead of '-'. This is safe as
  the operator is either 0 or 0x8000000000000000ULL.
- Added check for select_nr < INT_MAX in JOIN::build_explain() to
  avoid bug when get_select() could return NULL.
- Reordered elements in POSITION for better alignment.
- Changed sql_test.cc::print_plan() to use pointers instead of objects.
- Fixed bug in find_set() where could could execute '1 << -1'.
- Added variable have_sanitizer, used by mtr.  (This variable was before
  only in 10.5 and up).  It can now have one of two values:
  ASAN or UBSAN.
- Moved ~Archive_share() from ha_archive.cc to ha_archive.h and marked
  it virtual. This was an effort to get UBSAN to work with loaded storage
  engines. I kept the change as the new place is better.
- Added in CONNECT engine COLBLK::SetName(), to get around a wrong cast
  in tabutil.cpp.

Changes that should not be needed but had to be done to suppress warnings
from UBSAN:

- Added static_cast<<uint16_t>> around shift to get rid of a LOT of
  compiler warnings when using UBSAN.
- Had to change some '/' of 2 base integers to shift to get rid of
  some compile time warnings.

Fixes:

MDEV-25505 Assertion `old_flags == ((my_flags & 0x10000U) ? 1 : 0)
fixed (was caused by an old version if this commit).

Reviewed by:
- Json changes: Alexey Botchkov
- Charset changes in ctype-uca.c: Alexander Barkov
- InnoDB changes: Marko Mäkelä
- sql_acl.cc changes: Vicențiu Ciorbaru
- build_explain() changes: Sergey Petrunia
Temporary commit to log changes for UBSAN
  • Loading branch information
montywi authored and vuvova committed May 19, 2021
1 parent b332ffc commit cc125be
Show file tree
Hide file tree
Showing 13 changed files with 218 additions and 157 deletions.
2 changes: 1 addition & 1 deletion BUILD/compile-pentium64-ubsan
Expand Up @@ -24,6 +24,6 @@ path=`dirname $0`
. "$path/SETUP.sh"

extra_flags="$pentium64_cflags $debug_cflags -fsanitize=undefined -DWITH_UBSAN -Wno-conversion -Wno-uninitialized"
extra_configs="$pentium_configs $debug_configs -DWITH_UBSAN=ON -DMYSQL_MAINTAINER_MODE=NO --without-spider --without-tokudb"
extra_configs="$pentium_configs $debug_configs -DWITH_UBSAN=ON -DMYSQL_MAINTAINER_MODE=NO --without-spider"

. "$path/FINISH.sh"
9 changes: 9 additions & 0 deletions mysql-test/suite/maria/alter.result
Expand Up @@ -184,3 +184,12 @@ SET GLOBAL aria_max_sort_file_size= @max_size.save;
#
# End of 10.4 test
#
#
# MDEV-25505 Assertion `old_flags == ((my_flags & 0x10000U) ? 1 : 0)'
# failed in my_realloc
#
CREATE TABLE t1 (pk int, c text, primary key (pk), key(c(32))) ENGINE=Aria ROW_FORMAT=DYNAMIC;
ALTER TABLE t1 DISABLE KEYS;
INSERT INTO t1 VALUES (1, 'Nine chars or more');
ALTER TABLE t1 ENABLE KEYS;
DROP TABLE t1;
11 changes: 11 additions & 0 deletions mysql-test/suite/maria/alter.test
Expand Up @@ -192,3 +192,14 @@ SET GLOBAL aria_max_sort_file_size= @max_size.save;
--echo #
--echo # End of 10.4 test
--echo #

--echo #
--echo # MDEV-25505 Assertion `old_flags == ((my_flags & 0x10000U) ? 1 : 0)'
--echo # failed in my_realloc
--echo #

CREATE TABLE t1 (pk int, c text, primary key (pk), key(c(32))) ENGINE=Aria ROW_FORMAT=DYNAMIC;
ALTER TABLE t1 DISABLE KEYS;
INSERT INTO t1 VALUES (1, 'Nine chars or more');
ALTER TABLE t1 ENABLE KEYS;
DROP TABLE t1;
1 change: 1 addition & 0 deletions mysql-test/suite/plugins/t/multiauth.test
@@ -1,4 +1,5 @@
--source include/not_ubsan.inc

let $REGEX_VERSION_ID=/$mysql_get_server_version/VERSION_ID/;
let $REGEX_PASSWORD_LAST_CHANGED=/password_last_changed": [0-9]*/password_last_changed": #/;
let $REGEX_GLOBAL_PRIV=$REGEX_PASSWORD_LAST_CHANGED $REGEX_VERSION_ID;
Expand Down
6 changes: 4 additions & 2 deletions sql/item_strfunc.cc
Expand Up @@ -3867,6 +3867,7 @@ String *Item_load_file::val_str(String *str)
File file;
MY_STAT stat_info;
char path[FN_REFLEN];
size_t file_size;
DBUG_ENTER("load_file");

if (!(file_name= args[0]->val_str(str))
Expand All @@ -3891,10 +3892,11 @@ String *Item_load_file::val_str(String *str)
/* my_error(ER_TEXTFILE_NOT_READABLE, MYF(0), file_name->c_ptr()); */
goto err;
}
file_size= stat_info.st_size;

{
THD *thd= current_thd;
if (stat_info.st_size > (long) thd->variables.max_allowed_packet)
if (file_size >= (size_t) thd->variables.max_allowed_packet)
{
push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN,
ER_WARN_ALLOWED_PACKET_OVERFLOWED,
Expand All @@ -3903,7 +3905,7 @@ String *Item_load_file::val_str(String *str)
goto err;
}
}
if (tmp_value.alloc((size_t)stat_info.st_size))
if (tmp_value.alloc(file_size))
goto err;
if ((file= mysql_file_open(key_file_loadfile,
file_name->ptr(), O_RDONLY, MYF(0))) < 0)
Expand Down
55 changes: 29 additions & 26 deletions sql/sql_acl.cc
Expand Up @@ -6955,7 +6955,7 @@ int mysql_table_grant(THD *thd, TABLE_LIST *table_list,
bool revoke_grant)
{
privilege_t column_priv(NO_ACL);
int result;
int result, res;
List_iterator <LEX_USER> str_list (user_list);
LEX_USER *Str, *tmp_Str;
bool create_new_users=0;
Expand Down Expand Up @@ -7156,10 +7156,10 @@ int mysql_table_grant(THD *thd, TABLE_LIST *table_list,
revoke_grant))
result= TRUE;
}
if (int res= replace_table_table(thd, grant_table,
tables.tables_priv_table().table(),
*Str, db_name, table_name,
rights, column_priv, revoke_grant))
if ((res= replace_table_table(thd, grant_table,
tables.tables_priv_table().table(),
*Str, db_name, table_name,
rights, column_priv, revoke_grant)))
{
if (res > 0)
{
Expand Down Expand Up @@ -11277,7 +11277,7 @@ mysql_revoke_sp_privs(THD *thd, Grant_tables *tables, const Sp_handler *sph,
bool mysql_revoke_all(THD *thd, List <LEX_USER> &list)
{
uint counter, revoked;
int result;
int result, res;
ACL_DB *acl_db;
DBUG_ENTER("mysql_revoke_all");

Expand Down Expand Up @@ -11361,30 +11361,33 @@ bool mysql_revoke_all(THD *thd, List <LEX_USER> &list)
{
for (counter= 0, revoked= 0 ; counter < column_priv_hash.records ; )
{
const char *user,*host;
GRANT_TABLE *grant_table=
(GRANT_TABLE*) my_hash_element(&column_priv_hash, counter);
const char *user,*host;
GRANT_TABLE *grant_table= ((GRANT_TABLE*)
my_hash_element(&column_priv_hash, counter));

user= grant_table->user;
host= safe_str(grant_table->host.hostname);

if (!strcmp(lex_user->user.str,user) &&
!strcmp(lex_user->host.str, host))
{
List<LEX_COLUMN> columns;
/* TODO(cvicentiu) refactor to use
{
List<LEX_COLUMN> columns;
/* TODO(cvicentiu) refactor replace_db_table to use
Db_table instead of TABLE directly. */
if (replace_column_table(grant_table,
tables.columns_priv_table().table(),
*lex_user, columns, grant_table->db,
grant_table->tname, ALL_KNOWN_ACL, 1))
result= -1;

/* TODO(cvicentiu) refactor replace_db_table to use
Db_table instead of TABLE directly. */
if (replace_column_table(grant_table,
tables.columns_priv_table().table(),
*lex_user, columns,
grant_table->db, grant_table->tname,
ALL_KNOWN_ACL, 1))
result= -1;
if (int res= replace_table_table(thd, grant_table,
tables.tables_priv_table().table(),
*lex_user,
grant_table->db, grant_table->tname,
ALL_KNOWN_ACL, NO_ACL, 1))
{
if ((res= replace_table_table(thd, grant_table,
tables.tables_priv_table().table(),
*lex_user, grant_table->db,
grant_table->tname, ALL_KNOWN_ACL,
NO_ACL, 1)))
{
if (res > 0)
result= -1;
else
Expand All @@ -11397,8 +11400,8 @@ bool mysql_revoke_all(THD *thd, List <LEX_USER> &list)
continue;
}
}
}
counter++;
}
counter++;
}
} while (revoked);

Expand Down
96 changes: 95 additions & 1 deletion sql/sql_select.cc
Expand Up @@ -366,7 +366,6 @@ bool dbug_user_var_equals_int(THD *thd, const char *name, int value)
}
#endif /* DBUG_OFF */


/*
Intialize POSITION structure.
*/
Expand All @@ -391,6 +390,100 @@ POSITION::POSITION()
}


void JOIN::init(THD *thd_arg, List<Item> &fields_arg,
ulonglong select_options_arg, select_result *result_arg)
{
join_tab= 0;
table= 0;
table_count= 0;
top_join_tab_count= 0;
const_tables= 0;
const_table_map= found_const_table_map= 0;
aggr_tables= 0;
eliminated_tables= 0;
join_list= 0;
implicit_grouping= FALSE;
sort_and_group= 0;
first_record= 0;
do_send_rows= 1;
duplicate_rows= send_records= 0;
found_records= accepted_rows= 0;
fetch_limit= HA_POS_ERROR;
thd= thd_arg;
sum_funcs= sum_funcs2= 0;
procedure= 0;
having= tmp_having= having_history= 0;
having_is_correlated= false;
group_list_for_estimates= 0;
select_options= select_options_arg;
result= result_arg;
lock= thd_arg->lock;
select_lex= 0; //for safety
select_distinct= MY_TEST(select_options & SELECT_DISTINCT);
no_order= 0;
simple_order= 0;
simple_group= 0;
rand_table_in_field_list= 0;
ordered_index_usage= ordered_index_void;
need_distinct= 0;
skip_sort_order= 0;
with_two_phase_optimization= 0;
save_qep= 0;
spl_opt_info= 0;
ext_keyuses_for_splitting= 0;
spl_opt_info= 0;
need_tmp= 0;
hidden_group_fields= 0; /*safety*/
error= 0;
select= 0;
return_tab= 0;
ref_ptrs.reset();
items0.reset();
items1.reset();
items2.reset();
items3.reset();
zero_result_cause= 0;
optimization_state= JOIN::NOT_OPTIMIZED;
have_query_plan= QEP_NOT_PRESENT_YET;
initialized= 0;
cleaned= 0;
cond_equal= 0;
having_equal= 0;
exec_const_cond= 0;
group_optimized_away= 0;
no_rows_in_result_called= 0;
positions= best_positions= 0;
pushdown_query= 0;
original_join_tab= 0;
explain= NULL;
tmp_table_keep_current_rowid= 0;

all_fields= fields_arg;
if (&fields_list != &fields_arg) /* Avoid valgrind-warning */
fields_list= fields_arg;
non_agg_fields.empty();
bzero((char*) &keyuse,sizeof(keyuse));
having_value= Item::COND_UNDEF;
tmp_table_param.init();
tmp_table_param.end_write_records= HA_POS_ERROR;
rollup.state= ROLLUP::STATE_NONE;

no_const_tables= FALSE;
first_select= sub_select;
set_group_rpa= false;
group_sent= 0;

outer_ref_cond= pseudo_bits_cond= NULL;
in_to_exists_where= NULL;
in_to_exists_having= NULL;
emb_sjm_nest= NULL;
sjm_lookup_tables= 0;
sjm_scan_tables= 0;
is_orig_degenerated= false;
with_ties_order_count= 0;
};


static void trace_table_dependencies(THD *thd,
JOIN_TAB *join_tabs, uint table_count)
{
Expand Down Expand Up @@ -29888,6 +29981,7 @@ bool JOIN::optimize_upper_rownum_func()
return process_direct_rownum_comparison(thd, unit, outer_select->where);
}


/**
Test if the predicate compares rownum() with a constant

Expand Down
93 changes: 1 addition & 92 deletions sql/sql_select.h
Expand Up @@ -1546,98 +1546,7 @@ class JOIN :public Sql_alloc
}

void init(THD *thd_arg, List<Item> &fields_arg, ulonglong select_options_arg,
select_result *result_arg)
{
join_tab= 0;
table= 0;
table_count= 0;
top_join_tab_count= 0;
const_tables= 0;
const_table_map= found_const_table_map= 0;
aggr_tables= 0;
eliminated_tables= 0;
join_list= 0;
implicit_grouping= FALSE;
sort_and_group= 0;
first_record= 0;
do_send_rows= 1;
duplicate_rows= send_records= 0;
found_records= accepted_rows= 0;
fetch_limit= HA_POS_ERROR;
thd= thd_arg;
sum_funcs= sum_funcs2= 0;
procedure= 0;
having= tmp_having= having_history= 0;
having_is_correlated= false;
group_list_for_estimates= 0;
select_options= select_options_arg;
result= result_arg;
lock= thd_arg->lock;
select_lex= 0; //for safety
select_distinct= MY_TEST(select_options & SELECT_DISTINCT);
no_order= 0;
simple_order= 0;
simple_group= 0;
rand_table_in_field_list= 0;
ordered_index_usage= ordered_index_void;
need_distinct= 0;
skip_sort_order= 0;
with_two_phase_optimization= 0;
save_qep= 0;
spl_opt_info= 0;
ext_keyuses_for_splitting= 0;
spl_opt_info= 0;
need_tmp= 0;
hidden_group_fields= 0; /*safety*/
error= 0;
select= 0;
return_tab= 0;
ref_ptrs.reset();
items0.reset();
items1.reset();
items2.reset();
items3.reset();
zero_result_cause= 0;
optimization_state= JOIN::NOT_OPTIMIZED;
have_query_plan= QEP_NOT_PRESENT_YET;
initialized= 0;
cleaned= 0;
cond_equal= 0;
having_equal= 0;
exec_const_cond= 0;
group_optimized_away= 0;
no_rows_in_result_called= 0;
positions= best_positions= 0;
pushdown_query= 0;
original_join_tab= 0;
explain= NULL;
tmp_table_keep_current_rowid= 0;

all_fields= fields_arg;
if (&fields_list != &fields_arg) /* Avoid valgrind-warning */
fields_list= fields_arg;
non_agg_fields.empty();
bzero((char*) &keyuse,sizeof(keyuse));
having_value= Item::COND_UNDEF;
tmp_table_param.init();
tmp_table_param.end_write_records= HA_POS_ERROR;
rollup.state= ROLLUP::STATE_NONE;

no_const_tables= FALSE;
first_select= sub_select;
set_group_rpa= false;
group_sent= 0;

outer_ref_cond= pseudo_bits_cond= NULL;
in_to_exists_where= NULL;
in_to_exists_having= NULL;
emb_sjm_nest= NULL;
sjm_lookup_tables= 0;
sjm_scan_tables= 0;
is_orig_degenerated= false;

with_ties_order_count= 0;
}
select_result *result_arg);

/* True if the plan guarantees that it will be returned zero or one row */
bool only_const_tables() { return const_tables == table_count; }
Expand Down

0 comments on commit cc125be

Please sign in to comment.