[Commits] Rev 3512: MDEV-4647: Crash on setting wsep system variables to default in lp:~maria-captains/maria/maria-5.5-galera

Nirbhay Choubey nirbhay at skysql.com
Mon Jul 21 23:16:00 EEST 2014


At lp:~maria-captains/maria/maria-5.5-galera

------------------------------------------------------------
revno: 3512
revision-id: nirbhay at skysql.com-20140721201552-b1t5zcpaial0d0fp
parent: nirbhay at skysql.com-20140715045429-d00tr2k9h0ftaqr4
committer: Nirbhay Choubey <nirbhay at skysql.com>
branch nick: maria-5.5-galera_sys_vars
timestamp: Mon 2014-07-21 16:15:52 -0400
message:
  MDEV-4647: Crash on setting wsep system variables to default
  
  The variables' ON_CHECK functions relied on set_var's "value"
  member, which is NULL for SET ... =default. Fixed by using
  save_result instead.
  
  Also, for many wsrep variables, pointers to their respective
  global variables were used to provide default values. The patch
  fixes this by using appropriate macros and string literals.
-------------- next part --------------
=== modified file 'sql/sys_vars.cc'
--- a/sql/sys_vars.cc	2014-04-16 10:08:29 +0000
+++ b/sql/sys_vars.cc	2014-07-21 20:15:52 +0000
@@ -3691,8 +3691,7 @@
 static Sys_var_charptr Sys_wsrep_provider(
        "wsrep_provider", "Path to replication provider library",
        PREALLOCATED GLOBAL_VAR(wsrep_provider), CMD_LINE(REQUIRED_ARG, OPT_WSREP_PROVIDER),
-       IN_FS_CHARSET, DEFAULT(wsrep_provider), 
-       //       IN_FS_CHARSET, DEFAULT(wsrep_provider_default), 
+       IN_FS_CHARSET, DEFAULT(WSREP_NONE),
        NO_MUTEX_GUARD, NOT_IN_BINLOG,
        ON_CHECK(wsrep_provider_check), ON_UPDATE(wsrep_provider_update));
 
@@ -3700,8 +3699,7 @@
        "wsrep_provider_options", "provider specific options",
        PREALLOCATED GLOBAL_VAR(wsrep_provider_options), 
        CMD_LINE(REQUIRED_ARG, OPT_WSREP_PROVIDER_OPTIONS),
-       IN_FS_CHARSET, DEFAULT(wsrep_provider_options), 
-       NO_MUTEX_GUARD, NOT_IN_BINLOG,
+       IN_FS_CHARSET, DEFAULT(""), NO_MUTEX_GUARD, NOT_IN_BINLOG,
        ON_CHECK(wsrep_provider_options_check), 
        ON_UPDATE(wsrep_provider_options_update));
 
@@ -3714,7 +3712,7 @@
 static Sys_var_charptr Sys_wsrep_cluster_name(
        "wsrep_cluster_name", "Name for the cluster",
        PREALLOCATED GLOBAL_VAR(wsrep_cluster_name), CMD_LINE(REQUIRED_ARG),
-       IN_FS_CHARSET, DEFAULT(wsrep_cluster_name), 
+       IN_FS_CHARSET, DEFAULT(WSREP_CLUSTER_NAME),
        NO_MUTEX_GUARD, NOT_IN_BINLOG,
        ON_CHECK(wsrep_cluster_name_check),
        ON_UPDATE(wsrep_cluster_name_update));
@@ -3724,7 +3722,7 @@
        "wsrep_cluster_address", "Address to initially connect to cluster",
        PREALLOCATED GLOBAL_VAR(wsrep_cluster_address), 
        CMD_LINE(REQUIRED_ARG, OPT_WSREP_CLUSTER_ADDRESS),
-       IN_FS_CHARSET, DEFAULT(wsrep_cluster_address),
+       IN_FS_CHARSET, DEFAULT(""),
        &PLock_wsrep_slave_threads, NOT_IN_BINLOG,
        ON_CHECK(wsrep_cluster_address_check), 
        ON_UPDATE(wsrep_cluster_address_update));
@@ -3732,21 +3730,21 @@
 static Sys_var_charptr Sys_wsrep_node_name (
        "wsrep_node_name", "Node name",
        PREALLOCATED GLOBAL_VAR(wsrep_node_name), CMD_LINE(REQUIRED_ARG),
-       IN_FS_CHARSET, DEFAULT(wsrep_node_name), 
-       NO_MUTEX_GUARD, NOT_IN_BINLOG);
+       IN_FS_CHARSET, DEFAULT(""), NO_MUTEX_GUARD, NOT_IN_BINLOG,
+       wsrep_node_name_check, wsrep_node_name_update);
 
 static Sys_var_charptr Sys_wsrep_node_address (
        "wsrep_node_address", "Node address",
        PREALLOCATED GLOBAL_VAR(wsrep_node_address), CMD_LINE(REQUIRED_ARG),
-       IN_FS_CHARSET, DEFAULT(wsrep_node_address), 
+       IN_FS_CHARSET, DEFAULT(""),
        NO_MUTEX_GUARD, NOT_IN_BINLOG,
-       ON_CHECK(wsrep_node_address_check), 
+       ON_CHECK(wsrep_node_address_check),
        ON_UPDATE(wsrep_node_address_update));
 
 static Sys_var_charptr Sys_wsrep_node_incoming_address(
        "wsrep_node_incoming_address", "Client connection address",
        PREALLOCATED GLOBAL_VAR(wsrep_node_incoming_address),CMD_LINE(REQUIRED_ARG),
-       IN_FS_CHARSET, DEFAULT(wsrep_node_incoming_address), 
+       IN_FS_CHARSET, DEFAULT(WSREP_NODE_INCOMING_AUTO),
        NO_MUTEX_GUARD, NOT_IN_BINLOG);
 
 static Sys_var_ulong Sys_wsrep_slave_threads(
@@ -3794,7 +3792,7 @@
 static Sys_var_charptr sys_wsrep_sst_method(
        "wsrep_sst_method", "State snapshot transfer method",
        GLOBAL_VAR(wsrep_sst_method),CMD_LINE(REQUIRED_ARG),
-       IN_FS_CHARSET, DEFAULT(wsrep_sst_method), NO_MUTEX_GUARD, NOT_IN_BINLOG,
+       IN_FS_CHARSET, DEFAULT(WSREP_SST_DEFAULT), NO_MUTEX_GUARD, NOT_IN_BINLOG,
        ON_CHECK(wsrep_sst_method_check),
        ON_UPDATE(wsrep_sst_method_update)); 
 
@@ -3802,7 +3800,7 @@
        "wsrep_sst_receive_address", "Address where node is waiting for "
        "SST contact", 
        GLOBAL_VAR(wsrep_sst_receive_address),CMD_LINE(REQUIRED_ARG),
-       IN_FS_CHARSET, DEFAULT(wsrep_sst_receive_address), NO_MUTEX_GUARD, 
+       IN_FS_CHARSET, DEFAULT(WSREP_SST_ADDRESS_AUTO), NO_MUTEX_GUARD,
        NOT_IN_BINLOG,
        ON_CHECK(wsrep_sst_receive_address_check),
        ON_UPDATE(wsrep_sst_receive_address_update)); 
@@ -3810,7 +3808,7 @@
 static Sys_var_charptr Sys_wsrep_sst_auth(
        "wsrep_sst_auth", "Authentication for SST connection",
        PREALLOCATED GLOBAL_VAR(wsrep_sst_auth), CMD_LINE(REQUIRED_ARG, OPT_WSREP_SST_AUTH),
-       IN_FS_CHARSET, DEFAULT(wsrep_sst_auth), NO_MUTEX_GUARD, 
+       IN_FS_CHARSET, DEFAULT(NULL), NO_MUTEX_GUARD,
        NOT_IN_BINLOG,
        ON_CHECK(wsrep_sst_auth_check),
        ON_UPDATE(wsrep_sst_auth_update)); 
@@ -3839,7 +3837,7 @@
        "wsrep_start_position", "global transaction position to start from ",
        PREALLOCATED GLOBAL_VAR(wsrep_start_position), 
        CMD_LINE(REQUIRED_ARG, OPT_WSREP_START_POSITION),
-       IN_FS_CHARSET, DEFAULT(wsrep_start_position),
+       IN_FS_CHARSET, DEFAULT(WSREP_START_POSITION_ZERO),
        NO_MUTEX_GUARD, NOT_IN_BINLOG,
        ON_CHECK(wsrep_start_position_check), 
        ON_UPDATE(wsrep_start_position_update));

=== modified file 'sql/wsrep_sst.cc'
--- a/sql/wsrep_sst.cc	2014-07-09 15:04:28 +0000
+++ b/sql/wsrep_sst.cc	2014-07-21 20:15:52 +0000
@@ -31,32 +31,6 @@
 
 extern const char wsrep_defaults_file[];
 
-#define WSREP_SST_OPT_ROLE     "--role"
-#define WSREP_SST_OPT_ADDR     "--address"
-#define WSREP_SST_OPT_AUTH     "--auth"
-#define WSREP_SST_OPT_DATA     "--datadir"
-#define WSREP_SST_OPT_CONF     "--defaults-file"
-#define WSREP_SST_OPT_PARENT   "--parent"
-
-// mysqldump-specific options
-#define WSREP_SST_OPT_USER     "--user"
-#define WSREP_SST_OPT_PSWD     "--password"
-#define WSREP_SST_OPT_HOST     "--host"
-#define WSREP_SST_OPT_PORT     "--port"
-#define WSREP_SST_OPT_LPORT    "--local-port"
-
-// donor-specific
-#define WSREP_SST_OPT_SOCKET   "--socket"
-#define WSREP_SST_OPT_GTID     "--gtid"
-#define WSREP_SST_OPT_BYPASS   "--bypass"
-
-#define WSREP_SST_MYSQLDUMP    "mysqldump"
-#define WSREP_SST_RSYNC        "rsync"
-#define WSREP_SST_SKIP         "skip"
-#define WSREP_SST_DEFAULT      WSREP_SST_RSYNC
-#define WSREP_SST_ADDRESS_AUTO "AUTO"
-#define WSREP_SST_AUTH_MASK    "********"
-
 const char* wsrep_sst_method          = WSREP_SST_DEFAULT;
 const char* wsrep_sst_receive_address = WSREP_SST_ADDRESS_AUTO;
 const char* wsrep_sst_donor           = "";
@@ -68,17 +42,16 @@
 
 bool wsrep_sst_method_check (sys_var *self, THD* thd, set_var* var)
 {
-    char   buff[FN_REFLEN];
-    String str(buff, sizeof(buff), system_charset_info), *res;
-    const char* c_str = NULL;
-
-    if ((res   = var->value->val_str(&str)) &&
-        (c_str = res->c_ptr()) &&
-        strlen(c_str) > 0)
-        return 0;
-
-    my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "wsrep_sst_method", c_str ? c_str : "NULL");
+  if ((! var->save_result.string_value.str) ||
+      (var->save_result.string_value.length == 0 ))
+  {
+    my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str,
+             var->save_result.string_value.str ?
+             var->save_result.string_value.str : "NULL");
     return 1;
+  }
+
+  return 0;
 }
 
 bool wsrep_sst_method_update (sys_var *self, THD* thd, enum_var_type type)
@@ -86,6 +59,7 @@
     return 0;
 }
 
+// TODO: Improve address verification.
 static bool sst_receive_address_check (const char* str)
 {
     if (!strncasecmp(str, "127.0.0.1", strlen("127.0.0.1")) ||
@@ -99,15 +73,30 @@
 
 bool  wsrep_sst_receive_address_check (sys_var *self, THD* thd, set_var* var)
 {
-    const char* c_str = var->value->str_value.c_ptr();
-
-    if (sst_receive_address_check (c_str))
-    {
-        my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "wsrep_sst_receive_address", c_str ? c_str : "NULL");
-        return 1;
-    }
-
-    return 0;
+  char addr_buf[FN_REFLEN];
+
+  if ((! var->save_result.string_value.str) ||
+      (var->save_result.string_value.length > (FN_REFLEN - 1))) // safety
+  {
+    goto err;
+  }
+
+  memcpy(addr_buf, var->save_result.string_value.str,
+         var->save_result.string_value.length);
+  addr_buf[var->save_result.string_value.length]= 0;
+
+  if (sst_receive_address_check(addr_buf))
+  {
+    goto err;
+  }
+
+  return 0;
+
+err:
+  my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str,
+           var->save_result.string_value.str ?
+           var->save_result.string_value.str : "NULL");
+  return 1;
 }
 
 bool wsrep_sst_receive_address_update (sys_var *self, THD* thd,

=== modified file 'sql/wsrep_sst.h'
--- a/sql/wsrep_sst.h	2014-07-09 15:04:28 +0000
+++ b/sql/wsrep_sst.h	2014-07-21 20:15:52 +0000
@@ -16,7 +16,33 @@
 #ifndef WSREP_SST_H
 #define WSREP_SST_H
 
-#include <mysql.h> // my_bool
+#include <mysql.h>                    // my_bool
+
+#define WSREP_SST_OPT_ROLE     "--role"
+#define WSREP_SST_OPT_ADDR     "--address"
+#define WSREP_SST_OPT_AUTH     "--auth"
+#define WSREP_SST_OPT_DATA     "--datadir"
+#define WSREP_SST_OPT_CONF     "--defaults-file"
+#define WSREP_SST_OPT_PARENT   "--parent"
+
+// mysqldump-specific options
+#define WSREP_SST_OPT_USER     "--user"
+#define WSREP_SST_OPT_PSWD     "--password"
+#define WSREP_SST_OPT_HOST     "--host"
+#define WSREP_SST_OPT_PORT     "--port"
+#define WSREP_SST_OPT_LPORT    "--local-port"
+
+// donor-specific
+#define WSREP_SST_OPT_SOCKET   "--socket"
+#define WSREP_SST_OPT_GTID     "--gtid"
+#define WSREP_SST_OPT_BYPASS   "--bypass"
+
+#define WSREP_SST_MYSQLDUMP    "mysqldump"
+#define WSREP_SST_RSYNC        "rsync"
+#define WSREP_SST_SKIP         "skip"
+#define WSREP_SST_DEFAULT      WSREP_SST_RSYNC
+#define WSREP_SST_ADDRESS_AUTO "AUTO"
+#define WSREP_SST_AUTH_MASK    "********"
 
 /* system variables */
 extern const char* wsrep_sst_method;

=== modified file 'sql/wsrep_var.cc'
--- a/sql/wsrep_var.cc	2014-07-09 15:04:28 +0000
+++ b/sql/wsrep_var.cc	2014-07-21 20:15:52 +0000
@@ -26,9 +26,6 @@
 #include <cstdio>
 #include <cstdlib>
 
-#define WSREP_START_POSITION_ZERO "00000000-0000-0000-0000-000000000000:-1"
-#define WSREP_CLUSTER_NAME "my_wsrep_cluster"
-
 const  char* wsrep_provider         = 0;
 const  char* wsrep_provider_options = 0;
 const  char* wsrep_cluster_address  = 0;
@@ -98,22 +95,22 @@
 
 bool wsrep_start_position_check (sys_var *self, THD* thd, set_var* var)
 {
-  char   buff[FN_REFLEN];
-  String str(buff, sizeof(buff), system_charset_info), *res;
-  const char*   start_str = NULL;
-
-  if (!(res = var->value->val_str(&str))) goto err;
-
-  start_str = res->c_ptr();
-
-  if (!start_str) goto err;
-
-  if (!wsrep_start_position_verify(start_str)) return 0;
+  char start_pos_buf[FN_REFLEN];
+
+  if ((! var->save_result.string_value.str) ||
+      (var->save_result.string_value.length > (FN_REFLEN - 1))) // safety
+    goto err;
+
+  memcpy(start_pos_buf, var->save_result.string_value.str,
+         var->save_result.string_value.length);
+  start_pos_buf[var->save_result.string_value.length]= 0;
+
+  if (!wsrep_start_position_verify(start_pos_buf)) return 0;
 
 err:
-
-  my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str, 
-           start_str ? start_str : "NULL");
+  my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str,
+           var->save_result.string_value.str ?
+           var->save_result.string_value.str : "NULL");
   return 1;
 }
 
@@ -199,22 +196,22 @@
 
 bool wsrep_provider_check (sys_var *self, THD* thd, set_var* var)
 {
-  char   buff[FN_REFLEN];
-  String str(buff, sizeof(buff), system_charset_info), *res;
-  const char*   provider_str = NULL;
-
-  if (!(res = var->value->val_str(&str))) goto err;
-
-  provider_str = res->c_ptr();
-
-  if (!provider_str) goto err;
-
-  if (!wsrep_provider_verify(provider_str)) return 0;
+  char wsrep_provider_buf[FN_REFLEN];
+
+  if ((! var->save_result.string_value.str) ||
+      (var->save_result.string_value.length > (FN_REFLEN - 1))) // safety
+    goto err;
+
+  memcpy(wsrep_provider_buf, var->save_result.string_value.str,
+         var->save_result.string_value.length);
+  wsrep_provider_buf[var->save_result.string_value.length]= 0;
+
+  if (!wsrep_provider_verify(wsrep_provider_buf)) return 0;
 
 err:
-
-  my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str, 
-           provider_str ? provider_str : "NULL");
+  my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str,
+           var->save_result.string_value.str ?
+           var->save_result.string_value.str : "NULL");
   return 1;
 }
 
@@ -309,21 +306,23 @@
 
 bool wsrep_cluster_address_check (sys_var *self, THD* thd, set_var* var)
 {
-  char   buff[FN_REFLEN];
-  String str(buff, sizeof(buff), system_charset_info), *res;
-  const char*   cluster_address_str = NULL;
-
-  if (!(res = var->value->val_str(&str))) goto err;
-
-  cluster_address_str = res->c_ptr();
-
-  if (!wsrep_cluster_address_verify(cluster_address_str)) return 0;
+  char addr_buf[FN_REFLEN];
+
+  if ((! var->save_result.string_value.str) ||
+      (var->save_result.string_value.length > (FN_REFLEN - 1))) // safety
+    goto err;
+
+  memcpy(addr_buf, var->save_result.string_value.str,
+         var->save_result.string_value.length);
+  addr_buf[var->save_result.string_value.length]= 0;
+
+  if (!wsrep_cluster_address_verify(addr_buf)) return 0;
 
  err:
-
-  my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str, 
-             cluster_address_str ? cluster_address_str : "NULL");
-  return 1    ;
+  my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str,
+           var->save_result.string_value.str ?
+           var->save_result.string_value.str : "NULL");
+  return 1;
 }
 
 bool wsrep_cluster_address_update (sys_var *self, THD* thd, enum_var_type type)
@@ -363,25 +362,18 @@
   wsrep_cluster_address = (value) ? my_strdup(value, MYF(0)) : NULL;
 }
 
+/* wsrep_cluster_name cannot be NULL or an empty string. */
 bool wsrep_cluster_name_check (sys_var *self, THD* thd, set_var* var)
 {
-  char   buff[FN_REFLEN];
-  String str(buff, sizeof(buff), system_charset_info), *res;
-  const char* cluster_name_str = NULL;
-
-  if (!(res = var->value->val_str(&str))) goto err;
-
-  cluster_name_str = res->c_ptr();
-
-  if (!cluster_name_str || strlen(cluster_name_str) == 0) goto err;
-
+  if (!var->save_result.string_value.str ||
+      (var->save_result.string_value.length == 0))
+  {
+    my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str,
+             (var->save_result.string_value.str ?
+              var->save_result.string_value.str : "NULL"));
+    return 1;
+  }
   return 0;
-
- err:
-
-  my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str, 
-             cluster_name_str ? cluster_name_str : "NULL");
-  return 1;
 }
 
 bool wsrep_cluster_name_update (sys_var *self, THD* thd, enum_var_type type)
@@ -391,23 +383,15 @@
 
 bool wsrep_node_name_check (sys_var *self, THD* thd, set_var* var)
 {
-  char   buff[FN_REFLEN];
-  String str(buff, sizeof(buff), system_charset_info), *res;
-  const char* node_name_str = NULL;
-
-  if (!(res = var->value->val_str(&str))) goto err;
-
-  node_name_str = res->c_ptr();
-
-  if (!node_name_str || strlen(node_name_str) == 0) goto err;
-
+  // TODO: for now 'allow' 0-length string to be valid (default)
+  if (!var->save_result.string_value.str)
+  {
+    my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str,
+             (var->save_result.string_value.str ?
+              var->save_result.string_value.str : "NULL"));
+    return 1;
+  }
   return 0;
-
- err:
-
-  my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str,
-           node_name_str ? node_name_str : "NULL");
-  return 1;
 }
 
 bool wsrep_node_name_update (sys_var *self, THD* thd, enum_var_type type)
@@ -418,22 +402,23 @@
 // TODO: do something more elaborate, like checking connectivity
 bool wsrep_node_address_check (sys_var *self, THD* thd, set_var* var)
 {
-  char   buff[FN_REFLEN];
-  String str(buff, sizeof(buff), system_charset_info), *res;
-  const char* node_address_str = NULL;
-
-  if (!(res = var->value->val_str(&str))) goto err;
-
-  node_address_str = res->c_ptr();
-
-  if (!node_address_str || strlen(node_address_str) == 0) goto err;
-
+  char addr_buf[FN_REFLEN];
+
+  if ((! var->save_result.string_value.str) ||
+      (var->save_result.string_value.length > (FN_REFLEN - 1))) // safety
+    goto err;
+
+  memcpy(addr_buf, var->save_result.string_value.str,
+         var->save_result.string_value.length);
+  addr_buf[var->save_result.string_value.length]= 0;
+
+  // TODO: for now 'allow' 0-length string to be valid (default)
   return 0;
 
- err:
-
+err:
   my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), var->var->name.str,
-           node_address_str ? node_address_str : "NULL");
+           var->save_result.string_value.str ?
+           var->save_result.string_value.str : "NULL");
   return 1;
 }
 
@@ -453,7 +438,8 @@
 bool wsrep_slave_threads_check (sys_var *self, THD* thd, set_var* var)
 {
   mysql_mutex_lock(&LOCK_wsrep_slave_threads);
-  wsrep_slave_count_change += (var->value->val_int() - wsrep_slave_threads);
+  wsrep_slave_count_change += (var->save_result.ulonglong_value -
+                               wsrep_slave_threads);
   mysql_mutex_unlock(&LOCK_wsrep_slave_threads);
 
   return 0;
@@ -471,7 +457,7 @@
 
 bool wsrep_desync_check (sys_var *self, THD* thd, set_var* var)
 {
-  bool new_wsrep_desync = var->value->val_bool();
+  bool new_wsrep_desync= (bool) var->save_result.ulonglong_value;
   if (wsrep_desync == new_wsrep_desync) {
     if (new_wsrep_desync) {
       push_warning (thd, MYSQL_ERROR::WARN_LEVEL_WARN,

=== modified file 'sql/wsrep_var.h'
--- a/sql/wsrep_var.h	2014-01-09 19:54:57 +0000
+++ b/sql/wsrep_var.h	2014-07-21 20:15:52 +0000
@@ -16,7 +16,9 @@
 #ifndef WSREP_VAR_H
 #define WSREP_VAR_H
 
-#define WSREP_NODE_INCOMING_AUTO "AUTO"
+#define WSREP_CLUSTER_NAME        "my_wsrep_cluster"
+#define WSREP_NODE_INCOMING_AUTO  "AUTO"
+#define WSREP_START_POSITION_ZERO "00000000-0000-0000-0000-000000000000:-1"
 
 // MySQL variables funcs
 



More information about the commits mailing list