Re: [HACKERS] bug in GUC - Mailing list pgsql-patches

Tom Lane wrote:
> Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
>
> ... We don't want to elog(ERROR) partway through that
> process.  Especially not in the postmaster, where elog(ERROR) is
> tantamount to elog(FATAL).  (But of course the postmaster shouldn't ever
> run out of memory anyway...)
>
> It's possible that this should all be rethought, but it would be a much
> more wide-ranging change than we've been discussing.
>
>             regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings
>

Here's a patch dealing with the unchecked mallocs and strdups in guc.c.
Rather than mixing in palloc and TopMemoryContext into the code (would
be rather messy given that most allocs actually falls into the category
of not always being permitted to do elog(ERROR/FATAL)), I added a couple
of simple static functions for guc_alloc, guc_realloc, and guc_strdup
that are used throughout the guc.c file.

Kind regards,

Thomas Hallgren
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/misc/guc.c,v
retrieving revision 1.211
diff -u -r1.211 guc.c
--- src/backend/utils/misc/guc.c    11 Jun 2004 03:54:54 -0000    1.211
+++ src/backend/utils/misc/guc.c    27 Jun 2004 18:43:18 -0000
@@ -1786,6 +1786,32 @@
 static void ReportGUCOption(struct config_generic * record);
 static char *_ShowOption(struct config_generic * record);

+static void* check_alloc(int elevel, void* data)
+{
+    if(data == NULL)
+    {
+        ereport(elevel,
+            (errcode(ERRCODE_OUT_OF_MEMORY),
+                 errmsg("out of memory")));
+    }
+    return data;
+}
+
+static void* guc_alloc(int elevel, size_t size)
+{
+    return check_alloc(elevel, malloc(size));
+}
+
+static void* guc_realloc(int elevel, void* old, size_t size)
+{
+    return check_alloc(elevel, realloc(old, size));
+}
+
+static char* guc_strdup(int elevel, const char* src)
+{
+    return (char*)check_alloc(elevel, strdup(src));
+}
+
 struct config_generic** get_guc_variables()
 {
     return guc_variables;
@@ -1842,11 +1868,7 @@
     size_vars = num_vars + num_vars / 4;

     guc_vars = (struct config_generic **)
-        malloc(size_vars * sizeof(struct config_generic *));
-    if (!guc_vars)
-        ereport(FATAL,
-                (errcode(ERRCODE_OUT_OF_MEMORY),
-                 errmsg("out of memory")));
+        guc_alloc(FATAL, size_vars * sizeof(struct config_generic *));

     num_vars = 0;

@@ -1905,35 +1927,41 @@
  * Add a new GUC variable to the list of known variables. The
  * list is expanded if needed.
  */
-static void
-add_guc_variable(struct config_generic *var)
+static bool
+add_guc_variable(int elevel, struct config_generic *var)
 {
     if(num_guc_variables + 1 >= size_guc_variables)
     {
         /* Increase the vector with 20%
          */
-        int size_vars = size_guc_variables + size_guc_variables / 4;
+        int size_vars = size_guc_variables + size_guc_variables / 5;
         struct config_generic** guc_vars;

         if(size_vars == 0)
+        {
             size_vars = 100;
-
-        guc_vars = (struct config_generic**)
-                    malloc(size_vars * sizeof(struct config_generic*));
-
-        if (guc_variables != NULL)
+            guc_vars = (struct config_generic**)
+                    guc_alloc(elevel, size_vars * sizeof(struct config_generic*));
+        }
+        else
         {
-            memcpy(guc_vars, guc_variables,
-                    num_guc_variables * sizeof(struct config_generic*));
-            free(guc_variables);
+            guc_vars = (struct config_generic**)
+                    guc_realloc(elevel, guc_variables, size_vars * sizeof(struct config_generic*));
         }

-        guc_variables = guc_vars;
+        if(guc_vars == NULL)
+            /*
+             * Out of memory
+             */
+            return false;
+
         size_guc_variables = size_vars;
+        guc_variables = guc_vars;
     }
     guc_variables[num_guc_variables++] = var;
     qsort((void*) guc_variables, num_guc_variables,
         sizeof(struct config_generic*), guc_var_compare);
+    return true;
 }

 /*
@@ -1941,15 +1969,22 @@
  * to a valid custom variable class at this point.
  */
 static struct config_string*
-add_placeholder_variable(const char *name)
+add_placeholder_variable(int elevel, const char *name)
 {
     size_t sz = sizeof(struct config_string) + sizeof(char*);
-    struct config_string*  var = (struct config_string*)malloc(sz);
-    struct config_generic* gen = &var->gen;
+    struct config_generic* gen;
+
+    struct config_string*  var = (struct config_string*)guc_alloc(elevel, sz);
+    if(var == NULL)
+        return NULL;

+    gen = &var->gen;
     memset(var, 0, sz);

-    gen->name       = strdup(name);
+    gen->name = guc_strdup(elevel, name);
+    if(gen->name == NULL)
+        return NULL;
+
     gen->context    = PGC_USERSET;
     gen->group      = CUSTOM_OPTIONS;
     gen->short_desc = "GUC placeholder variable";
@@ -1960,7 +1995,8 @@
      * no 'static' place to point to.
      */
     var->variable = (char**)(var + 1);
-    add_guc_variable((struct config_generic*)var);
+    if(!add_guc_variable(elevel, (struct config_generic*)var))
+        var = NULL;
     return var;
 }

@@ -1969,7 +2005,7 @@
  * else return NULL.
  */
 static struct config_generic *
-find_option(const char *name)
+find_option(int elevel, const char *name)
 {
     const char *dot;
     const char **key = &name;
@@ -1998,7 +2034,7 @@
     for (i = 0; map_old_guc_names[i] != NULL; i += 2)
     {
         if (guc_name_compare(name, map_old_guc_names[i]) == 0)
-            return find_option(map_old_guc_names[i+1]);
+            return find_option(elevel, map_old_guc_names[i+1]);
     }

     /* Check if the name is qualified, and if so, check if the qualifier
@@ -2009,7 +2045,7 @@
         /*
          * Add a placeholder variable for this name
          */
-        return (struct config_generic*)add_placeholder_variable(name);
+        return (struct config_generic*)add_placeholder_variable(elevel, name);

     /* Unknown name */
     return NULL;
@@ -2159,11 +2195,7 @@
                         break;
                     }

-                    str = strdup(conf->boot_val);
-                    if (str == NULL)
-                        ereport(FATAL,
-                                (errcode(ERRCODE_OUT_OF_MEMORY),
-                                 errmsg("out of memory")));
+                    str = guc_strdup(FATAL, conf->boot_val);
                     conf->reset_val = str;

                     if (conf->assign_hook)
@@ -2710,7 +2742,7 @@
     else
         elevel = ERROR;

-    record = find_option(name);
+    record = find_option(elevel, name);
     if (record == NULL)
     {
         ereport(elevel,
@@ -3142,14 +3174,9 @@

                 if (value)
                 {
-                    newval = strdup(value);
+                    newval = guc_strdup(elevel, value);
                     if (newval == NULL)
-                    {
-                        ereport(elevel,
-                                (errcode(ERRCODE_OUT_OF_MEMORY),
-                                 errmsg("out of memory")));
                         return false;
-                    }

                     if (record->context == PGC_USERLIMIT)
                     {
@@ -3200,14 +3227,9 @@
                      * make this case work the same as the normal
                      * assignment case.
                      */
-                    newval = strdup(conf->reset_val);
+                    newval = guc_strdup(elevel, conf->reset_val);
                     if (newval == NULL)
-                    {
-                        ereport(elevel,
-                                (errcode(ERRCODE_OUT_OF_MEMORY),
-                                 errmsg("out of memory")));
                         return false;
-                    }
                     source = conf->gen.reset_source;
                 }
                 else
@@ -3338,7 +3360,7 @@
     struct config_generic *record;
     static char buffer[256];

-    record = find_option(name);
+    record = find_option(ERROR, name);
     if (record == NULL)
         ereport(ERROR,
                 (errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -3374,7 +3396,7 @@
     struct config_generic *record;
     static char buffer[256];

-    record = find_option(name);
+    record = find_option(ERROR, name);
     if (record == NULL)
         ereport(ERROR,
                 (errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -3430,7 +3452,7 @@
         return NULL;

     /* Else get flags for the variable */
-    record = find_option(name);
+    record = find_option(ERROR, name);
     if (record == NULL)
         ereport(ERROR,
                 (errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -3601,7 +3623,7 @@

     if(res == NULL)
     {
-        add_guc_variable(variable);
+        add_guc_variable(ERROR, variable);
         return;
     }

@@ -3658,7 +3680,7 @@
     GucContext  context,
     enum config_type type)
 {
-    gen->name       = strdup(name);
+    gen->name       = guc_strdup(ERROR, name);
     gen->context    = context;
     gen->group      = CUSTOM_OPTIONS;
     gen->short_desc = short_desc;
@@ -3676,7 +3698,7 @@
     GucShowHook show_hook)
 {
     size_t sz = sizeof(struct config_bool);
-    struct config_bool*  var = (struct config_bool*)malloc(sz);
+    struct config_bool* var = (struct config_bool*)guc_alloc(ERROR, sz);

     memset(var, 0, sz);
     init_custom_variable(&var->gen, name, short_desc, long_desc, context, PGC_BOOL);
@@ -3698,7 +3720,7 @@
     GucShowHook show_hook)
 {
     size_t sz = sizeof(struct config_int);
-    struct config_int*  var = (struct config_int*)malloc(sz);
+    struct config_int*  var = (struct config_int*)guc_alloc(ERROR, sz);

     memset(var, 0, sz);
     init_custom_variable(&var->gen, name, short_desc, long_desc, context, PGC_INT);
@@ -3720,7 +3742,7 @@
     GucShowHook show_hook)
 {
     size_t sz = sizeof(struct config_real);
-    struct config_real*  var = (struct config_real*)malloc(sz);
+    struct config_real*  var = (struct config_real*)guc_alloc(ERROR, sz);

     memset(var, 0, sz);
     init_custom_variable(&var->gen, name, short_desc, long_desc, context, PGC_REAL);
@@ -3742,7 +3764,7 @@
     GucShowHook show_hook)
 {
     size_t sz = sizeof(struct config_string);
-    struct config_string*  var = (struct config_string*)malloc(sz);
+    struct config_string*  var = (struct config_string*)guc_alloc(ERROR, sz);

     memset(var, 0, sz);
     init_custom_variable(&var->gen, name, short_desc, long_desc, context, PGC_STRING);
@@ -3914,7 +3936,7 @@
 {
     struct config_generic *record;

-    record = find_option(name);
+    record = find_option(ERROR, name);
     if (record == NULL)
         ereport(ERROR,
                 (errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -4281,16 +4303,15 @@
     /*
      * Open file
      */
-    new_filename = malloc(strlen(DataDir) + strlen(CONFIG_EXEC_PARAMS) +
+    new_filename = guc_alloc(elevel, strlen(DataDir) + strlen(CONFIG_EXEC_PARAMS) +
                           strlen(".new") + 2);
-    filename = malloc(strlen(DataDir) + strlen(CONFIG_EXEC_PARAMS) + 2);
-    if (new_filename == NULL || filename == NULL)
-    {
-        ereport(elevel,
-                (errcode(ERRCODE_OUT_OF_MEMORY),
-                 errmsg("out of memory")));
+    if(new_filename == NULL)
         return;
-    }
+
+    filename = guc_alloc(elevel, strlen(DataDir) + strlen(CONFIG_EXEC_PARAMS) + 2);
+    if (filename == NULL)
+        return;
+
     sprintf(new_filename, "%s/" CONFIG_EXEC_PARAMS ".new", DataDir);
     sprintf(filename, "%s/" CONFIG_EXEC_PARAMS, DataDir);

@@ -4402,9 +4423,9 @@
                 elog(FATAL, "invalid format of exec config params file");
         }
         if (i == 0)
-            str = malloc(maxlen);
+            str = guc_alloc(FATAL, maxlen);
         else if (i == maxlen)
-            str = realloc(str, maxlen *= 2);
+            str = guc_realloc(FATAL, str, maxlen *= 2);
         str[i++] = ch;
     } while (ch != 0);

@@ -4430,14 +4451,9 @@
     /*
      * Open file
      */
-    filename = malloc(strlen(DataDir) + strlen(CONFIG_EXEC_PARAMS) + 2);
+    filename = guc_alloc(ERROR, strlen(DataDir) + strlen(CONFIG_EXEC_PARAMS) + 2);
     if (filename == NULL)
-    {
-        ereport(ERROR,
-                (errcode(ERRCODE_OUT_OF_MEMORY),
-                 errmsg("out of memory")));
         return;
-    }
     sprintf(filename, "%s/" CONFIG_EXEC_PARAMS, DataDir);

     fp = AllocateFile(filename, "r");
@@ -4459,7 +4475,7 @@
         if ((varname = read_string_with_null(fp)) == NULL)
             break;

-        if ((record = find_option(varname)) == NULL)
+        if ((record = find_option(FATAL, varname)) == NULL)
             elog(FATAL, "failed to locate variable %s in exec config params file",varname);
         if ((varvalue = read_string_with_null(fp)) == NULL)
             elog(FATAL, "invalid format of exec config params file");
@@ -4500,28 +4516,16 @@

     if (string[equal_pos] == '=')
     {
-        *name = malloc(equal_pos + 1);
-        if (!*name)
-            ereport(FATAL,
-                    (errcode(ERRCODE_OUT_OF_MEMORY),
-                     errmsg("out of memory")));
+        *name = guc_alloc(FATAL, equal_pos + 1);
         strncpy(*name, string, equal_pos);
         (*name)[equal_pos] = '\0';

-        *value = strdup(&string[equal_pos + 1]);
-        if (!*value)
-            ereport(FATAL,
-                    (errcode(ERRCODE_OUT_OF_MEMORY),
-                     errmsg("out of memory")));
+        *value = guc_strdup(FATAL, &string[equal_pos + 1]);
     }
     else
     {
         /* no equal sign in string */
-        *name = strdup(string);
-        if (!*name)
-            ereport(FATAL,
-                    (errcode(ERRCODE_OUT_OF_MEMORY),
-                     errmsg("out of memory")));
+        *name = guc_strdup(FATAL, string);
         *value = NULL;
     }