maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

well, I guess there is no alternative way to workaround it. :(
Ok to push.

Regards,
Sergei

On Mon, Dec 09, 2013 at 01:51:51PM +0100, Sergei Golubchik wrote:
> Hi.
> 
> I don't see my commit emails on the list, so here you are:
> 
> first patch is the bug fix itself.
> second - cleanup of the existing code after the bug fix.
> 
> Regards,
> Sergei
> 

> ------------------------------------------------------------
> revno: 3927
> fixes bug: https://mariadb.atlassian.net/browse/MDEV-4403
> committer: Sergei Golubchik <sergii@xxxxxxxxx>
> branch nick: 10.0
> timestamp: Mon 2013-12-09 12:39:13 +0100
> message:
>   MDEV-4403 Attempting to use cassandra storage engine causes "service 'my_snprintf_service' interface version mismatch"
>   
>   When a DSO is loaded we rewrite service pointers to point to the actual service structures.
>   But when a DSO is unloaded, we have to restore their original values, in case this DSO
>   wasn't removed from memory on dlclose() and is later loaded again.
> added:
>   mysql-test/suite/plugins/r/cassandra_reinstall.result
>   mysql-test/suite/plugins/t/cassandra_reinstall.test
> modified:
>   sql/sql_plugin.cc
>   sql/sql_plugin.h
> diff:
> === added file 'mysql-test/suite/plugins/r/cassandra_reinstall.result'
> --- mysql-test/suite/plugins/r/cassandra_reinstall.result	1970-01-01 00:00:00 +0000
> +++ mysql-test/suite/plugins/r/cassandra_reinstall.result	2013-12-09 11:39:13 +0000
> @@ -0,0 +1,14 @@
> +install soname 'ha_cassandra';
> +select plugin_name,plugin_status,plugin_library from information_schema.plugins where plugin_name = 'cassandra';
> +plugin_name	plugin_status	plugin_library
> +CASSANDRA	ACTIVE	ha_cassandra.so
> +uninstall plugin cassandra;
> +select plugin_name,plugin_status,plugin_library from information_schema.plugins where plugin_name = 'cassandra';
> +plugin_name	plugin_status	plugin_library
> +install soname 'ha_cassandra';
> +select plugin_name,plugin_status,plugin_library from information_schema.plugins where plugin_name = 'cassandra';
> +plugin_name	plugin_status	plugin_library
> +CASSANDRA	ACTIVE	ha_cassandra.so
> +uninstall plugin cassandra;
> +select plugin_name,plugin_status,plugin_library from information_schema.plugins where plugin_name = 'cassandra';
> +plugin_name	plugin_status	plugin_library
> 
> === added file 'mysql-test/suite/plugins/t/cassandra_reinstall.test'
> --- mysql-test/suite/plugins/t/cassandra_reinstall.test	1970-01-01 00:00:00 +0000
> +++ mysql-test/suite/plugins/t/cassandra_reinstall.test	2013-12-09 11:39:13 +0000
> @@ -0,0 +1,16 @@
> +#
> +# MDEV-4403 Attempting to use cassandra storage engine causes "service 'my_snprintf_service' interface version mismatch"
> +#
> +if (!$HA_CASSANDRA_SO) {
> +  skip No Cassandra engine;
> +}
> +
> +install soname 'ha_cassandra';
> +select plugin_name,plugin_status,plugin_library from information_schema.plugins where plugin_name = 'cassandra';
> +uninstall plugin cassandra;
> +select plugin_name,plugin_status,plugin_library from information_schema.plugins where plugin_name = 'cassandra';
> +install soname 'ha_cassandra';
> +select plugin_name,plugin_status,plugin_library from information_schema.plugins where plugin_name = 'cassandra';
> +uninstall plugin cassandra;
> +select plugin_name,plugin_status,plugin_library from information_schema.plugins where plugin_name = 'cassandra';
> +
> 
> === modified file 'sql/sql_plugin.cc'
> --- sql/sql_plugin.cc	2013-11-13 22:03:48 +0000
> +++ sql/sql_plugin.cc	2013-12-09 11:39:13 +0000
> @@ -309,6 +309,7 @@ static void unlock_variables(THD *thd, s
>  static void cleanup_variables(THD *thd, struct system_variables *vars);
>  static void plugin_vars_free_values(sys_var *vars);
>  static void restore_pluginvar_names(sys_var *first);
> +static void restore_ptr_backup(uint n, st_ptr_backup *backup);
>  static plugin_ref intern_plugin_lock(LEX *lex, plugin_ref plugin);
>  static void intern_plugin_unlock(LEX *lex, plugin_ref plugin);
>  static void reap_plugins(void);
> @@ -473,9 +474,16 @@ static st_plugin_dl *plugin_dl_insert_or
>  #endif /* HAVE_DLOPEN */
>  
>  
> -static inline void free_plugin_mem(struct st_plugin_dl *p)
> +static void free_plugin_mem(struct st_plugin_dl *p)
>  {
>  #ifdef HAVE_DLOPEN
> +  if (p->ptr_backup)
> +  {
> +    DBUG_ASSERT(p->nbackups);
> +    DBUG_ASSERT(p->handle);
> +    restore_ptr_backup(p->nbackups, p->ptr_backup);
> +    my_free(p->ptr_backup);
> +  }
>    if (p->handle)
>      dlclose(p->handle);
>  #endif
> @@ -706,6 +714,7 @@ static st_plugin_dl *plugin_dl_add(const
>    uint plugin_dir_len, dummy_errors, dlpathlen, i;
>    struct st_plugin_dl *tmp= 0, plugin_dl;
>    void *sym;
> +  st_ptr_backup tmp_backup[array_elements(list_of_services)];
>    DBUG_ENTER("plugin_dl_add");
>    DBUG_PRINT("enter", ("dl->str: '%s', dl->length: %d",
>                         dl->str, (int) dl->length));
> @@ -772,7 +781,8 @@ static st_plugin_dl *plugin_dl_add(const
>    {
>      if ((sym= dlsym(plugin_dl.handle, list_of_services[i].name)))
>      {
> -      uint ver= (uint)(intptr)*(void**)sym;
> +      void **ptr= (void **)sym;
> +      uint ver= (uint)(intptr)*ptr;
>        if (ver > list_of_services[i].version ||
>          (ver >> 8) < (list_of_services[i].version >> 8))
>        {
> @@ -783,8 +793,22 @@ static st_plugin_dl *plugin_dl_add(const
>          report_error(report, ER_CANT_OPEN_LIBRARY, dlpath, ENOEXEC, buf);
>          goto ret;
>        }
> -      *(void**)sym= list_of_services[i].service;
> +      tmp_backup[plugin_dl.nbackups++].save(ptr);
> +      *ptr= list_of_services[i].service;
> +    }
> +  }
> +
> +  if (plugin_dl.nbackups)
> +  {
> +    size_t bytes= plugin_dl.nbackups * sizeof(plugin_dl.ptr_backup[0]);
> +    plugin_dl.ptr_backup= (st_ptr_backup *)my_malloc(bytes, MYF(0));
> +    if (!plugin_dl.ptr_backup)
> +    {
> +      restore_ptr_backup(plugin_dl.nbackups, tmp_backup);
> +      report_error(report, ER_OUTOFMEMORY, bytes);
> +      goto ret;
>      }
> +    memcpy(plugin_dl.ptr_backup, tmp_backup, bytes);
>    }
>  
>    /* Duplicate and convert dll name */
> @@ -4017,3 +4041,38 @@ sys_var *find_plugin_sysvar(st_plugin_in
>    return 0;
>  }
>  
> +/*
> +  On dlclose() we need to restore values of all symbols that we've modified in
> +  the DSO. The reason is - the DSO might not actually be unloaded, so on the
> +  next dlopen() these symbols will have old values, they won't be
> +  reinitialized.
> +
> +  Perhaps, there can be many reason, why a DSO won't be unloaded. Strictly
> +  speaking, it's implementation defined whether to unload an unused DSO or to
> +  keep it in memory.
> +
> +  In particular, this happens for some plugins: In 2009 a new ELF stub was
> +  introduced, see Ulrich Drepper's email "Unique symbols for C++"
> +  http://www.redhat.com/archives/posix-c++-wg/2009-August/msg00002.html
> +
> +  DSO that has objects with this stub (STB_GNU_UNIQUE) cannot be unloaded
> +  (this is mentioned in the email, see the url above).
> +
> +  These "unique" objects are, for example, static variables in templates,
> +  in inline functions, in classes. So any DSO that uses them can
> +  only be loaded once. And because Boost has them, any DSO that uses Boost
> +  almost certainly cannot be unloaded.
> +
> +  To know whether a particular DSO has these objects, one can use
> +
> +    readelf -s /path/to/plugin.so|grep UNIQUE
> +
> +  There's nothing we can do about it, but to reset the DSO to its initial
> +  state before dlclose().
> +*/
> +static void restore_ptr_backup(uint n, st_ptr_backup *backup)
> +{
> +  while (n--)
> +    (backup++)->restore();
> +}
> +
> 
> === modified file 'sql/sql_plugin.h'
> --- sql/sql_plugin.h	2013-06-03 07:57:34 +0000
> +++ sql/sql_plugin.h	2013-12-09 11:39:13 +0000
> @@ -81,15 +81,24 @@ typedef struct st_mysql_show_var SHOW_VA
>  
>  /* A handle for the dynamic library containing a plugin or plugins. */
>  
> +struct st_ptr_backup {
> +  void **ptr;
> +  void *value;
> +  void save(void **p) { ptr= p; value= *p; }
> +  void restore() { *ptr= value; }
> +};
> +
>  struct st_plugin_dl
>  {
>    LEX_STRING dl;
>    void *handle;
>    struct st_maria_plugin *plugins;
> +  st_ptr_backup *ptr_backup;
> +  uint nbackups;
> +  uint ref_count;            /* number of plugins loaded from the library */
>    int mysqlversion;
>    int mariaversion;
>    bool   allocated;
> -  uint ref_count;            /* number of plugins loaded from the library */
>  };
>  
>  /* A handle of a plugin */

> ------------------------------------------------------------
> revno: 3928
> committer: Sergei Golubchik <sergii@xxxxxxxxx>
> branch nick: 10.0
> timestamp: Mon 2013-12-09 12:39:19 +0100
> message:
>   remove sys_var specific restore_pluginvar_names() function,
>   use generic restore_ptr_backup() approach
> modified:
>   sql/sql_plugin.cc
>   sql/sql_plugin.h
> diff:
> === modified file 'sql/sql_plugin.cc'
> --- sql/sql_plugin.cc	2013-12-09 11:39:13 +0000
> +++ sql/sql_plugin.cc	2013-12-09 11:39:19 +0000
> @@ -257,15 +257,6 @@ class sys_var_pluginvar: public sys_var
>  public:
>    struct st_plugin_int *plugin;
>    struct st_mysql_sys_var *plugin_var;
> -  /**
> -    variable name from whatever is hard-coded in the plugin source
> -    and doesn't have pluginname- prefix is replaced by an allocated name
> -    with a plugin prefix. When plugin is uninstalled we need to restore the
> -    pointer to point to the hard-coded value, because plugin may be
> -    installed/uninstalled many times without reloading the shared object.
> -  */
> -  const char *orig_pluginvar_name;
> -
>    static void *operator new(size_t size, MEM_ROOT *mem_root)
>    { return (void*) alloc_root(mem_root, size); }
>    static void operator delete(void *ptr_arg,size_t size)
> @@ -278,7 +269,7 @@ class sys_var_pluginvar: public sys_var
>               (plugin_var_arg->flags & PLUGIN_VAR_READONLY ? READONLY : 0),
>               0, -1, NO_ARG, pluginvar_show_type(plugin_var_arg), 0, 0,
>               VARIABLE_NOT_IN_BINLOG, NULL, NULL, NULL),
> -    plugin_var(plugin_var_arg), orig_pluginvar_name(plugin_var_arg->name)
> +    plugin_var(plugin_var_arg)
>    { plugin_var->name= name_arg; }
>    sys_var_pluginvar *cast_pluginvar() { return this; }
>    bool check_update_type(Item_result type);
> @@ -308,7 +299,6 @@ static bool register_builtin(struct st_m
>  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 restore_pluginvar_names(sys_var *first);
>  static void restore_ptr_backup(uint n, st_ptr_backup *backup);
>  static plugin_ref intern_plugin_lock(LEX *lex, plugin_ref plugin);
>  static void intern_plugin_unlock(LEX *lex, plugin_ref plugin);
> @@ -1122,7 +1112,6 @@ static bool plugin_add(MEM_ROOT *tmp_roo
>      if (!(tmp_plugin_ptr= plugin_insert_or_reuse(&tmp)))
>      {
>        mysql_del_sys_var_chain(tmp.system_vars);
> -      restore_pluginvar_names(tmp.system_vars);
>        goto err;
>      }
>      plugin_array_version++;
> @@ -1139,6 +1128,8 @@ static bool plugin_add(MEM_ROOT *tmp_roo
>  
>  err:
>      errs++;
> +    if (tmp.nbackups)
> +      restore_ptr_backup(tmp.nbackups, tmp.ptr_backup);
>      if (name->str)
>        break;
>    }
> @@ -1217,7 +1208,7 @@ static void plugin_del(struct st_plugin_
>    mysql_rwlock_wrlock(&LOCK_system_variables_hash);
>    mysql_del_sys_var_chain(plugin->system_vars);
>    mysql_rwlock_unlock(&LOCK_system_variables_hash);
> -  restore_pluginvar_names(plugin->system_vars);
> +  restore_ptr_backup(plugin->nbackups, plugin->ptr_backup);
>    plugin_vars_free_values(plugin->system_vars);
>    my_hash_delete(&plugin_hash[plugin->plugin->type], (uchar*)plugin);
>    plugin_dl_del(plugin->plugin_dl);
> @@ -2938,16 +2929,6 @@ static st_bookmark *register_var(const c
>    return result;
>  }
>  
> -static void restore_pluginvar_names(sys_var *first)
> -{
> -  for (sys_var *var= first; var; var= var->next)
> -  {
> -    sys_var_pluginvar *pv= var->cast_pluginvar();
> -    pv->plugin_var->name= pv->orig_pluginvar_name;
> -  }
> -}
> -
> -
>  /*
>    returns a pointer to the memory which holds the thd-local variable or
>    a pointer to the global variable if thd==null.
> @@ -3823,7 +3804,7 @@ static my_option *construct_help_options
>      to get the correct (not double-prefixed) help text.
>      We won't need @@sysvars anymore and don't care about their proper names.
>    */
> -  restore_pluginvar_names(p->system_vars);
> +  restore_ptr_backup(p->nbackups, p->ptr_backup);
>  
>    if (construct_options(mem_root, p, opts))
>      DBUG_RETURN(NULL);
> @@ -3868,6 +3849,7 @@ static int test_plugin_options(MEM_ROOT
>    sys_var *v __attribute__((unused));
>    struct st_bookmark *var;
>    uint len, count= EXTRA_OPTIONS;
> +  st_ptr_backup *tmp_backup= 0;
>    DBUG_ENTER("test_plugin_options");
>    DBUG_ASSERT(tmp->plugin && tmp->name.str);
>  
> @@ -3940,59 +3922,85 @@ static int test_plugin_options(MEM_ROOT
>      plugin_name= tmp->name;
>  
>    error= 1;
> -  for (opt= tmp->plugin->system_vars; opt && *opt; opt++)
> +
> +  if (tmp->plugin->system_vars)
>    {
> -    st_mysql_sys_var *o= *opt;
> +    for (len=0, opt= tmp->plugin->system_vars; *opt; len++, opt++) /* no-op */;
> +    tmp_backup= (st_ptr_backup *)my_alloca(len * sizeof(tmp_backup[0]));
> +    DBUG_ASSERT(tmp->nbackups == 0);
> +    DBUG_ASSERT(tmp->ptr_backup == 0);
>  
> -    /*
> -      PLUGIN_VAR_STR command-line options without PLUGIN_VAR_MEMALLOC, point
> -      directly to values in the argv[] array. For plugins started at the
> -      server startup, argv[] array is allocated with load_defaults(), and
> -      freed when the server is shut down.  But for plugins loaded with
> -      INSTALL PLUGIN, the memory allocated with load_defaults() is freed with
> -      freed() at the end of mysql_install_plugin(). Which means we cannot
> -      allow any pointers into that area.
> -      Thus, for all plugins loaded after the server was started,
> -      we copy string values to a plugin's memroot.
> -    */
> -    if (mysqld_server_started &&
> -        ((o->flags & (PLUGIN_VAR_STR | PLUGIN_VAR_NOCMDOPT |
> -                       PLUGIN_VAR_MEMALLOC)) == PLUGIN_VAR_STR))
> +    for (opt= tmp->plugin->system_vars; *opt; opt++)
>      {
> -      sysvar_str_t* str= (sysvar_str_t *)o;
> -      if (*str->value)
> -        *str->value= strdup_root(mem_root, *str->value);
> -    }
> +      st_mysql_sys_var *o= *opt;
>  
> -    if (o->flags & PLUGIN_VAR_NOSYSVAR)
> -      continue;
> -    if ((var= find_bookmark(plugin_name.str, o->name, o->flags)))
> -      v= new (mem_root) sys_var_pluginvar(&chain, var->key + 1, o);
> -    else
> +      /*
> +        PLUGIN_VAR_STR command-line options without PLUGIN_VAR_MEMALLOC, point
> +        directly to values in the argv[] array. For plugins started at the
> +        server startup, argv[] array is allocated with load_defaults(), and
> +        freed when the server is shut down.  But for plugins loaded with
> +        INSTALL PLUGIN, the memory allocated with load_defaults() is freed with
> +        freed() at the end of mysql_install_plugin(). Which means we cannot
> +        allow any pointers into that area.
> +        Thus, for all plugins loaded after the server was started,
> +        we copy string values to a plugin's memroot.
> +      */
> +      if (mysqld_server_started &&
> +          ((o->flags & (PLUGIN_VAR_STR | PLUGIN_VAR_NOCMDOPT |
> +                         PLUGIN_VAR_MEMALLOC)) == PLUGIN_VAR_STR))
> +      {
> +        sysvar_str_t* str= (sysvar_str_t *)o;
> +        if (*str->value)
> +          *str->value= strdup_root(mem_root, *str->value);
> +      }
> +
> +      if (o->flags & PLUGIN_VAR_NOSYSVAR)
> +        continue;
> +      tmp_backup[tmp->nbackups++].save(&o->name);
> +      if ((var= find_bookmark(plugin_name.str, o->name, o->flags)))
> +        v= new (mem_root) sys_var_pluginvar(&chain, var->key + 1, o);
> +      else
> +      {
> +        len= plugin_name.length + strlen(o->name) + 2;
> +        varname= (char*) alloc_root(mem_root, len);
> +        strxmov(varname, plugin_name.str, "-", o->name, NullS);
> +        my_casedn_str(&my_charset_latin1, varname);
> +        convert_dash_to_underscore(varname, len-1);
> +        v= new (mem_root) sys_var_pluginvar(&chain, varname, o);
> +      }
> +      DBUG_ASSERT(v); /* check that an object was actually constructed */
> +    } /* end for */
> +
> +    if (tmp->nbackups)
>      {
> -      len= plugin_name.length + strlen(o->name) + 2;
> -      varname= (char*) alloc_root(mem_root, len);
> -      strxmov(varname, plugin_name.str, "-", o->name, NullS);
> -      my_casedn_str(&my_charset_latin1, varname);
> -      convert_dash_to_underscore(varname, len-1);
> -      v= new (mem_root) sys_var_pluginvar(&chain, varname, o);
> -    }
> -    DBUG_ASSERT(v); /* check that an object was actually constructed */
> -  } /* end for */
> -  if (chain.first)
> -  {
> -    chain.last->next = NULL;
> -    if (mysql_add_sys_var_chain(chain.first))
> +      size_t bytes= tmp->nbackups * sizeof(tmp->ptr_backup[0]);
> +      tmp->ptr_backup= (st_ptr_backup *)alloc_root(mem_root, bytes);
> +      if (!tmp->ptr_backup)
> +      {
> +        restore_ptr_backup(tmp->nbackups, tmp_backup);
> +        goto err;
> +      }
> +      memcpy(tmp->ptr_backup, tmp_backup, bytes);
> +    }
> +
> +    if (chain.first)
>      {
> -      sql_print_error("Plugin '%s' has conflicting system variables",
> -                      tmp->name.str);
> -      goto err;
> +      chain.last->next = NULL;
> +      if (mysql_add_sys_var_chain(chain.first))
> +      {
> +        sql_print_error("Plugin '%s' has conflicting system variables",
> +                        tmp->name.str);
> +        goto err;
> +      }
> +      tmp->system_vars= chain.first;
>      }
> -    tmp->system_vars= chain.first;
>    }
> +
>    DBUG_RETURN(0);
>    
>  err:
> +  if (tmp_backup)
> +    my_afree(tmp_backup);
>    if (opts)
>      my_cleanup_options(opts);
>    DBUG_RETURN(error);
> 
> === modified file 'sql/sql_plugin.h'
> --- sql/sql_plugin.h	2013-12-09 11:39:13 +0000
> +++ sql/sql_plugin.h	2013-12-09 11:39:19 +0000
> @@ -85,6 +85,7 @@ struct st_ptr_backup {
>    void **ptr;
>    void *value;
>    void save(void **p) { ptr= p; value= *p; }
> +  void save(const char **p) { save((void**)p); }
>    void restore() { *ptr= value; }
>  };
>  
> @@ -108,6 +109,8 @@ struct st_plugin_int
>    LEX_STRING name;
>    struct st_maria_plugin *plugin;
>    struct st_plugin_dl *plugin_dl;
> +  st_ptr_backup *ptr_backup;
> +  uint nbackups;
>    uint state;
>    uint ref_count;               /* number of threads using the plugin */
>    uint locks_total;             /* how many times the plugin was locked */