← Back to team overview

maria-developers team mailing list archive

  1. maria-developers team
  2. Mailing list archive
  3. Message #12212

Re: [Commits] 85b116d1ab9: MDEV-22317: SIGSEGV in my_free/delete_dynamic in optimized builds (ARIA)

  Thread Previous • Date Previous • • Thread Next 

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<i_string> do_db;
 I_List<i_string> ignore_db;
/home/sujatha/bug_repo/MDEV-22317-10.5/sql/rpl_filter.cc:36:17: error: ‘DYNAMIC_ARRAY {aka struct st_dynamic_array}’ has no member named ‘empty’ 
   wild_do_table.empty();
                 ^~~~~
/home/sujatha/bug_repo/MDEV-22317-10.5/sql/rpl_filter.cc:37:21: error: ‘DYNAMIC_ARRAY {aka struct st_dynamic_array}’ has no member named ‘empty’ 
   wild_ignore_table.empty();
                     ^~~~~

"wild_do_table" and "wild_ignore_table" arrays get initialized as part of
"add_wild_do_table" and "add_wild_ignore_table". The initialization is done
through following calls.

status= parse_filter_rule(table_spec, &Rpl_filter::add_wild_do_table);

status= parse_filter_rule(table_spec, &Rpl_filter::add_wild_ignore_table);

When default empty string is provided for replicate_wild_ignore_table=''
and replicate_wild_do_table='' the dynamic array is not initialized.
'wild_do_table.elements' will be '0'. No cleanup is required in this case.

Hence we should do the cleanup only when 'wild_do_table_inited' is true.

Please let me know your thoughts.

Thank you

S.Sujatha




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@xxxxxxxxxxx
https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

References

  Thread Previous • Date Previous • • Thread Next