Fix a memory leak in BuildNodesArrayValues. (#693)

Author: DimCitus

src/bin/pg_autoctl/pgsql.c

@@ -1491,12 +1491,30 @@ pgsql_drop_replication_slot(PGSQL *pgsql, const char *slotName)
  * We return how many parameters we filled in paramTypes and paramValues from
  * the nodeArray.
  */
-static int
+#define NODEID_MAX_LENGTH 20    /* should allow for bigint digits */
+
+typedef struct nodesArraysValuesParams
+{
+	int count;
+	Oid types[NODE_ARRAY_MAX_COUNT * 2];
+	char *values[NODE_ARRAY_MAX_COUNT * 2];
+
+	/*
+	 * Pre-allocate arrays for the data separately from the values array, which
+	 * needs to be a (const char **) thing rather than a (char [][]) thing,
+	 * because of the pgsql_execute_with_params and libpq APIs.
+	 */
+	char nodeIds[NODE_ARRAY_MAX_COUNT][NODEID_MAX_LENGTH];
+	char lsns[NODE_ARRAY_MAX_COUNT][PG_LSN_MAXLENGTH];
+} nodesArraysValuesParams;
+
+
+static bool
 BuildNodesArrayValues(NodeAddressArray *nodeArray,
-					  Oid *paramTypes, char **paramValues,
+					  nodesArraysValuesParams *sqlParams,
 					  char *values, int size)
 {
-	char buffer[BUFSIZE];
+	char buffer[BUFSIZE] = { 0 };
 	int nodeIndex = 0;
 	int paramIndex = 0;
 	int valuesIndex = 0;
@@ -1509,16 +1527,22 @@ BuildNodesArrayValues(NodeAddressArray *nodeArray,
 	for (nodeIndex = 0; nodeIndex < nodeArray->count; nodeIndex++)
 	{
 		NodeAddress *node = &(nodeArray->nodes[nodeIndex]);
-		IntString nodeIdString = intToString(node->nodeId);
+		char *nodeIdString = intToString(node->nodeId).strValue;
 
 		int idParamIndex = paramIndex;
 		int lsnParamIndex = paramIndex + 1;
 
-		paramTypes[idParamIndex] = INT4OID;
-		paramValues[idParamIndex] = strdup(nodeIdString.strValue);
+		sqlParams->types[idParamIndex] = INT4OID;
+		strlcpy(sqlParams->nodeIds[nodeIndex], nodeIdString, NODEID_MAX_LENGTH);
+
+		/* store the (char *) pointer to the data in values */
+		sqlParams->values[idParamIndex] = sqlParams->nodeIds[nodeIndex];
 
-		paramTypes[lsnParamIndex] = LSNOID;
-		paramValues[lsnParamIndex] = node->lsn;
+		sqlParams->types[lsnParamIndex] = LSNOID;
+		strlcpy(sqlParams->lsns[nodeIndex], node->lsn, PG_LSN_MAXLENGTH);
+
+		/* store the (char *) pointer to the data in values */
+		sqlParams->values[lsnParamIndex] = sqlParams->lsns[nodeIndex];
 
 		valuesIndex += sformat(buffer + valuesIndex, BUFSIZE - valuesIndex,
 							   "%s($%d, $%d%s)",
@@ -1542,6 +1566,9 @@ BuildNodesArrayValues(NodeAddressArray *nodeArray,
 		paramIndex += 2;
 	}
 
+	/* count how many parameters where appended to the VALUES() parts */
+	sqlParams->count = paramIndex;
+
 	/* when we didn't find any node to process, return our empty set */
 	if (paramIndex == 0)
 	{
@@ -1563,7 +1590,8 @@ BuildNodesArrayValues(NodeAddressArray *nodeArray,
 			return false;
 		}
 	}
-	return paramIndex;
+
+	return true;
 }
 
 
@@ -1615,13 +1643,14 @@ pgsql_replication_slot_create_and_drop(PGSQL *pgsql, NodeAddressArray *nodeArray
 		"SELECT 'drop', slot_name, NULL::pg_lsn FROM dropped";
 	/* *INDENT-ON* */
 
-	Oid paramTypes[NODE_ARRAY_MAX_COUNT * 2] = { 0 };
-	const char *paramValues[NODE_ARRAY_MAX_COUNT * 2] = { 0 };
+	nodesArraysValuesParams sqlParams = { 0 };
 	ReplicationSlotMaintainContext context = { 0 };
 
-	int paramCount = BuildNodesArrayValues(nodeArray,
-										   paramTypes, (char **) paramValues,
-										   values, BUFSIZE);
+	if (!BuildNodesArrayValues(nodeArray, &sqlParams, values, BUFSIZE))
+	{
+		/* errors have already been logged */
+		return false;
+	}
 
 	/* add the computed ($1,$2), ... string to the query "template" */
 	int bytes = sformat(sql, 2 * BUFSIZE, sqlTemplate, values);
@@ -1635,7 +1664,9 @@ pgsql_replication_slot_create_and_drop(PGSQL *pgsql, NodeAddressArray *nodeArray
 	}
 
 	return pgsql_execute_with_params(pgsql, sql,
-									 paramCount, paramTypes, paramValues,
+									 sqlParams.count,
+									 sqlParams.types,
+									 (const char **) sqlParams.values,
 									 &context,
 									 parseReplicationSlotMaintain);
 }
@@ -1686,13 +1717,14 @@ pgsql_replication_slot_maintain(PGSQL *pgsql, NodeAddressArray *nodeArray)
 		"SELECT 'advance', slot_name, end_lsn FROM advanced ";
 	/* *INDENT-ON* */
 
-	Oid paramTypes[NODE_ARRAY_MAX_COUNT * 2] = { 0 };
-	const char *paramValues[NODE_ARRAY_MAX_COUNT * 2] = { 0 };
+	nodesArraysValuesParams sqlParams = { 0 };
 	ReplicationSlotMaintainContext context = { 0 };
 
-	int paramCount = BuildNodesArrayValues(nodeArray,
-										   paramTypes, (char **) paramValues,
-										   values, BUFSIZE);
+	if (!BuildNodesArrayValues(nodeArray, &sqlParams, values, BUFSIZE))
+	{
+		/* errors have already been logged */
+		return false;
+	}
 
 	/* add the computed ($1,$2), ... string to the query "template" */
 	int bytes = sformat(sql, 2 * BUFSIZE, sqlTemplate, values);
@@ -1706,7 +1738,9 @@ pgsql_replication_slot_maintain(PGSQL *pgsql, NodeAddressArray *nodeArray)
 	}
 
 	return pgsql_execute_with_params(pgsql, sql,
-									 paramCount, paramTypes, paramValues,
+									 sqlParams.count,
+									 sqlParams.types,
+									 (const char **) sqlParams.values,
 									 &context,
 									 parseReplicationSlotMaintain);
 }