Fix null pointer caused by partial initialization of ConnParamsHashEntry (#2789)

It has been reported a null pointer dereference could be triggered in FreeConnParamsHashEntryFields. Likely cause is an error in GetConnParams which will leave the cached ConnParamsHashEntry in a state that would cause the null pointer dereference in a subsequent connection establishment to the same server. This has been simulated by inserting ereport(ERROR, ...) at certain places in the code.

Not only would ConnParamsHashEntry be in a state that would cause a crash, it was also leaking memory in the ConnectionContext due to the loss of pointers as they are only stored on the ConnParamsHashEntry at the end of the function.

This patch rewrites both the GetConnParams to store pointers 'durably' at every point in the code so that an error would not lose the pointer as well as FreeConnParamsHashEntryFields in a way that it can clear half initialised ConnParamsHashEntry's in a safer manner.

src/backend/distributed/connection/connection_configuration.c

@@ -229,12 +229,16 @@ void
 GetConnParams(ConnectionHashKey *key, char ***keywords, char ***values,
 			  Index *runtimeParamStart, MemoryContext context)
 {
-	/* make space for the port as a string: sign, 10 digits, NUL */
-	char *nodePortString = MemoryContextAlloc(context, 12 * sizeof(char *));
+	/*
+	 * make space for the port as a string: sign, 10 digits, NUL. We keep it on the stack
+	 * till we can later copy it to the right context. By having the declaration here
+	 * already we can add a pointer to the runtimeValues.
+	 */
+	char nodePortString[12] = "";
 
 	/*
 	 * This function has three sections:
-	 *   - Initialize the keywords and values with copies of global parameters
+	 *   - Initialize the keywords and values (to be copied later) of global parameters
 	 *   - Append user/host-specific parameters calculated from the given key
 	 *   - (Enterprise-only) append user/host-specific authentication params
 	 *
@@ -247,28 +251,34 @@ GetConnParams(ConnectionHashKey *key, char ***keywords, char ***values,
 	 * invalidated during config reloads.
 	 */
 	const char *runtimeKeywords[] = {
-		MemoryContextStrdup(context, "host"),
-		MemoryContextStrdup(context, "port"),
-		MemoryContextStrdup(context, "dbname"),
-		MemoryContextStrdup(context, "user"),
-		MemoryContextStrdup(context, "client_encoding")
+		"host",
+		"port",
+		"dbname",
+		"user",
+		"client_encoding"
 	};
 	const char *runtimeValues[] = {
-		MemoryContextStrdup(context, key->hostname),
+		key->hostname,
 		nodePortString,
-		MemoryContextStrdup(context, key->database),
-		MemoryContextStrdup(context, key->user),
-		MemoryContextStrdup(context, GetDatabaseEncodingName())
+		key->database,
+		key->user,
+		GetDatabaseEncodingName()
 	};
 
 	/*
-	 * Declare local params for readability; we'll assign to outparams at end.
+	 * Declare local params for readability;
+	 *
+	 * assignment is done directly to not loose the pointers if any of the later
+	 * allocations cause an error. FreeConnParamsHashEntryFields knows about the
+	 * possibility of half initialized keywords or values and correctly reclaims them when
+	 * the cache is reused.
+	 *
 	 * Need to zero enough space for all possible libpq parameters.
 	 */
-	char **connKeywords = MemoryContextAllocZero(context, ConnParams.maxSize *
-												 sizeof(char *));
-	char **connValues = MemoryContextAllocZero(context, ConnParams.maxSize *
-											   sizeof(char *));
+	char **connKeywords = *keywords = MemoryContextAllocZero(context, ConnParams.maxSize *
+															 sizeof(char *));
+	char **connValues = *values = MemoryContextAllocZero(context, ConnParams.maxSize *
+														 sizeof(char *));
 
 	/* auth keywords will begin after global and runtime ones are appended */
 	Index authParamsIdx = ConnParams.size + lengthof(runtimeKeywords);
@@ -293,26 +303,27 @@ GetConnParams(ConnectionHashKey *key, char ***keywords, char ***values,
 		connValues[paramIndex] = ConnParams.values[paramIndex];
 	}
 
-	/* remember where global/GUC params end and runtime ones start */
+	/*
+	 * remember where global/GUC params end and runtime ones start, all entries after this
+	 * point should be allocated in context and will be freed upon
+	 * FreeConnParamsHashEntryFields
+	 */
 	*runtimeParamStart = ConnParams.size;
 
-	/* second step: begin at end of global params and copy runtime ones */
+	/* second step: begin after global params and copy runtime params into our context */
 	for (runtimeParamIndex = 0;
 		 runtimeParamIndex < lengthof(runtimeKeywords);
 		 runtimeParamIndex++)
 	{
-		/* copy the keyword&value pointers to the new array */
+		/* copy the keyword & value into our context and append to the new array */
 		connKeywords[ConnParams.size + runtimeParamIndex] =
-			(char *) runtimeKeywords[runtimeParamIndex];
+			MemoryContextStrdup(context, runtimeKeywords[runtimeParamIndex]);
 		connValues[ConnParams.size + runtimeParamIndex] =
-			(char *) runtimeValues[runtimeParamIndex];
+			MemoryContextStrdup(context, runtimeValues[runtimeParamIndex]);
 	}
 
 	/* final step: add terminal NULL, required by libpq */
 	connKeywords[authParamsIdx] = connValues[authParamsIdx] = NULL;
-
-	*keywords = connKeywords;
-	*values = connValues;
 }
 
 

src/backend/distributed/connection/connection_management.c

@@ -958,23 +958,42 @@ StartConnectionEstablishment(ConnectionHashKey *key)
 static void
 FreeConnParamsHashEntryFields(ConnParamsHashEntry *entry)
 {
-	char **keyword = &entry->keywords[entry->runtimeParamStart];
-	char **value = &entry->values[entry->runtimeParamStart];
+	char **keyword = NULL;
+	char **value = NULL;
 
-	while (*keyword != NULL)
+	/*
+	 * if there was a memory error during the initialization of ConnParamHashEntry in
+	 * GetConnParams the keywords or values might not have been initialized completely.
+	 * We check if they have been initialized before freeing them.
+	 *
+	 * We only iteratively free the lists starting at the index pointed to by
+	 * entry->runtimeParamStart as all entries before are settings that are managed
+	 * separately.
+	 */
+
+	if (entry->keywords != NULL)
 	{
-		pfree(*keyword);
-		keyword++;
+		keyword = &entry->keywords[entry->runtimeParamStart];
+		while (*keyword != NULL)
+		{
+			pfree(*keyword);
+			keyword++;
+		}
+		pfree(entry->keywords);
+		entry->keywords = NULL;
 	}
 
-	while (*value != NULL)
+	if (entry->values != NULL)
 	{
-		pfree(*value);
-		value++;
+		value = &entry->values[entry->runtimeParamStart];
+		while (*value != NULL)
+		{
+			pfree(*value);
+			value++;
+		}
+		pfree(entry->values);
+		entry->values = NULL;
 	}
-
-	pfree(entry->keywords);
-	pfree(entry->values);
 }