Tools 2804 (#84)

* fix: tools-2804 prevent leaking heap allocated config defaults when used as shared lib * test: add new defaults function to unit tests * ci: change config_init to config_set * ci: rename _config_default to _config_init * fix: update c client to include event loop mem leak fix
aerospike · Dec 23, 2023 · e98716f · e98716f
1 parent 63219b7
commit e98716f
Show file tree

Hide file tree

Showing 14 changed files with 223 additions and 122 deletions.
diff --git a/include/backup_config.h b/include/backup_config.h
@@ -197,7 +197,7 @@ typedef struct backup_config {
  * The backup_config_t struct returned by this method is always destroyable (and
  * should be destroyed) regardless of the return value
  */
-int backup_config_init(int argc, char* argv[], backup_config_t* conf);
+int backup_config_set(int argc, char* argv[], backup_config_t* conf);
 
 /*
  * Validates the backup config, checking for mutually exclusive options,
@@ -207,7 +207,18 @@ int backup_config_init(int argc, char* argv[], backup_config_t* conf);
  */
 int backup_config_validate(backup_config_t* conf);
 
-void backup_config_default(backup_config_t* conf);
+/* backup_config_set_defaults sets conf fields that are heap allocated
+ * to their default value. This is used internally to allow users of the shared library
+ * this function should be called after backup_config_init and before backup_config_set
+ * to set their conf values without leaking the heap allocated default values
+ */
+void backup_config_set_heap_defaults(backup_config_t *conf);
+
+/*
+ * backup_config_init initializes all conf fields to their
+ * zero value or a default value.
+ */
+void backup_config_init(backup_config_t* conf);
 
 void backup_config_destroy(backup_config_t* conf);
 

diff --git a/include/conf.h b/include/conf.h
@@ -39,7 +39,7 @@ extern "C" {
 // Globals.
 //
 
-extern char *DEFAULTPASSWORD;
+extern char *DEFAULT_PASSWORD;
 
 
 //==========================================================

diff --git a/include/restore_config.h b/include/restore_config.h
@@ -188,7 +188,7 @@ typedef struct restore_config {
  * The restore_config_t struct returned by this method is always destroyable (and
  * should be destroyed) regardless of the return value
  */
-int restore_config_init(int argc, char* argv[], restore_config_t* conf);
+int restore_config_set(int argc, char* argv[], restore_config_t* conf);
 
 /*
  * Validates the restore config, checking for mutually exclusive options,
@@ -198,7 +198,18 @@ int restore_config_init(int argc, char* argv[], restore_config_t* conf);
  */
 int restore_config_validate(restore_config_t *conf);
 
-void restore_config_default(restore_config_t* conf);
+/* restore_config_set_defaults sets conf fields that are heap allocated
+ * to their default value. This is used internally to allow users of the shared library
+ * this function should be called after restore_config_init and before restore_config_set
+ * to set their conf values without leaking the heap allocated default values
+ */
+void restore_config_set_heap_defaults(restore_config_t *conf);
+
+/*
+ * restore_config_init initializes all conf fields to their
+ * zero value or a default value.
+ */
+void restore_config_init(restore_config_t* conf);
 
 void restore_config_destroy(restore_config_t* conf);
 

diff --git a/modules/c-client b/modules/c-client
diff --git a/src/backup.c b/src/backup.c
@@ -209,7 +209,7 @@ backup_main(int32_t argc, char **argv)
 
 	backup_config_t conf;
 
-	int backup_config_res = backup_config_init(argc, argv, &conf);
+	int backup_config_res = backup_config_set(argc, argv, &conf);
 	if (backup_config_res != 0) {
 		if (backup_config_res == BACKUP_CONFIG_INIT_EXIT) {
 			res = EXIT_SUCCESS;
@@ -259,6 +259,7 @@ backup_status_t*
 backup_run(backup_config_t* conf) {
 	as_vector_init(&g_globals, sizeof(backup_globals_t), 1);
 
+	backup_config_set_heap_defaults(conf);
 	backup_status_t* status = start_backup(conf);
 
 	file_proxy_cloud_shutdown();

diff --git a/src/backup_config.c b/src/backup_config.c
@@ -49,7 +49,7 @@ static void usage(const char *name);
 //
 
 int
-backup_config_init(int argc, char* argv[], backup_config_t* conf)
+backup_config_set(int argc, char* argv[], backup_config_t* conf)
 {
 	static struct option options[] = {
 
@@ -162,7 +162,8 @@ backup_config_init(int argc, char* argv[], backup_config_t* conf)
 		{ NULL, 0, NULL, 0 }
 	};
 
-	backup_config_default(conf);
+	backup_config_init(conf);
+	backup_config_set_heap_defaults(conf);
 
 	int32_t opt;
 	int64_t tmp;
@@ -377,7 +378,7 @@ backup_config_init(int argc, char* argv[], backup_config_t* conf)
 					// No password specified should
 					// force it to default password
 					// to trigger prompt.
-					conf->password = safe_strdup(DEFAULTPASSWORD);
+					conf->password = safe_strdup(DEFAULT_PASSWORD);
 				}
 			}
 			break;
@@ -665,7 +666,7 @@ backup_config_init(int argc, char* argv[], backup_config_t* conf)
 				} else {
 					// No password specified should force it to default password
 					// to trigger prompt.
-					conf->tls.keyfile_pw = safe_strdup(DEFAULTPASSWORD);
+					conf->tls.keyfile_pw = safe_strdup(DEFAULT_PASSWORD);
 				}
 			}
 			break;
@@ -820,10 +821,6 @@ backup_config_validate(backup_config_t* conf)
 		conf->port = DEFAULT_PORT;
 	}
 
-	if (conf->host == NULL) {
-		conf->host = safe_strdup(DEFAULT_HOST);
-	}
-
 	if (conf->ns[0] == 0 && !conf->remove_artifacts) {
 		err("Please specify a namespace (-n option)");
 		return BACKUP_CONFIG_VALIDATE_FAILURE;
@@ -923,13 +920,32 @@ backup_config_validate(backup_config_t* conf)
 }
 
 void
-backup_config_default(backup_config_t* conf)
+backup_config_set_heap_defaults(backup_config_t* conf) {
+	if (conf->host == NULL) {
+		conf->host = safe_strdup(DEFAULT_HOST);
+	}
+
+	if (conf->password == NULL) {
+		conf->password = safe_strdup(DEFAULT_PASSWORD);
+	}
+
+	if (conf->secret_cfg.addr == NULL) {
+		conf->secret_cfg.addr = safe_strdup(DEFAULT_SECRET_AGENT_HOST);
+	}
+
+	if (conf->secret_cfg.port == NULL) {
+		conf->secret_cfg.port = safe_strdup(DEFAULT_SECRET_AGENT_PORT);
+	}
+}
+
+void
+backup_config_init(backup_config_t* conf)
 {
 	conf->host = NULL;
 	conf->port = -1;
 	conf->use_services_alternate = false;
 	conf->user = NULL;
-	conf->password = safe_strdup(DEFAULTPASSWORD);
+	conf->password = NULL;
 	conf->auth_mode = NULL;
 
 	conf->s3_region = NULL;
@@ -988,8 +1004,6 @@ backup_config_default(backup_config_t* conf)
 	conf->retry_delay = 0;
 
 	sa_cfg_init(&conf->secret_cfg);
-	conf->secret_cfg.addr = safe_strdup(DEFAULT_SECRET_AGENT_HOST);
-	conf->secret_cfg.port = safe_strdup(DEFAULT_SECRET_AGENT_PORT);
 }
 
 void

diff --git a/src/backup_status.c b/src/backup_status.c
@@ -319,7 +319,7 @@ backup_status_init(backup_status_t* status, backup_config_t* conf)
 
 	char* password;
 	if (conf->user) {
-		if (strcmp(conf->password, DEFAULTPASSWORD) == 0) {
+		if (strcmp(conf->password, DEFAULT_PASSWORD) == 0) {
 			password = getpass("Enter Password: ");
 		}
 		else {
@@ -334,7 +334,7 @@ backup_status_init(backup_status_t* status, backup_config_t* conf)
 
 	if (conf->tls.keyfile && conf->tls.keyfile_pw) {
 		char* tls_keyfile_pw;
-		if (strcmp(conf->tls.keyfile_pw, DEFAULTPASSWORD) == 0) {
+		if (strcmp(conf->tls.keyfile_pw, DEFAULT_PASSWORD) == 0) {
 			tls_keyfile_pw = getpass("Enter TLS-Keyfile Password: ");
 		}
 		else {

diff --git a/src/conf.c b/src/conf.c
@@ -59,7 +59,7 @@
 // Globals.
 //
 
-char *DEFAULTPASSWORD = "SomeDefaultRandomPassword";
+char *DEFAULT_PASSWORD = "SomeDefaultRandomPassword";
 
 
 //=========================================================
@@ -124,6 +124,8 @@ config_from_file(void *c, const char *instance, const char *fname,
 
 		if (is_backup) {
 
+			backup_config_set_heap_defaults((backup_config_t*)c);
+
 			sa_cfg* secret_cfg = &((backup_config_t*)c)->secret_cfg;
 			if (! config_secret_agent(config_table, secret_cfg, instance, errbuf)) {
 				status = false;
@@ -141,6 +143,8 @@ config_from_file(void *c, const char *instance, const char *fname,
 			}
 		} else {
 
+			restore_config_set_heap_defaults((restore_config_t*)c);
+
 			sa_cfg* secret_cfg = &((restore_config_t*)c)->secret_cfg;
 			if (! config_secret_agent(config_table, secret_cfg, instance, errbuf)) {
 				status = false;

diff --git a/src/restore.c b/src/restore.c
@@ -83,7 +83,7 @@ restore_main(int32_t argc, char **argv)
 
 	restore_config_t conf;
 
-	int restore_config_res = restore_config_init(argc, argv, &conf);
+	int restore_config_res = restore_config_set(argc, argv, &conf);
 	if (restore_config_res != 0) {
 		if (restore_config_res == RESTORE_CONFIG_INIT_EXIT) {
 			res = EXIT_SUCCESS;
@@ -129,6 +129,7 @@ restore_main(int32_t argc, char **argv)
  */
 restore_status_t*
 restore_run(restore_config_t *conf) {
+	restore_config_set_heap_defaults(conf);
 	restore_status_t *status = start_restore(conf);
 	file_proxy_cloud_shutdown();
 

diff --git a/src/restore_config.c b/src/restore_config.c
@@ -50,7 +50,7 @@ static void usage(const char* name);
 //
 
 int
-restore_config_init(int argc, char* argv[], restore_config_t* conf)
+restore_config_set(int argc, char* argv[], restore_config_t* conf)
 {
 	static struct option options[] = {
 
@@ -159,7 +159,8 @@ restore_config_init(int argc, char* argv[], restore_config_t* conf)
 		{ NULL, 0, NULL, 0 }
 	};
 
-	restore_config_default(conf);
+	restore_config_init(conf);
+	restore_config_set_heap_defaults(conf);
 
 	int32_t optcase;
 	int64_t tmp;
@@ -374,7 +375,7 @@ restore_config_init(int argc, char* argv[], restore_config_t* conf)
 					// No password specified should
 					// force it to default password
 					// to trigger prompt.
-					conf->password = safe_strdup(DEFAULTPASSWORD);
+					conf->password = safe_strdup(DEFAULT_PASSWORD);
 				}
 			}
 			break;
@@ -612,7 +613,7 @@ restore_config_init(int argc, char* argv[], restore_config_t* conf)
 					// No password specified should
 					// force it to default password
 					// to trigger prompt.
-					conf->tls.keyfile_pw = safe_strdup(DEFAULTPASSWORD);
+					conf->tls.keyfile_pw = safe_strdup(DEFAULT_PASSWORD);
 				}
 			}
 			break;
@@ -846,13 +847,34 @@ restore_config_validate(restore_config_t *conf) {
 }
 
 void
-restore_config_default(restore_config_t *conf)
+restore_config_set_heap_defaults(restore_config_t *conf) {
+	if (conf->host == NULL) {
+		conf->host = safe_strdup(DEFAULT_HOST);
+	}
+
+	if (conf->password == NULL) {
+		conf->password = safe_strdup(DEFAULT_PASSWORD);
+	}
+
+	if (conf->secret_cfg.addr == NULL) {
+		conf->secret_cfg.addr = safe_strdup(DEFAULT_SECRET_AGENT_HOST);
+	}
+
+	if (conf->secret_cfg.port == NULL) {
+		conf->secret_cfg.port = safe_strdup(DEFAULT_SECRET_AGENT_PORT);
+	}
+}
+
+void
+restore_config_init(restore_config_t *conf)
 {
-	conf->host = safe_strdup(DEFAULT_HOST);
+	memset(conf, 0, sizeof(restore_config_t));
+
+	conf->host = NULL;
 	conf->use_services_alternate = false;
 	conf->port = DEFAULT_PORT;
 	conf->user = NULL;
-	conf->password = safe_strdup(DEFAULTPASSWORD);
+	conf->password = NULL;
 	conf->auth_mode = NULL;
 
 	conf->s3_region = NULL;
@@ -904,8 +926,6 @@ restore_config_default(restore_config_t *conf)
 	conf->tls_name = NULL;
 
 	sa_cfg_init(&conf->secret_cfg);
-	conf->secret_cfg.addr = safe_strdup(DEFAULT_SECRET_AGENT_HOST);
-	conf->secret_cfg.port = safe_strdup(DEFAULT_SECRET_AGENT_PORT);
 }
 
 void

diff --git a/src/restore_status.c b/src/restore_status.c
@@ -425,7 +425,7 @@ _init_as_config(as_config* as_conf, const restore_config_t* conf,
 
 	char* password;
 	if (conf->user) {
-		if (strcmp(conf->password, DEFAULTPASSWORD) == 0) {
+		if (strcmp(conf->password, DEFAULT_PASSWORD) == 0) {
 			password = getpass("Enter Password: ");
 		}
 		else {
@@ -443,7 +443,7 @@ _init_as_config(as_config* as_conf, const restore_config_t* conf,
 	}
 	else if (conf->tls.keyfile && conf->tls.keyfile_pw) {
 		char* keyfile_pw;
-		if (strcmp(conf->tls.keyfile_pw, DEFAULTPASSWORD) == 0) {
+		if (strcmp(conf->tls.keyfile_pw, DEFAULT_PASSWORD) == 0) {
 			keyfile_pw = getpass("Enter TLS-Keyfile Password: ");
 		}
 		else {