Plug minimal memleak in keeper's configuration (#583)

Admittedly this leak could have been plugged with a couple of well placed calls
to the now obsolete keeper_config_destroy() function. However, given the
lifecycle and globality of the struct the variable is found, ownership is
blurred in some cases.

Instead the leaking variable is now living in the stack along with the rest. A
new variable for its length is added instead of MAXCONNINFO which seems a bit
too excessive. Alignment concerns aside, some relief from the stack stress might
be desirable.

Finally, the necessity for keeper_config_destroy is now removed. Thus the
function itself is removed for the benefit of the reader of the codebase.

Co-authored-by: Georgios Kokolatos <gkokolatos@pm.com>

Author: gkokolatos

src/bin/pg_autoctl/cli_common.c

@@ -1405,8 +1405,6 @@ cli_drop_local_node(KeeperConfig *config, bool dropAndDestroy)
 		log_info("HINT: to completely remove your local Postgres instance and "
 				 "setup, consider `pg_autoctl drop node --destroy`");
 	}
-
-	keeper_config_destroy(config);
 }
 
 

src/bin/pg_autoctl/cli_config.c

@@ -221,8 +221,6 @@ cli_config_check(int argc, char **argv)
 			exit(EXIT_CODE_INTERNAL_ERROR);
 		}
 	}
-
-	keeper_config_destroy(&config);
 }
 
 
@@ -470,7 +468,6 @@ cli_keeper_config_get(int argc, char **argv)
 				}
 			}
 
-			keeper_config_destroy(&config);
 			break;
 		}
 
@@ -493,7 +490,6 @@ cli_keeper_config_get(int argc, char **argv)
 				exit(EXIT_CODE_BAD_ARGS);
 			}
 
-			keeper_config_destroy(&config);
 			break;
 		}
 
@@ -549,7 +545,6 @@ cli_monitor_config_get(int argc, char **argv)
 				fformat(stdout, "\n");
 			}
 
-			keeper_config_destroy(&kconfig);
 			break;
 		}
 
@@ -572,7 +567,6 @@ cli_monitor_config_get(int argc, char **argv)
 				exit(EXIT_CODE_BAD_ARGS);
 			}
 
-			keeper_config_destroy(&kconfig);
 			break;
 		}
 
@@ -680,8 +674,6 @@ cli_keeper_config_set(int argc, char **argv)
 			log_error("Failed to lookup option %s", argv[0]);
 			exit(EXIT_CODE_BAD_ARGS);
 		}
-
-		keeper_config_destroy(&config);
 	}
 }
 
@@ -742,7 +734,5 @@ cli_monitor_config_set(int argc, char **argv)
 			log_error("Failed to lookup option %s", argv[0]);
 			exit(EXIT_CODE_BAD_ARGS);
 		}
-
-		keeper_config_destroy(&kconfig);
 	}
 }

src/bin/pg_autoctl/defaults.h

@@ -40,6 +40,7 @@
 #define GROUP_ID_DEFAULT 0
 #define POSTGRES_CONNECT_TIMEOUT "2"
 #define MAXIMUM_BACKUP_RATE "100M"
+#define MAXIMUM_BACKUP_RATE_LEN 32
 
 
 /*

src/bin/pg_autoctl/keeper.c

@@ -2034,9 +2034,9 @@ keeper_config_accept_new(Keeper *keeper, KeeperConfig *newConfig)
 				 "used to be \"%s\"",
 				 newConfig->maximum_backup_rate, config->maximum_backup_rate);
 
-		/* note: strneq checks args are not NULL, it's safe to proceed */
-		free(config->maximum_backup_rate);
-		config->maximum_backup_rate = strdup(newConfig->maximum_backup_rate);
+		strlcpy(config->maximum_backup_rate,
+				newConfig->maximum_backup_rate,
+				MAXIMUM_BACKUP_RATE_LEN);
 	}
 
 	/*
@@ -2190,9 +2190,6 @@ keeper_reload_configuration(Keeper *keeper, bool firstLoop, bool doInit)
 					 "continuing with the same configuration.",
 					 config->pathnames.config);
 		}
-
-		/* we're done the newConfig now */
-		keeper_config_destroy(&newConfig);
 	}
 	else
 	{

src/bin/pg_autoctl/keeper_config.c

@@ -133,8 +133,9 @@
 							   REPLICATION_PASSWORD_DEFAULT)
 
 #define OPTION_REPLICATION_MAXIMUM_BACKUP_RATE(config) \
-	make_string_option_default("replication", "maximum_backup_rate", NULL, \
-							   false, &config->maximum_backup_rate, \
+	make_strbuf_option_default("replication", "maximum_backup_rate", NULL, \
+							   false, MAXIMUM_BACKUP_RATE_LEN, \
+							   config->maximum_backup_rate, \
 							   MAXIMUM_BACKUP_RATE)
 
 #define OPTION_REPLICATION_BACKUP_DIR(config) \
@@ -684,19 +685,6 @@ keeper_config_update(KeeperConfig *config, int nodeId, int groupId)
 }
 
 
-/*
- * keeper_config_destroy frees memory that may be dynamically allocated.
- */
-void
-keeper_config_destroy(KeeperConfig *config)
-{
-	if (config->maximum_backup_rate != NULL)
-	{
-		free(config->maximum_backup_rate);
-	}
-}
-
-
 /*
  * keeper_config_init_nodekind initializes the config->nodeKind and
  * config->pgSetup.pgKind values from the configuration file or command line

src/bin/pg_autoctl/keeper_config.h

@@ -14,6 +14,7 @@
 #include <stdbool.h>
 
 #include "config.h"
+#include "defaults.h"
 #include "pgctl.h"
 #include "pgsql.h"
 
@@ -40,7 +41,7 @@ typedef struct KeeperConfig
 	/* PostgreSQL replication / tooling setup */
 	char replication_slot_name[MAXCONNINFO];
 	char replication_password[MAXCONNINFO];
-	char *maximum_backup_rate;
+	char maximum_backup_rate[MAXIMUM_BACKUP_RATE_LEN];
 	char backupDirectory[MAXPGPATH];
 
 	/* pg_autoctl timeouts */
@@ -84,7 +85,6 @@ bool keeper_config_set_setting(KeeperConfig *config,
 
 bool keeper_config_merge_options(KeeperConfig *config, KeeperConfig *options);
 bool keeper_config_update(KeeperConfig *config, int nodeId, int groupId);
-void keeper_config_destroy(KeeperConfig *config);
 bool keeper_config_update_with_absolute_pgdata(KeeperConfig *config);
 
 #endif /* KEEPER_CONFIG_H */

src/bin/pg_autoctl/pgsql.h

@@ -164,7 +164,7 @@ typedef struct ReplicationSource
 	char userName[NAMEDATALEN];
 	char slotName[MAXCONNINFO];
 	char password[MAXCONNINFO];
-	char maximumBackupRate[MAXCONNINFO];
+	char maximumBackupRate[MAXIMUM_BACKUP_RATE_LEN];
 	char backupDir[MAXCONNINFO];
 	char applicationName[MAXCONNINFO];
 	char targetLSN[PG_LSN_MAXLENGTH];

src/bin/pg_autoctl/primary_standby.c

@@ -804,7 +804,9 @@ standby_init_replication_source(LocalPostgresServer *postgres,
 	}
 
 	strlcpy(upstream->slotName, slotName, MAXCONNINFO);
-	strlcpy(upstream->maximumBackupRate, maximumBackupRate, MAXCONNINFO);
+	strlcpy(upstream->maximumBackupRate,
+			maximumBackupRate,
+			MAXIMUM_BACKUP_RATE_LEN);
 	strlcpy(upstream->backupDir, backupDirectory, MAXCONNINFO);
 
 	if (targetLSN != NULL)