Commit bf007a27 authored by Tom Lane's avatar Tom Lane

Clean up assorted issues in ALTER SYSTEM coding.

Fix unsafe use of a non-volatile variable in PG_TRY/PG_CATCH in
AlterSystemSetConfigFile().  While at it, clean up a bundle of other
infelicities and outright bugs, including corner-case-incorrect linked list
manipulation, a poorly designed and worse documented parse-and-validate
function (which even included some randomly chosen hard-wired substitutes
for the specified elevel in one code path ... wtf?), direct use of open()
instead of fd.c's facilities, inadequate checking of write()'s return
value, and generally poorly written commentary.
parent fd496129
...@@ -118,11 +118,11 @@ ProcessConfigFile(GucContext context) ...@@ -118,11 +118,11 @@ ProcessConfigFile(GucContext context)
bool error = false; bool error = false;
bool apply = false; bool apply = false;
int elevel; int elevel;
const char *ConfFileWithError;
ConfigVariable *item, ConfigVariable *item,
*head, *head,
*tail; *tail;
int i; int i;
char *ErrorConfFile = ConfigFileName;
/* /*
* Config files are processed on startup (by the postmaster only) * Config files are processed on startup (by the postmaster only)
...@@ -138,6 +138,7 @@ ProcessConfigFile(GucContext context) ...@@ -138,6 +138,7 @@ ProcessConfigFile(GucContext context)
elevel = IsUnderPostmaster ? DEBUG2 : LOG; elevel = IsUnderPostmaster ? DEBUG2 : LOG;
/* Parse the main config file into a list of option names and values */ /* Parse the main config file into a list of option names and values */
ConfFileWithError = ConfigFileName;
head = tail = NULL; head = tail = NULL;
if (!ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, &head, &tail)) if (!ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, &head, &tail))
...@@ -160,25 +161,23 @@ ProcessConfigFile(GucContext context) ...@@ -160,25 +161,23 @@ ProcessConfigFile(GucContext context)
{ {
/* Syntax error(s) detected in the file, so bail out */ /* Syntax error(s) detected in the file, so bail out */
error = true; error = true;
ErrorConfFile = PG_AUTOCONF_FILENAME; ConfFileWithError = PG_AUTOCONF_FILENAME;
goto cleanup_list; goto cleanup_list;
} }
} }
else else
{ {
ConfigVariable *prev = NULL;
/* /*
* Pick up only the data_directory if DataDir is not set, which * If DataDir is not set, the PG_AUTOCONF_FILENAME file cannot be
* means that the configuration file is read for the first time and * read. In this case, we don't want to accept any settings but
* PG_AUTOCONF_FILENAME file cannot be read yet. In this case, * data_directory from postgresql.conf, because they might be
* we shouldn't pick any settings except the data_directory * overwritten with settings in the PG_AUTOCONF_FILENAME file which
* from postgresql.conf because they might be overwritten * will be read later. OTOH, since data_directory isn't allowed in the
* with the settings in PG_AUTOCONF_FILENAME file which will be * PG_AUTOCONF_FILENAME file, it will never be overwritten later.
* read later. OTOH, since it's ensured that data_directory doesn't
* exist in PG_AUTOCONF_FILENAME file, it will never be overwritten
* later.
*/ */
ConfigVariable *prev = NULL;
/* Prune all items except "data_directory" from the list */
for (item = head; item;) for (item = head; item;)
{ {
ConfigVariable *ptr = item; ConfigVariable *ptr = item;
...@@ -189,15 +188,9 @@ ProcessConfigFile(GucContext context) ...@@ -189,15 +188,9 @@ ProcessConfigFile(GucContext context)
if (prev == NULL) if (prev == NULL)
head = ptr->next; head = ptr->next;
else else
{
prev->next = ptr->next; prev->next = ptr->next;
/* if (ptr->next == NULL)
* On removing last item in list, we need to update tail tail = prev;
* to ensure that list will be maintianed.
*/
if (prev->next == NULL)
tail = prev;
}
FreeConfigVariable(ptr); FreeConfigVariable(ptr);
} }
else else
...@@ -205,10 +198,10 @@ ProcessConfigFile(GucContext context) ...@@ -205,10 +198,10 @@ ProcessConfigFile(GucContext context)
} }
/* /*
* Quick exit if data_directory is not present in list. * Quick exit if data_directory is not present in file.
* *
* Don't remember when we last successfully loaded the config file in * We need not do any further processing, in particular we don't set
* this case because that time will be set soon by subsequent load of * PgReloadTime; that will be set soon by subsequent full loading of
* the config file. * the config file.
*/ */
if (head == NULL) if (head == NULL)
...@@ -263,7 +256,7 @@ ProcessConfigFile(GucContext context) ...@@ -263,7 +256,7 @@ ProcessConfigFile(GucContext context)
item->name, item->name,
item->filename, item->sourceline))); item->filename, item->sourceline)));
error = true; error = true;
ErrorConfFile = item->filename; ConfFileWithError = item->filename;
} }
} }
...@@ -392,7 +385,7 @@ ProcessConfigFile(GucContext context) ...@@ -392,7 +385,7 @@ ProcessConfigFile(GucContext context)
else if (scres == 0) else if (scres == 0)
{ {
error = true; error = true;
ErrorConfFile = item->filename; ConfFileWithError = item->filename;
} }
/* else no error but variable's active value was not changed */ /* else no error but variable's active value was not changed */
...@@ -421,23 +414,23 @@ ProcessConfigFile(GucContext context) ...@@ -421,23 +414,23 @@ ProcessConfigFile(GucContext context)
ereport(ERROR, ereport(ERROR,
(errcode(ERRCODE_CONFIG_FILE_ERROR), (errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("configuration file \"%s\" contains errors", errmsg("configuration file \"%s\" contains errors",
ErrorConfFile))); ConfFileWithError)));
else if (apply) else if (apply)
ereport(elevel, ereport(elevel,
(errcode(ERRCODE_CONFIG_FILE_ERROR), (errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("configuration file \"%s\" contains errors; unaffected changes were applied", errmsg("configuration file \"%s\" contains errors; unaffected changes were applied",
ErrorConfFile))); ConfFileWithError)));
else else
ereport(elevel, ereport(elevel,
(errcode(ERRCODE_CONFIG_FILE_ERROR), (errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("configuration file \"%s\" contains errors; no changes were applied", errmsg("configuration file \"%s\" contains errors; no changes were applied",
ErrorConfFile))); ConfFileWithError)));
} }
/* /*
* Calling FreeConfigVariables() any earlier than this can cause problems, * Calling FreeConfigVariables() any earlier than this can cause problems,
* because ErrorConfFile could be pointing to a string that will be freed * because ConfFileWithError could be pointing to a string that will be
* here. * freed here.
*/ */
FreeConfigVariables(head); FreeConfigVariables(head);
} }
...@@ -477,8 +470,11 @@ AbsoluteConfigLocation(const char *location, const char *calling_file) ...@@ -477,8 +470,11 @@ AbsoluteConfigLocation(const char *location, const char *calling_file)
* Read and parse a single configuration file. This function recurses * Read and parse a single configuration file. This function recurses
* to handle "include" directives. * to handle "include" directives.
* *
* See ParseConfigFp for details. This one merely adds opening the * If "strict" is true, treat failure to open the config file as an error,
* file rather than working from a caller-supplied file descriptor, * otherwise just skip the file.
*
* See ParseConfigFp for further details. This one merely adds opening the
* config file rather than working from a caller-supplied file descriptor,
* and absolute-ifying the path name if necessary. * and absolute-ifying the path name if necessary.
*/ */
bool bool
...@@ -516,12 +512,13 @@ ParseConfigFile(const char *config_file, const char *calling_file, bool strict, ...@@ -516,12 +512,13 @@ ParseConfigFile(const char *config_file, const char *calling_file, bool strict,
errmsg("could not open configuration file \"%s\": %m", errmsg("could not open configuration file \"%s\": %m",
abs_path))); abs_path)));
OK = false; OK = false;
goto cleanup;
} }
else
ereport(LOG, {
(errmsg("skipping missing configuration file \"%s\"", ereport(LOG,
abs_path))); (errmsg("skipping missing configuration file \"%s\"",
abs_path)));
}
goto cleanup; goto cleanup;
} }
...@@ -616,9 +613,7 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel, ...@@ -616,9 +613,7 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
{ {
char *opt_name = NULL; char *opt_name = NULL;
char *opt_value = NULL; char *opt_value = NULL;
ConfigVariable *item, ConfigVariable *item;
*cur_item = NULL,
*prev_item = NULL;
if (token == GUC_EOL) /* empty or comment line */ if (token == GUC_EOL) /* empty or comment line */
continue; continue;
...@@ -701,41 +696,13 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel, ...@@ -701,41 +696,13 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
} }
else else
{ {
/* /* ordinary variable, append to list */
* ordinary variable, append to list. For multiple items of
* same parameter, retain only which comes later.
*/
item = palloc(sizeof *item); item = palloc(sizeof *item);
item->name = opt_name; item->name = opt_name;
item->value = opt_value; item->value = opt_value;
item->filename = pstrdup(config_file); item->filename = pstrdup(config_file);
item->sourceline = ConfigFileLineno-1; item->sourceline = ConfigFileLineno-1;
item->next = NULL; item->next = NULL;
/* Remove the existing item of same parameter from the list */
for (cur_item = *head_p; cur_item; prev_item = cur_item,
cur_item = cur_item->next)
{
if (strcmp(item->name, cur_item->name) == 0)
{
if (prev_item == NULL)
*head_p = cur_item->next;
else
{
prev_item->next = cur_item->next;
/*
* On removing last item in list, we need to update tail
* to ensure that list will be maintianed.
*/
if (prev_item->next == NULL)
*tail_p = prev_item;
}
FreeConfigVariable(cur_item);
break;
}
}
if (*head_p == NULL) if (*head_p == NULL)
*head_p = item; *head_p = item;
else else
...@@ -911,21 +878,6 @@ cleanup: ...@@ -911,21 +878,6 @@ cleanup:
return status; return status;
} }
/*
* Free a ConfigVariable
*/
static void
FreeConfigVariable(ConfigVariable *item)
{
if (item != NULL)
{
pfree(item->name);
pfree(item->value);
pfree(item->filename);
pfree(item);
}
}
/* /*
* Free a list of ConfigVariables, including the names and the values * Free a list of ConfigVariables, including the names and the values
*/ */
...@@ -944,6 +896,18 @@ FreeConfigVariables(ConfigVariable *list) ...@@ -944,6 +896,18 @@ FreeConfigVariables(ConfigVariable *list)
} }
} }
/*
* Free a single ConfigVariable
*/
static void
FreeConfigVariable(ConfigVariable *item)
{
pfree(item->name);
pfree(item->value);
pfree(item->filename);
pfree(item);
}
/* /*
* scanstr * scanstr
......
This diff is collapsed.
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment