Hello Andrei, Good morning. Thank you for the review comments. Please find my replies inline. On 27/04/20 11:26 pm, Andrei Elkin wrote:
Sujatha, howdy.
Thanks for a prompt checking the case! The patch looks good, only
- if (!wild_do_table.elements) + if (status && wild_do_table_inited) I would suggest to reorder the && arg:s to be then in temporal wrt their evaluation order.
Sure Andrei. I have refined changes as shown below. diff --git a/sql/rpl_filter.cc b/sql/rpl_filter.cc index 635a0f4e2d6..8035763bf35 100644 --- a/sql/rpl_filter.cc +++ b/sql/rpl_filter.cc @@ -416,10 +416,13 @@ Rpl_filter::set_wild_do_table(const char* table_spec) status= parse_filter_rule(table_spec, &Rpl_filter::add_wild_do_table); - if (!wild_do_table.elements) + if (wild_do_table_inited && status) { - delete_dynamic(&wild_do_table); - wild_do_table_inited= 0; + if (!wild_do_table.elements) + { + delete_dynamic(&wild_do_table); + wild_do_table_inited= 0; + } }
revision-id: 85b116d1ab9ea85dcef63d259b8f6366466e2750 (mariadb-10.5.2-185-g85b116d1ab9) parent(s): fbe2712705d464bf8488df249c36115e2c1f63f7 author: Sujatha committer: Sujatha timestamp: 2020-04-27 17:43:51 +0530 message:
MDEV-22317: SIGSEGV in my_free/delete_dynamic in optimized builds (ARIA)
Problem: ======= SET @@GLOBAL.replicate_wild_ignore_table=''; SET @@GLOBAL.replicate_wild_do_table='';
Reports following valgrind error.
Conditional jump or move depends on uninitialised value(s) Rpl_filter::set_wild_ignore_table(char const*) (rpl_filter.cc:439)
Conditional jump or move depends on uninitialised value(s) at 0xF60390: delete_dynamic (array.c:304) by 0x74F3F2: Rpl_filter::set_wild_do_table(char const*) (rpl_filter.cc:421)
Analysis: ======== List of values provided for options "wild_do_table" and "wild_ignore_table" are stored in DYNAMIC_ARRAYS. When an empty list is provided these dynamic arrays are not initialized. Correct. And that's 'cos
Rpl_filter::Rpl_filter() : parallel_mode(SLAVE_PARALLEL_CONSERVATIVE), table_rules_on(0), do_table_inited(0), ignore_table_inited(0), wild_do_table_inited(0), wild_ignore_table_inited(0) { do_db.empty(); ignore_db.empty(); rewrite_db.empty(); }
does not initialize the two members of your patch's interest with empty()-ing. I wonder if
+ wild_do_table.empty() + wild_ignore_table.empty() ?
that could work out as well which looks as more consistent solution to me. [It feels like binlog_filter object might object this idea though].
'do_db' and 'ignore_db' are of type list iterators. Hence they have 'empty'
defined for them. 'empty' is not a member of DYNAMIC_ARRAY.
I_List
Cheers,
Andrei
Existing code treats empty element list as an error and tries to clean the uninitialized list. This results in above valgrind issue.
Fix: === The clean up should be initiated only when there is an error while parsing the 'wild_do_table' or 'wild_ignore_table' list and the dynamic_array is in initialized state. Otherwise for empty list it should simply return success.
--- mysql-test/suite/rpl/r/rpl_filter_wild_tables_dynamic.result | 2 ++ mysql-test/suite/rpl/t/rpl_filter_wild_tables_dynamic.test | 2 ++ sql/rpl_filter.cc | 4 ++-- 3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/mysql-test/suite/rpl/r/rpl_filter_wild_tables_dynamic.result b/mysql-test/suite/rpl/r/rpl_filter_wild_tables_dynamic.result index 47cd362a549..fe0b283fc7c 100644 --- a/mysql-test/suite/rpl/r/rpl_filter_wild_tables_dynamic.result +++ b/mysql-test/suite/rpl/r/rpl_filter_wild_tables_dynamic.result @@ -7,6 +7,8 @@ SET @@GLOBAL.replicate_wild_ignore_table="test.b%"; ERROR HY000: This operation cannot be performed as you have a running slave ''; run STOP SLAVE '' first connection slave; include/stop_slave.inc +SET @@GLOBAL.replicate_wild_do_table=""; +SET @@GLOBAL.replicate_wild_ignore_table=""; SET @@GLOBAL.replicate_wild_do_table="test.a%"; SET @@GLOBAL.replicate_wild_ignore_table="test.b%"; include/start_slave.inc diff --git a/mysql-test/suite/rpl/t/rpl_filter_wild_tables_dynamic.test b/mysql-test/suite/rpl/t/rpl_filter_wild_tables_dynamic.test index 6db61927eec..657a95cec15 100644 --- a/mysql-test/suite/rpl/t/rpl_filter_wild_tables_dynamic.test +++ b/mysql-test/suite/rpl/t/rpl_filter_wild_tables_dynamic.test @@ -13,6 +13,8 @@ SET @@GLOBAL.replicate_wild_ignore_table="test.b%";
connection slave; source include/stop_slave.inc; +SET @@GLOBAL.replicate_wild_do_table=""; +SET @@GLOBAL.replicate_wild_ignore_table=""; SET @@GLOBAL.replicate_wild_do_table="test.a%"; SET @@GLOBAL.replicate_wild_ignore_table="test.b%"; source include/start_slave.inc; diff --git a/sql/rpl_filter.cc b/sql/rpl_filter.cc index 635a0f4e2d6..49b498d3568 100644 --- a/sql/rpl_filter.cc +++ b/sql/rpl_filter.cc @@ -416,7 +416,7 @@ Rpl_filter::set_wild_do_table(const char* table_spec)
status= parse_filter_rule(table_spec, &Rpl_filter::add_wild_do_table);
- if (!wild_do_table.elements) + if (status && wild_do_table_inited) { delete_dynamic(&wild_do_table); wild_do_table_inited= 0; @@ -436,7 +436,7 @@ Rpl_filter::set_wild_ignore_table(const char* table_spec)
status= parse_filter_rule(table_spec, &Rpl_filter::add_wild_ignore_table);
- if (!wild_ignore_table.elements) + if (status && wild_ignore_table_inited) { delete_dynamic(&wild_ignore_table); wild_ignore_table_inited= 0; _______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits