bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:2812) Bug#40552

[Alexey Kopytov]

#At file:///Users/kaa/src/bzr/bugteam/bug40552/my51-bug40552/ based on
revid:azundris@stripped

 2812 Alexey Kopytov	2009-02-27
      Fix for bug #40552: Race condition around default_directories  
                          in load_defaults() 
      
      load_defaults(), my_search_option_files() and 
      my_print_default_files()  utilized a global variable 
      containing  a pointer to thread local memory. This could lead 
      to race conditions when those functions were called with high 
      concurrency. 
      
      Fixed by changing the interface of the said functions to avoid 
      the necessity for using a global variable.
      
      Since we cannot change load_defaults() prototype for API
      compatibility reasons, it was renamed my_load_defaults().
      Now load_defaults() is a thread-unsafe wrapper around
      a thread-safe version, my_load_defaults().
      modified:
        include/my_sys.h
        mysys/default.c
        server-tools/instance-manager/instance_map.cc
        server-tools/instance-manager/options.cc
        server-tools/instance-manager/options.h
        sql-common/client.c

per-file messages:
  mysys/default.c
    1. Added a thread-safe version of load_defaults(), changed
    load_defaults() with the old interface to be a thread-unsafe
    wrapper around the thread-safe version.
    2. Always use a private MEM_ROOT in my_print_default_files, 
    don't use a global variable.
  sql-common/client.c
    Use a thread-safe version of load_defaults().
=== modified file 'include/my_sys.h'
--- a/include/my_sys.h	2009-02-05 06:16:00 +0000
+++ b/include/my_sys.h	2009-02-27 09:26:06 +0000
@@ -842,14 +842,17 @@ extern void *memdup_root(MEM_ROOT *root,
 extern int get_defaults_options(int argc, char **argv,
                                 char **defaults, char **extra_defaults,
                                 char **group_suffix);
+extern int my_load_defaults(const char *conf_file, const char **groups,
+                            int *argc, char ***argv, const char ***);
 extern int load_defaults(const char *conf_file, const char **groups,
-			 int *argc, char ***argv);
+                         int *argc, char ***argv);
 extern int modify_defaults_file(const char *file_location, const char *option,
                                 const char *option_value,
                                 const char *section_name, int remove_option);
 extern int my_search_option_files(const char *conf_file, int *argc,
                                   char ***argv, uint *args_used,
-                                  Process_option_func func, void *func_ctx);
+                                  Process_option_func func, void *func_ctx,
+                                  const char **default_directories);
 extern void free_defaults(char **argv);
 extern void my_print_default_files(const char *conf_file);
 extern void print_defaults(const char *conf_file, const char **groups);

=== modified file 'mysys/default.c'
--- a/mysys/default.c	2008-07-22 11:33:37 +0000
+++ b/mysys/default.c	2009-02-27 09:26:06 +0000
@@ -151,7 +151,7 @@ static char *remove_end_comment(char *pt
 
 int my_search_option_files(const char *conf_file, int *argc, char ***argv,
                            uint *args_used, Process_option_func func,
-                           void *func_ctx)
+                           void *func_ctx, const char **default_directories)
 {
   const char **dirs, *forced_default_file, *forced_extra_defaults;
   int error= 0;
@@ -359,18 +359,46 @@ int get_defaults_options(int argc, char 
   return org_argc - argc;
 }
 
+/*
+  Wrapper around my_load_defaults() for interface compatibility.
+
+  SYNOPSIS
+    load_defaults()
+    conf_file			Basename for configuration file to search for.
+    				If this is a path, then only this file is read.
+    groups			Which [group] entrys to read.
+				Points to an null terminated array of pointers
+    argc			Pointer to argc of original program
+    argv			Pointer to argv of original program
+
+  NOTES
+
+    This function is NOT thread-safe as it uses a global pointer internally.
+    See also notes for my_load_defaults().
+
+  RETURN
+    0 ok
+    1 The given conf_file didn't exists
+*/
+int load_defaults(const char *conf_file, const char **groups,
+                  int *argc, char ***argv)
+{
+  return my_load_defaults(conf_file, groups, argc, argv, &default_directories);
+}
 
 /*
   Read options from configurations files
 
   SYNOPSIS
-    load_defaults()
+    my_load_defaults()
     conf_file			Basename for configuration file to search for.
     				If this is a path, then only this file is read.
     groups			Which [group] entrys to read.
 				Points to an null terminated array of pointers
     argc			Pointer to argc of original program
     argv			Pointer to argv of original program
+    default_directories         Pointer to a location where a pointer to the list
+                                of default directories will be stored
 
   IMPLEMENTATION
 
@@ -386,13 +414,18 @@ int get_defaults_options(int argc, char 
     that was put in *argv
 
    RETURN
-     0	ok
-     1	The given conf_file didn't exists
+     - If successful, 0 is returned. If 'default_directories' is not NULL,
+     a pointer to the array of default directory paths is stored to a location
+     it points to. That stored value must be passed to my_search_option_files()
+     later.
+     
+     - 1 is returned if the given conf_file didn't exist. In this case, the
+     value pointed to by default_directories is undefined.
 */
 
 
-int load_defaults(const char *conf_file, const char **groups,
-                  int *argc, char ***argv)
+int my_load_defaults(const char *conf_file, const char **groups,
+                  int *argc, char ***argv, const char ***default_directories)
 {
   DYNAMIC_ARRAY args;
   TYPELIB group;
@@ -402,10 +435,11 @@ int load_defaults(const char *conf_file,
   MEM_ROOT alloc;
   char *ptr,**res;
   struct handle_option_ctx ctx;
+  const char **dirs;
   DBUG_ENTER("load_defaults");
 
   init_alloc_root(&alloc,512,0);
-  if ((default_directories= init_default_directories(&alloc)) == NULL)
+  if ((dirs= init_default_directories(&alloc)) == NULL)
     goto err;
   /*
     Check if the user doesn't want any default option processing
@@ -426,6 +460,8 @@ int load_defaults(const char *conf_file,
     (*argc)--;
     *argv=res;
     *(MEM_ROOT*) ptr= alloc;			/* Save alloc root for free */
+    if (default_directories)
+      *default_directories= dirs;
     DBUG_RETURN(0);
   }
 
@@ -444,7 +480,8 @@ int load_defaults(const char *conf_file,
   ctx.group= &group;
 
   error= my_search_option_files(conf_file, argc, argv, &args_used,
-                                handle_default_option, (void *) &ctx);
+                                handle_default_option, (void *) &ctx,
+                                dirs);
   /*
     Here error contains <> 0 only if we have a fully specified conf_file
     or a forced default file
@@ -490,6 +527,10 @@ int load_defaults(const char *conf_file,
     puts("");
     exit(0);
   }
+
+  if (error == 0 && default_directories)
+    *default_directories= dirs;
+
   DBUG_RETURN(error);
 
  err:
@@ -895,15 +936,11 @@ void my_print_default_files(const char *
     fputs(conf_file,stdout);
   else
   {
-    /*
-      If default_directories is already initialized, use it.  Otherwise,
-      use a private MEM_ROOT.
-    */
-    const char **dirs = default_directories;
+    const char **dirs;
     MEM_ROOT alloc;
     init_alloc_root(&alloc,512,0);
 
-    if (!dirs && (dirs= init_default_directories(&alloc)) == NULL)
+    if ((dirs= init_default_directories(&alloc)) == NULL)
     {
       fputs("Internal error initializing default directories list", stdout);
     }

=== modified file 'server-tools/instance-manager/instance_map.cc'
--- a/server-tools/instance-manager/instance_map.cc	2007-05-10 09:59:39 +0000
+++ b/server-tools/instance-manager/instance_map.cc	2009-02-27 09:26:06 +0000
@@ -536,7 +536,8 @@ int Instance_map::load()
   */
   if (my_search_option_files(Options::Main::config_file, &argc,
                              (char ***) &argv, &args_used,
-                             process_option, (void*) this))
+                             process_option, (void*) this,
+                             Options::default_directories))
     log_info("Falling back to compiled-in defaults.");
 
   return complete_initialization();

=== modified file 'server-tools/instance-manager/options.cc'
--- a/server-tools/instance-manager/options.cc	2007-05-10 09:59:39 +0000
+++ b/server-tools/instance-manager/options.cc	2009-02-27 09:26:06 +0000
@@ -86,6 +86,7 @@ const char *Options::Main::bind_address=
 uint Options::Main::monitoring_interval= DEFAULT_MONITORING_INTERVAL;
 uint Options::Main::port_number= DEFAULT_PORT;
 my_bool Options::Main::mysqld_safe_compatible= FALSE;
+const char **Options::default_directories= NULL;
 
 /* Options::User_management */
 
@@ -439,7 +440,8 @@ int Options::load(int argc, char **argv)
   log_info("Loading config file '%s'...",
            (const char *) Main::config_file);
 
-  load_defaults(Main::config_file, default_groups, &argc, &saved_argv);
+  my_load_defaults(Main::config_file, default_groups, &argc,
+                   &saved_argv, &default_directories);
 
   if ((handle_options(&argc, &saved_argv, my_long_options, get_one_option)))
     return ERR_INVALID_USAGE;

=== modified file 'server-tools/instance-manager/options.h'
--- a/server-tools/instance-manager/options.h	2007-01-27 01:46:45 +0000
+++ b/server-tools/instance-manager/options.h	2009-02-27 09:26:06 +0000
@@ -91,6 +91,9 @@ struct Options
 #endif
 
 public:
+  /* Array of paths to be passed to my_search_option_files() later */
+  static const char **default_directories;
+
   static int load(int argc, char **argv);
   static void cleanup();
 

=== modified file 'sql-common/client.c'
--- a/sql-common/client.c	2008-03-29 08:02:54 +0000
+++ b/sql-common/client.c	2009-02-27 09:26:06 +0000
@@ -1021,7 +1021,7 @@ void mysql_read_default_options(struct s
   argc=1; argv=argv_buff; argv_buff[0]= (char*) "client";
   groups[0]= (char*) "client"; groups[1]= (char*) group; groups[2]=0;
 
-  load_defaults(filename, groups, &argc, &argv);
+  my_load_defaults(filename, groups, &argc, &argv, NULL);
   if (argc != 1)				/* If some default option */
   {
     char **option=argv;