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

@@ -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,8 +830,11 @@ 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);
-  plugin_dl_del(&plugin->plugin_dl->dl);
+  if (plugin->plugin_dl)
+    plugin_dl_del(&plugin->plugin_dl->dl);
   plugin->state= PLUGIN_IS_FREED;
   plugin_array_version++;
   rw_wrlock(&LOCK_system_variables_hash);
@@ -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);