fixed: memleak in --help, sigsegv on shutdown

Ingo's patch:
WL#2936 - Falcon & MySQL plugin interface: server variables
Added initialization for plugin string variables with their
default values.
Added deallocation of string values before a plugin and its
variables is deleted.
Added examples to plugin_example


mysys/my_getopt.c:
  Ingo's patch:
  WL#2936 - Falcon & MySQL plugin interface: server variables
  Added initialization for string options. Since string variables
  do often have their default value assigned already, assign the
  default value only if the variable value is NULL.
plugin/fulltext/plugin_example.c:
  Ingo's patch:
  WL#2936 - Falcon & MySQL plugin interface: server variables
  Added examples for thread variables, which have a SESSION and
  a GLOBAL value.
sql/mysqld.cc:
  removed second fix_paths() in --help output (memory leak).
  removed invalid string defaul values (binlog_format)
  don't hide the error message in the help text
sql/sql_plugin.cc:
  don't do plugin_dl_del for built-in plugins (sigsegv).
  Ingo's patch:
  WL#2936 - Falcon & MySQL plugin interface: server variables
  Clearing newly allocated variable value space. This is important
  for string variables. They are initialized to their default
  value only if their initial value is NULL.
  Setting default values for strings.
  Added a function to free global value space for string variables.
  Call the function before deleting a plugin and its variables.

mysys/my_getopt.c

@@ -813,6 +813,7 @@ ulonglong getopt_ull_limit_value(ulonglong num, const struct my_option *optp)
 static void init_one_value(const struct my_option *option, gptr *variable,
 			   longlong value)
 {
+  DBUG_ENTER("init_one_value");
   switch ((option->var_type & GET_TYPE_MASK)) {
   case GET_BOOL:
     *((my_bool*) variable)= (my_bool) value;
@@ -837,9 +838,29 @@ static void init_one_value(const struct my_option *option, gptr *variable,
   case GET_SET:
     *((ulonglong*) variable)=  (ulonglong) value;
     break;
+  case GET_STR:
+    /*
+      Do not clear variable value if it has no default value.
+      The default value may already be set.
+    */
+    if ((char*) value)
+      *((char**) variable)= (char*) value;
+    break;
+  case GET_STR_ALLOC:
+    /*
+      Do not clear variable value if it has no default value.
+      The default value may already be set.
+    */
+    if ((char*) value)
+    {
+      my_free((*(char**) variable), MYF(MY_ALLOW_ZERO_PTR));
+      *((char**) variable)= my_strdup((char*) value, MYF(MY_WME));
+    }
+    break;
   default: /* dummy default to avoid compiler warnings */
     break;
   }
+  DBUG_VOID_RETURN;
 }
 
 
@@ -858,9 +879,11 @@ static void init_one_value(const struct my_option *option, gptr *variable,
 
 static void init_variables(const struct my_option *options)
 {
+  DBUG_ENTER("init_variables");
   for (; options->name; options++)
   {
     gptr *variable;
+    DBUG_PRINT("options", ("name: '%s'", options->name));
     /*
       We must set u_max_value first as for some variables
       options->u_max_value == options->value and in this case we want to
@@ -874,6 +897,7 @@ static void init_variables(const struct my_option *options)
 	(variable= (*getopt_get_addr)("", 0, options)))
       init_one_value(options, variable, options->def_value);
   }
+  DBUG_VOID_RETURN;
 }
 
 

plugin/fulltext/plugin_example.c

@@ -225,16 +225,28 @@ static char     *sysvar_two_value;
 static MYSQL_SYSVAR_LONG(simple_sysvar_one, sysvar_one_value,
   PLUGIN_VAR_RQCMDARG,
   "Simple fulltext parser example system variable number one. Give a number.",
-  NULL, NULL, 100L, 10L, ~0L, 0);
+  NULL, NULL, 77L, 7L, 777L, 0);
 
 static MYSQL_SYSVAR_STR(simple_sysvar_two, sysvar_two_value,
   PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_MEMALLOC,
   "Simple fulltext parser example system variable number two. Give a string.",
-  NULL, NULL, NULL);
+  NULL, NULL, "simple sysvar two default");
+
+static MYSQL_THDVAR_LONG(simple_thdvar_one,
+  PLUGIN_VAR_RQCMDARG,
+  "Simple fulltext parser example thread variable number one. Give a number.",
+  NULL, NULL, 88L, 8L, 888L, 0);
+
+static MYSQL_THDVAR_STR(simple_thdvar_two,
+  PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_MEMALLOC,
+  "Simple fulltext parser example thread variable number two. Give a string.",
+  NULL, NULL, "simple thdvar two default");
 
 static struct st_mysql_sys_var* simple_system_variables[]= {
   MYSQL_SYSVAR(simple_sysvar_one),
   MYSQL_SYSVAR(simple_sysvar_two),
+  MYSQL_SYSVAR(simple_thdvar_one),
+  MYSQL_SYSVAR(simple_thdvar_two),
   NULL
 };
 

sql/mysqld.cc

@@ -1114,10 +1114,10 @@ void unireg_end(void)
 extern "C" void unireg_abort(int exit_code)
 {
   DBUG_ENTER("unireg_abort");
-  if (opt_help)
-    usage();
   if (exit_code)
     sql_print_error("Aborting\n");
+  else if (opt_help)
+    usage();
   clean_up(exit_code || !opt_bootstrap); /* purecov: inspected */
   DBUG_PRINT("quit",("done with cleanup in unireg_abort"));
   wait_for_signal_thread_to_end();
@@ -5056,8 +5056,7 @@ struct my_option my_long_options[] =
    " to 'row' and back implicitly per each query accessing a NDB table."
 #endif
    ,(gptr*) &opt_binlog_format, (gptr*) &opt_binlog_format,
-   0, GET_STR, REQUIRED_ARG, BINLOG_FORMAT_MIXED, BINLOG_FORMAT_STMT,
-   BINLOG_FORMAT_MIXED, 0, 0, 0},
+   0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
   {"binlog-do-db", OPT_BINLOG_DO_DB,
    "Tells the master it should log updates for the specified database, and exclude all others not explicitly mentioned.",
    0, 0, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
@@ -6888,7 +6887,6 @@ Starts the MySQL database server\n");
 #endif
   print_defaults(MYSQL_CONFIG_NAME,load_default_groups);
   puts("");
-  fix_paths();
   set_ports();
 
   /* Print out all the options including plugin supplied options */

sql/sql_plugin.cc

@@ -196,6 +196,7 @@ static bool register_builtin(struct st_mysql_plugin *, struct st_plugin_int *,
                              struct st_plugin_int **);
 static void unlock_variables(THD *thd, struct system_variables *vars);
 static void cleanup_variables(THD *thd, struct system_variables *vars);
+static void plugin_vars_free_values(sys_var *vars);
 static void plugin_opt_set_limits(struct my_option *options,
                                   const struct st_mysql_sys_var *opt);
 #define my_intern_plugin_lock(A,B) intern_plugin_lock(A,B CALLER_INFO)
@@ -829,7 +830,10 @@ static void plugin_del(struct st_plugin_int *plugin)
 {
   DBUG_ENTER("plugin_del(plugin)");
   safe_mutex_assert_owner(&LOCK_plugin);
+  /* Free allocated strings before deleting the plugin. */
+  plugin_vars_free_values(plugin->system_vars);
   hash_delete(&plugin_hash[plugin->plugin->type], (byte*)plugin);
+  if (plugin->plugin_dl)
     plugin_dl_del(&plugin->plugin_dl->dl);
   plugin->state= PLUGIN_IS_FREED;
   plugin_array_version++;
@@ -1588,8 +1592,9 @@ bool mysql_install_plugin(THD *thd, const LEX_STRING *name, const LEX_STRING *dl
 
   pthread_mutex_lock(&LOCK_plugin);
   rw_wrlock(&LOCK_system_variables_hash);
-  argv[0]= ""; /* handle_options() assumes arg0 (program name) always exists */
-  argv[1]= NULL;
+  /* handle_options() assumes arg0 (program name) always exists */
+  argv[0]= const_cast<char*>(""); // without a cast gcc emits a warning
+  argv[1]= 0;
   argc= 1;
   error= plugin_add(thd->mem_root, name, dl, &argc, argv, REPORT_TO_USER);
   rw_unlock(&LOCK_system_variables_hash);
@@ -2171,6 +2176,17 @@ static st_bookmark *register_var(const char *plugin, const char *name,
       max_system_variables.dynamic_variables_ptr=
         my_realloc(max_system_variables.dynamic_variables_ptr, new_size,
                    MYF(MY_WME | MY_FAE | MY_ALLOW_ZERO_PTR));
+      /*
+        Clear the new variable value space. This is required for string
+        variables. If their value is non-NULL, it must point to a valid
+        string.
+      */
+      bzero(global_system_variables.dynamic_variables_ptr +
+            global_variables_dynamic_size,
+            new_size - global_variables_dynamic_size);
+      bzero(max_system_variables.dynamic_variables_ptr +
+            global_variables_dynamic_size,
+            new_size - global_variables_dynamic_size);
       global_variables_dynamic_size= new_size;
     }
 
@@ -2311,6 +2327,9 @@ static void unlock_variables(THD *thd, struct system_variables *vars)
 
 /*
   Frees memory used by system variables
+
+  Unlike plugin_vars_free_values() it frees all variables of all plugins,
+  it's used on shutdown.
 */
 static void cleanup_variables(THD *thd, struct system_variables *vars)
 {
@@ -2379,6 +2398,40 @@ void plugin_thdvar_cleanup(THD *thd)
 }
 
 
+/**
+  @brief Free values of thread variables of a plugin.
+
+  @detail This must be called before a plugin is deleted. Otherwise its
+  variables are no longer accessible and the value space is lost. Note
+  that only string values with PLUGIN_VAR_MEMALLOC are allocated and
+  must be freed.
+
+  @param[in]        vars        Chain of system variables of a plugin
+*/
+
+static void plugin_vars_free_values(sys_var *vars)
+{
+  DBUG_ENTER("plugin_vars_free_values");
+
+  for (sys_var *var= vars; var; var= var->next)
+  {
+    sys_var_pluginvar *piv= var->cast_pluginvar();
+    if (piv &&
+        ((piv->plugin_var->flags & PLUGIN_VAR_TYPEMASK) == PLUGIN_VAR_STR) &&
+        (piv->plugin_var->flags & PLUGIN_VAR_MEMALLOC))
+    {
+      /* Free the string from global_system_variables. */
+      char **valptr= (char**) piv->real_value_ptr(NULL, OPT_GLOBAL);
+      DBUG_PRINT("plugin", ("freeing value for: '%s'  addr: 0x%lx",
+                            var->name, (long) valptr));
+      my_free(*valptr, MYF(MY_WME | MY_FAE | MY_ALLOW_ZERO_PTR));
+      *valptr= NULL;
+    }
+  }
+  DBUG_VOID_RETURN;
+}
+
+
 bool sys_var_pluginvar::check_update_type(Item_result type)
 {
   if (is_readonly())
@@ -2421,10 +2474,10 @@ SHOW_TYPE sys_var_pluginvar::show_type()
 
 byte* sys_var_pluginvar::real_value_ptr(THD *thd, enum_var_type type)
 {
-  DBUG_ASSERT(thd);
+  DBUG_ASSERT(thd || (type != OPT_SESSION));
   if (plugin_var->flags & PLUGIN_VAR_THDLOCAL)
   {
-    if (type == OPT_GLOBAL)
+    if (type != OPT_SESSION)
       thd= NULL;
 
     return intern_sys_var_ptr(thd, *(int*) (plugin_var+1), false);
@@ -2614,7 +2667,9 @@ static void plugin_opt_set_limits(struct my_option *options,
     options->def_value= *(my_bool*) ((void**)(opt + 1) + 1);
     break;
   case PLUGIN_VAR_STR:
-    options->var_type= GET_STR;
+    options->var_type= ((opt->flags & PLUGIN_VAR_MEMALLOC) ?
+                        GET_STR_ALLOC : GET_STR);
+    options->def_value= (ulonglong)(intptr) *((char**) ((void**) (opt + 1) + 1));
     break;
   /* threadlocal variables */
   case PLUGIN_VAR_INT | PLUGIN_VAR_THDLOCAL:
@@ -2654,7 +2709,9 @@ static void plugin_opt_set_limits(struct my_option *options,
     options->def_value= *(my_bool*) ((int*) (opt + 1) + 1);
     break;
   case PLUGIN_VAR_STR | PLUGIN_VAR_THDLOCAL:
-    options->var_type= GET_STR;
+    options->var_type= ((opt->flags & PLUGIN_VAR_MEMALLOC) ?
+                        GET_STR_ALLOC : GET_STR);
+    options->def_value= (intptr) *((char**) ((void**) (opt + 1) + 1));
     break;
   default:
     DBUG_ASSERT(0);